You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@texera.apache.org by gi...@apache.org on 2026/07/30 03:58:48 UTC
(texera) 01/02: chore(file-service): remove unused DatasetFileNode physical-node builder (#7036)
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 <xi...@uci.edu>
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(("alice@texera.com", "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 "/alice@texera.com/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()
- }
- }
- }
}