Copilot commented on code in PR #4263:
URL: https://github.com/apache/texera/pull/4263#discussion_r2907462899
##########
amber/src/main/scala/org/apache/texera/amber/engine/common/Utils.scala:
##########
@@ -36,25 +35,52 @@ object Utils extends LazyLogging {
*
* @return the real absolute path to amber home directory
*/
+
+ import java.nio.file.{Files, Path, Paths}
+ import scala.jdk.CollectionConverters._
+ import scala.util.Using
+
lazy val amberHomePath: Path = {
val currentWorkingDirectory = Paths.get(".").toRealPath()
- // check if the current directory is the amber home path
+
if (isAmberHomePath(currentWorkingDirectory)) {
currentWorkingDirectory
} else {
- // from current path's parent directory, search its children to find
amber home path
- // current max depth is set to 2 (current path's siblings and direct
children)
- val searchChildren = Files
- .walk(currentWorkingDirectory.getParent, 2)
- .filter((path: Path) => isAmberHomePath(path))
- .findAny
- if (searchChildren.isPresent) {
- searchChildren.get
- } else {
+ val parent = Option(currentWorkingDirectory.getParent).getOrElse {
throw new RuntimeException(
- "Finding texera home path failed. Current working directory is " +
currentWorkingDirectory
+ s"Cannot search for texera home from filesystem root:
$currentWorkingDirectory"
)
}
+
+ // Pass 1: prefer the closest prefix (deepest ancestor) of
currentWorkingDirectory
+ val closestPrefix: Option[Path] =
+ Using.resource(Files.walk(parent, 2)) { stream =>
+ stream
+ .iterator()
+ .asScala
+ .filter(path => isAmberHomePath(path))
+ .map(_.toRealPath()) // normalize after filtering
+ .filter(path => path.startsWith(currentWorkingDirectory))
+ .maxByOption(_.getNameCount) // deepest prefix = closest ancestor
+ }
+
+ closestPrefix.getOrElse {
+ // Pass 2: fallback to any valid match
+ val anyMatch =
+ Using.resource(Files.walk(parent, 2)) { stream =>
+ stream
+ .filter((path: Path) => isAmberHomePath(path))
+ .findAny()
+ }
Review Comment:
`Files.walk(parent, 2)` is executed twice (pass 1 and pass 2). Since
directory walking can be relatively expensive (and can observe different FS
state between passes), consider walking once, collecting the matching
candidates, and then applying the preferred-selection logic + fallback
in-memory.
##########
amber/src/main/scala/org/apache/texera/amber/engine/common/Utils.scala:
##########
@@ -36,25 +35,52 @@ object Utils extends LazyLogging {
*
* @return the real absolute path to amber home directory
*/
+
+ import java.nio.file.{Files, Path, Paths}
+ import scala.jdk.CollectionConverters._
+ import scala.util.Using
+
lazy val amberHomePath: Path = {
val currentWorkingDirectory = Paths.get(".").toRealPath()
- // check if the current directory is the amber home path
+
if (isAmberHomePath(currentWorkingDirectory)) {
currentWorkingDirectory
} else {
- // from current path's parent directory, search its children to find
amber home path
- // current max depth is set to 2 (current path's siblings and direct
children)
- val searchChildren = Files
- .walk(currentWorkingDirectory.getParent, 2)
- .filter((path: Path) => isAmberHomePath(path))
- .findAny
- if (searchChildren.isPresent) {
- searchChildren.get
- } else {
+ val parent = Option(currentWorkingDirectory.getParent).getOrElse {
throw new RuntimeException(
- "Finding texera home path failed. Current working directory is " +
currentWorkingDirectory
+ s"Cannot search for texera home from filesystem root:
$currentWorkingDirectory"
)
}
+
+ // Pass 1: prefer the closest prefix (deepest ancestor) of
currentWorkingDirectory
+ val closestPrefix: Option[Path] =
+ Using.resource(Files.walk(parent, 2)) { stream =>
+ stream
+ .iterator()
+ .asScala
+ .filter(path => isAmberHomePath(path))
+ .map(_.toRealPath()) // normalize after filtering
+ .filter(path => path.startsWith(currentWorkingDirectory))
+ .maxByOption(_.getNameCount) // deepest prefix = closest ancestor
+ }
+
+ closestPrefix.getOrElse {
+ // Pass 2: fallback to any valid match
+ val anyMatch =
+ Using.resource(Files.walk(parent, 2)) { stream =>
+ stream
+ .filter((path: Path) => isAmberHomePath(path))
+ .findAny()
+ }
+
+ if (anyMatch.isPresent) {
+ anyMatch.get().toRealPath()
+ } else {
+ throw new RuntimeException(
+ s"Finding texera home path failed. Current working directory is
$currentWorkingDirectory"
+ )
+ }
+ }
}
Review Comment:
This change alters path-resolution behavior in a way that’s easy to regress
(especially with multiple sibling repos). Since the `amber` module already has
ScalaTest coverage, adding a focused unit test that creates a temp directory
tree and sets `user.dir` to validate the selection logic would help prevent
future nondeterministic failures.
##########
amber/src/main/scala/org/apache/texera/amber/engine/common/Utils.scala:
##########
@@ -36,25 +35,52 @@ object Utils extends LazyLogging {
*
* @return the real absolute path to amber home directory
*/
+
+ import java.nio.file.{Files, Path, Paths}
+ import scala.jdk.CollectionConverters._
+ import scala.util.Using
Review Comment:
These new imports are placed inside the `Utils` object, while other files in
this package keep imports at the top of the file. Consider moving them to the
top-level import section for consistency and to make dependencies easier to
spot.
##########
amber/src/main/scala/org/apache/texera/amber/engine/common/Utils.scala:
##########
@@ -36,25 +35,52 @@ object Utils extends LazyLogging {
*
* @return the real absolute path to amber home directory
*/
+
+ import java.nio.file.{Files, Path, Paths}
+ import scala.jdk.CollectionConverters._
+ import scala.util.Using
+
lazy val amberHomePath: Path = {
val currentWorkingDirectory = Paths.get(".").toRealPath()
- // check if the current directory is the amber home path
+
if (isAmberHomePath(currentWorkingDirectory)) {
currentWorkingDirectory
} else {
- // from current path's parent directory, search its children to find
amber home path
- // current max depth is set to 2 (current path's siblings and direct
children)
- val searchChildren = Files
- .walk(currentWorkingDirectory.getParent, 2)
- .filter((path: Path) => isAmberHomePath(path))
- .findAny
- if (searchChildren.isPresent) {
- searchChildren.get
- } else {
+ val parent = Option(currentWorkingDirectory.getParent).getOrElse {
throw new RuntimeException(
- "Finding texera home path failed. Current working directory is " +
currentWorkingDirectory
+ s"Cannot search for texera home from filesystem root:
$currentWorkingDirectory"
)
Review Comment:
The new exception message refers to "texera home" even though this method is
computing `amberHomePath` and the scaladoc says "amber home directory".
Aligning the wording (e.g., "Amber home" or "Texera/Amber home") would make
this error easier to understand when it appears in logs.
--
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]
