This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/texera.git
commit 3bda8fab59e2581f0f4260cd91503157be51c80d Author: Xinyuan Lin <[email protected]> AuthorDate: Wed Jul 29 20:43:13 2026 -0700 chore(file-service): remove unused DatasetFileNode physical-node builder (#7036) ### What changes were proposed in this PR? `DatasetFileNode` has two tree builders; only one is reachable. This removes the dead one. | Builder | Callers | Action | | --- | --- | --- | | `fromLakeFSRepositoryCommittedObjects` | live — the dataset REST resources | untouched | | `fromPhysicalFileNodes` + private `addNodeToTree` | none outside the spec | **removed** | The dead builder converts a `PhysicalFileNode` tree — the pre-LakeFS, local-JGit representation — into a `DatasetFileNode` tree. Since dataset storage moved to LakeFS, nothing produces that input in production. Also removed, because they become unused: - `import org.apache.texera.amber.core.storage.util.dataset.PhysicalFileNode` and `import java.util` in `DatasetFileNode.scala` - `import PhysicalFileNode`, `import java.nio.file.{Files, Path}` and `import scala.jdk.CollectionConverters._` in the spec - the `fromPhysicalFileNodes` spec block, and the mention of it in the spec's header comment Leaving any of those behind would fail `scalafixAll --check`. Side effect worth noting: the two builders shared ~24 lines of byte-identical owner/dataset/version scaffolding plus two byte-identical local `sortChildren` definitions. Deleting the dead one removes that duplication without needing a refactor. −141 lines, no behaviour change. **Follow-up, deliberately not in this PR.** This drops file-service's only compile-time dependency on `PhysicalFileNode`. Once it lands, `PhysicalFileNode` together with `JGitVersionControl.getRootFileNodeOfCommit` / `createOrGetNode` / `ensureParentChildLink` and `GitVersionControlLocalFileStorage.retrieveRootFileNodesOfVersion` has no non-test caller anywhere. That chain spans `common/workflow-core` and the amber test tree, so it deserves its own review rather than being bundled here. ### Any related issues, documentation, discussions? Closes #7035 ### How was this PR tested? Existing tests only — this PR adds none, since it removes code and the spec case that covered it. Locally, from the repo root with Java 17: - `sbt "scalafixAll --check"` — clean (this is what catches the five now-unused imports). - `sbt scalafmtCheckAll` — clean. - `sbt "FileService/testOnly *DatasetFileNodeSpec"` — 8 tests, all pass (was 9; the removed one covered `fromPhysicalFileNodes`). Verification that nothing references the removed code, re-runnable by a reviewer: ``` git grep -nw "fromPhysicalFileNodes\|addNodeToTree" ``` ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 5) --- .../service/type/dataset/DatasetFileNode.scala | 82 ---------------------- .../service/type/dataset/DatasetFileNodeSpec.scala | 61 +--------------- 2 files changed, 2 insertions(+), 141 deletions(-) diff --git a/file-service/src/main/scala/org/apache/texera/service/type/dataset/DatasetFileNode.scala b/file-service/src/main/scala/org/apache/texera/service/type/dataset/DatasetFileNode.scala index 7c91d30b94..ba789f0e5a 100644 --- a/file-service/src/main/scala/org/apache/texera/service/type/dataset/DatasetFileNode.scala +++ b/file-service/src/main/scala/org/apache/texera/service/type/dataset/DatasetFileNode.scala @@ -20,9 +20,7 @@ package org.apache.texera.service.`type` import io.lakefs.clients.sdk.model.ObjectStats -import org.apache.texera.amber.core.storage.util.dataset.PhysicalFileNode -import java.util import scala.collection.mutable // DatasetFileNode represents a unique file in dataset, its full path is in the format of: @@ -149,86 +147,6 @@ object DatasetFileNode { rootNode.getChildren } - def fromPhysicalFileNodes( - map: Map[(String, String, String), List[PhysicalFileNode]] - ): List[DatasetFileNode] = { - val rootNode = new DatasetFileNode("/", "directory", null, "") - val ownerNodes = mutable.Map[String, DatasetFileNode]() - - map.foreach { - case ((ownerEmail, datasetName, versionName), physicalNodes) => - val ownerNode = ownerNodes.getOrElseUpdate( - ownerEmail, { - val newNode = new DatasetFileNode(ownerEmail, "directory", rootNode, ownerEmail) - rootNode.children = Some(rootNode.getChildren :+ newNode) - newNode - } - ) - - val datasetNode = ownerNode.getChildren.find(_.getName == datasetName).getOrElse { - val newNode = new DatasetFileNode(datasetName, "directory", ownerNode, ownerEmail) - ownerNode.children = Some(ownerNode.getChildren :+ newNode) - newNode - } - - val versionNode = datasetNode.getChildren.find(_.getName == versionName).getOrElse { - val newNode = new DatasetFileNode(versionName, "directory", datasetNode, ownerEmail) - datasetNode.children = Some(datasetNode.getChildren :+ newNode) - newNode - } - - physicalNodes.foreach(node => addNodeToTree(versionNode, node, ownerEmail)) - } - - // Sorting function to sort children of a node alphabetically in descending order - def sortChildren(node: DatasetFileNode): Unit = { - node.children = Some(node.getChildren.sortBy(_.getName)(Ordering.String.reverse)) - node.getChildren.foreach(sortChildren) - } - - // Apply the sorting to the root node - sortChildren(rootNode) - - rootNode.getChildren - } - - private def addNodeToTree( - parentNode: DatasetFileNode, - physicalNode: PhysicalFileNode, - ownerEmail: String - ): Unit = { - val queue = new util.LinkedList[(DatasetFileNode, PhysicalFileNode)]() - queue.add((parentNode, physicalNode)) - - while (!queue.isEmpty) { - val (currentParent, currentPhysicalNode) = queue.poll() - val relativePath = currentPhysicalNode.getRelativePath.toString.split("/").toList - val nodeName = relativePath.last - - val fileType = - if (currentPhysicalNode.isDirectory) "directory" else "file" - val fileSize = - if (fileType == "file") Some(currentPhysicalNode.getSize) else None - val existingNode = currentParent.getChildren.find(child => - child.getName == nodeName && child.getNodeType == fileType - ) - val fileNode = existingNode.getOrElse { - val newNode = new DatasetFileNode( - nodeName, - fileType, - currentParent, - ownerEmail, - fileSize - ) - currentParent.children = Some(currentParent.getChildren :+ newNode) - newNode - } - - // Add children of the current physical node to the queue - currentPhysicalNode.getChildren.forEach(child => queue.add((fileNode, child))) - } - } - /** * Traverses a given list of DatasetFileNode and returns the total size of all files. * diff --git a/file-service/src/test/scala/org/apache/texera/service/type/dataset/DatasetFileNodeSpec.scala b/file-service/src/test/scala/org/apache/texera/service/type/dataset/DatasetFileNodeSpec.scala index 93e08a9afc..e4ec48bbf6 100644 --- a/file-service/src/test/scala/org/apache/texera/service/type/dataset/DatasetFileNodeSpec.scala +++ b/file-service/src/test/scala/org/apache/texera/service/type/dataset/DatasetFileNodeSpec.scala @@ -20,19 +20,13 @@ package org.apache.texera.service.`type` import io.lakefs.clients.sdk.model.ObjectStats -import org.apache.texera.amber.core.storage.util.dataset.PhysicalFileNode import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.matchers.should.Matchers -import java.nio.file.{Files, Path} -import scala.jdk.CollectionConverters._ - // Unit tests for DatasetFileNode: the instance helpers getFilePath, the // constructor's nodeType require guard, and the companion helpers -// calculateTotalSize / fromLakeFSRepositoryCommittedObjects / -// fromPhysicalFileNodes. The LakeFS factory is fixtured with lightweight -// ObjectStats POJOs; the physical-node factory is fixtured against a real -// temporary directory tree because it inspects the filesystem. +// calculateTotalSize / fromLakeFSRepositoryCommittedObjects. The LakeFS +// factory is fixtured with lightweight ObjectStats POJOs. class DatasetFileNodeSpec extends AnyFlatSpec with Matchers { // -- constructor require guard ---------------------------------------------- @@ -131,55 +125,4 @@ class DatasetFileNodeSpec extends AnyFlatSpec with Matchers { // Total size equals the sum of the three files. DatasetFileNode.calculateTotalSize(roots) shouldBe 6L } - - // -- fromPhysicalFileNodes -------------------------------------------------- - - "fromPhysicalFileNodes" should "build a tree from a physical filesystem subtree" in { - // Create a real temp tree: <repo>/dir1/a.csv , because PhysicalFileNode - // uses Files.isDirectory / Files.isRegularFile against the actual path. - val repo: Path = Files.createTempDirectory("dataset-file-node-spec") - try { - val dir1 = Files.createDirectory(repo.resolve("dir1")) - val fileA = Files.write(dir1.resolve("a.csv"), "hello".getBytes) - - val dir1Node = new PhysicalFileNode(repo, dir1, 0L) - // getSize returns the value passed at construction, not the on-disk size. - val fileNode = new PhysicalFileNode(repo, fileA, 5L) - dir1Node.addChildNode(fileNode) - - val roots = DatasetFileNode.fromPhysicalFileNodes( - Map(("[email protected]", "ds", "v1") -> List(dir1Node)) - ) - - roots should have size 1 - val versionNode = roots.head.getChildren - .find(_.getName == "ds") - .get - .getChildren - .find(_.getName == "v1") - .get - - val dirNode = versionNode.getChildren.find(_.getName == "dir1").get - dirNode.getNodeType shouldBe "directory" - - val leaf = dirNode.getChildren.find(_.getName == "a.csv").get - leaf.getNodeType shouldBe "file" - leaf.getSize shouldBe Some(5L) - leaf.getFilePath shouldBe "/[email protected]/ds/v1/dir1/a.csv" - - DatasetFileNode.calculateTotalSize(roots) shouldBe 5L - } finally { - // Best-effort cleanup of the temp tree. - val stream = Files.walk(repo) - try { - stream - .sorted(java.util.Comparator.reverseOrder[Path]()) - .iterator() - .asScala - .foreach(p => Files.deleteIfExists(p)) - } finally { - stream.close() - } - } - } }
