zhang-arvin commented on issue #18256: URL: https://github.com/apache/iceberg/issues/18256#issuecomment-5836108510
PR opened: https://github.com/apache/iceberg/pull/18261 Added `TestResolveBranch` for both Spark 4.1 and 4.2 (identical files). One finding worth a maintainer's eye, since it changes what these tests can assert: while writing them I found the premise in the issue only partly holds on current `main`. - For a path load with `.option("branch", ...)`, `IcebergSource.catalogAndIdentifier` already builds `PathIdentifier(path + "#branch_x")` before analysis runs, so `ResolveBranch` returns the relation **unchanged** and its `PathIdentifier` cases never fire. - `<location>#files` is a metadata-table load, where the branch is never determined (`branch == null`), so the comma-append case is unreachable that way too. - The only route I could reproduce that actually reaches `case path: PathIdentifier` is a branch taken from the **session WAP branch** (`spark.wap.branch`). The tests therefore assert the rewrite that is genuinely observable (`<location>#branch_test`) plus the metadata-selector and write cases with their actual behavior, rather than an assertion that would pass for the wrong reason. If the `path.location.contains("#")` case is intentionally defensive, it may be worth saying so in the code or dropping it; I'd appreciate a pointer if there is a public route I missed. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
