aicam commented on code in PR #7762:
URL: https://github.com/apache/texera/pull/7762#discussion_r3832591345
##########
file-service/src/test/scala/org/apache/texera/service/resource/DatasetResourceSpec.scala:
##########
@@ -3484,4 +3484,37 @@ class DatasetResourceSpec
val commit = LakeFSStorageClient.createCommit(repoName, "main", "commit
all files")
LakeFSStorageClient.retrieveObjectsOfVersion(repoName, commit.getId).size
shouldEqual totalFiles
}
+
+ // Guards the resource-type prefix the file tree is rooted at.
`FileResolver` dispatches on
+ // that leading segment to choose the backing table, so a dataset tree must
stay under
+ // `/datasets/...` now that the builder takes the resource type as a
parameter.
Review Comment:
This comment contradicts the assertion it documents: it says the tree must
stay under `` `/datasets/...` `` (plural), but the assertion on line 3500
checks `` `/dataset/...` `` (singular), which is correct —
`ResourceType.Dataset` is `Value("dataset")` since #7789 singularized the
prefix.
Worth fixing because the comment is the load-bearing explanation for the
test: a reader trusting it would "correct" the assertion to the plural form and
silently break `FileResolver.parsePrefixedPath` dispatch. The same stale plural
appears in the PR description.
##########
file-service/src/main/scala/org/apache/texera/service/type/dataset/DatasetFileNode.scala:
##########
@@ -119,23 +128,27 @@ object DatasetFileNode {
var currentPath = ""
var parentNode: DatasetFileNode = versionNode
- pathParts.foreach { part =>
- currentPath = if (currentPath.isEmpty) part else
s"$currentPath/$part"
+ pathParts.zipWithIndex.foreach {
+ case (part, idx) =>
+ currentPath = if (currentPath.isEmpty) part else
s"$currentPath/$part"
- val isFile = pathParts.last == part
- val nodeType = if (isFile) "file" else "directory"
- val fileSize = if (isFile) Some(obj.getSizeBytes.longValue()) else
None
+ // Positional, not by value: a path that repeats its final
segment (e.g.
+ // "model/model") would otherwise treat the intermediate
directory as the leaf,
+ // giving it the object's size and nesting the real file
underneath it.
+ val isFile = idx == pathParts.length - 1
+ val nodeType = if (isFile) "file" else "directory"
+ val fileSize = if (isFile) Some(obj.getSizeBytes.longValue())
else None
- val existingNode = directoryMap.get(currentPath)
+ val existingNode = directoryMap.get(currentPath)
- val node = existingNode.getOrElse {
- val newNode = new DatasetFileNode(part, nodeType, parentNode,
ownerEmail, fileSize)
- parentNode.children = Some(parentNode.getChildren :+ newNode)
- if (!isFile) directoryMap(currentPath) = newNode
- newNode
- }
+ val node = existingNode.getOrElse {
+ val newNode = new DatasetFileNode(part, nodeType, parentNode,
ownerEmail, fileSize)
+ parentNode.children = Some(parentNode.getChildren :+ newNode)
+ if (!isFile) directoryMap(currentPath) = newNode
Review Comment:
The positional `isFile` fix is right, but the dedupe path next to it still
has the same class of bug for a *prefix* collision (pre-existing, not
introduced here — but this rewritten block is where it would be fixed).
`existingNode` is looked up only in `directoryMap`, and `directoryMap` is
only ever populated for non-leaf segments (`if (!isFile)
directoryMap(currentPath) = newNode` on line 147). So a leaf neither dedupes
against the map nor registers into it.
LakeFS is a key-value store and `validateAndNormalizeFilePathOrThrow` does
not forbid it, so a user can upload both `model` and `model/weights.bin` into
one dataset version. LakeFS lists `model` first:
1. `model` is a leaf → `file` node with the object's size, **not**
registered in `directoryMap`.
2. `model/weights.bin` → at segment `model`, `directoryMap.get("model")`
returns `None`, so a **second** child named `model` is created, this time a
`directory`.
The version node now has two children named `model` (one file, one
directory), which the frontend renders as a duplicated entry. Same failure
shape as the `model/model` case this PR fixes.
A fix could register leaves in the map too (or a separate `nodeMap`) and
reconcile on collision — e.g. if an existing node is a `file` and a deeper
segment needs it as a directory, promote it rather than appending a sibling.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
