fix(spark): return an unqualified basePath from SparkClientFunctionalTestHarness - #19747
fix(spark): return an unqualified basePath from SparkClientFunctionalTestHarness#19747deepakpanda93 wants to merge 2 commits into
Conversation
…TestHarness
SparkClientFunctionalTestHarness#basePath() returned a scheme-qualified path,
unlike HoodieCommonTestHarness, CLIFunctionalTestHarness and the examples
harness, which all return an unqualified one.
The scheme breaks any helper that passes the value to java.nio.file.Paths.
Paths.get("file:///a/b", "c") yields the relative path "file:/a/b/c", with
"file:" read as an ordinary directory name, so FileCreateUtils wrote partition
data under the working directory instead of the table, without failing.
basePath() now returns the unqualified path and baseUri() exposes the URI for
callers that want the scheme. This removes 20 workarounds, including the
basePath() override in TestSparkSampleWritesUtils that cited this ticket and
basePath().substring(7) in TestHoodieSparkRollback.
Call sites that consumed the scheme implicitly are updated too: string
concatenation that relied on the trailing slash toUri() appends for an existing
directory, and substring(5) on metaClient.getBasePath() that stripped the
"file:" prefix. The latter now uses getBasePath().toUri().getPath(), which
yields the path component whether or not a scheme is present.
TestSparkClientFunctionalTestHarness covers the new contract.
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR makes SparkClientFunctionalTestHarness#basePath() return an unqualified path (adding baseUri() for scheme-needing callers), aligning it with the other test harnesses and removing ~20 workarounds that existed only because of the scheme leaking into java.nio.file.Paths. I traced the updated call sites — the string-concatenation spots consistently gained the leading / now that the returned path has no trailing slash, the getHoodieMetaClient calls receive the same value the old URI.create(...).getPath() produced, and the _UTC path now stays under @TempDir. The broad blast radius (many untouched tests inherit basePath()) is best validated by CI, which exercises those paths. No issues flagged from this automated pass — a Hudi committer or PMC member can take it from here for a final review.
cc @yihua
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19747 +/- ##
============================================
+ Coverage 77.95% 77.97% +0.02%
- Complexity 33447 33461 +14
============================================
Files 2539 2539
Lines 140891 140939 +48
Branches 17008 17036 +28
============================================
+ Hits 109829 109898 +69
+ Misses 23401 23383 -18
+ Partials 7661 7658 -3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
deepakpanda93
changed the title
validateFileListingInMetadataTable builds partition paths from basePath() and compares the resulting listing against a direct storage listing. The filesystem qualifies the paths it returns, so the two sides only match when the partition paths carry the scheme too. basePath() supplied it before this branch made the harness return an unqualified path. Build the partition paths from baseUri() instead, which is what that accessor is for. This restores the exact strings the comparison saw previously and keeps the assertion strict rather than making it scheme-insensitive. Caught by test-spark-java17-java-tests-part2 (scala-2.13, spark4.2) on TestHoodieSparkMergeOnReadTableCompaction.
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR makes SparkClientFunctionalTestHarness#basePath() return an unqualified path (adding baseUri() for the scheme form), aligning it with the other test harnesses and removing the Paths.get-driven relative-path workarounds. I traced the string-concatenation call sites (trailing-slash handling in the rollback and _UTC cases), the deliberate baseUri() retention in the MDT listing comparison, and confirmed via a repo-wide grep that all URI.create(basePath())/substring workarounds live only in the edited files. No correctness issues found. A few style/readability suggestions in the inline comments. Please take a look, and this should be ready for a Hudi committer or PMC member to take it from here. Code is clean overall; one minor readability nit on an inline fully-qualified type.
cc @yihua
| "partition path must sit under the table directory, but was " + partition); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
🤖 nit: since there's no competing Path type imported here, could you add import java.nio.file.Path; and drop the fully-qualified name for readability?
4 participants
Describe the issue this Pull Request addresses
Closes #15888 (HUDI-6042).
SparkClientFunctionalTestHarness#basePath()returns a scheme-qualified path, unlikeHoodieCommonTestHarness,CLIFunctionalTestHarnessand the examples harness, which all return an unqualified one.The scheme breaks any helper that passes the value to
java.nio.file.Paths:file:is read as an ordinary directory name, so the result is a relative path.FileCreateUtils#createPartitionMetaFile(line 398) andcreateMarkerFile(line 448) hand the raw string toPaths.get, so they wrote under the working directory instead of the table, without failing. Lines 384 and 409 of the same class normalise viagetBasePath().toUri().getPath()and were unaffected, which is why this stayed hidden.TestSparkSampleWritesUtilscarried abasePath()override with aTODO remove this and fix parent class (HUDI-6042)comment, working around exactly this.Summary and Changelog
basePath()now returns the unqualified path; a newbaseUri()returns the URI form for callers that want the scheme. This aligns the harness with the three other test harnesses in the repo.SparkClientFunctionalTestHarness:basePath()returnstempDir.toAbsolutePath().toString(); addedbaseUri().URI.create(basePath()).getPath()x18 inTestRollingMetadata,TestSparkRDDWriteClient,TestUpgradeDowngradebasePath().substring(7)inTestHoodieSparkRollbackHUDI-6042override inTestSparkSampleWritesUtilsTestSparkRDDWriteClient:basePath + "_UTC"->"/_UTC".toUri()appends a trailing slash for an existing directory andtoString()does not, so this had become a sibling directory escaping@TempDircleanup.TestHoodieSparkRollback(x2):basePath + ".hoodie/"->"/.hoodie/", same trailing-slash reason.TestHoodieSparkRollback(x3):metaClient.getBasePath().toString().substring(5)->getBasePath().toUri().getPath(). The old form stripped afile:prefix that no longer exists; the new form yields the path component whether or not a scheme is present, matching the idiom already used inFileCreateUtils.TestSparkClientFunctionalTestHarnesscovering the path contract.Impact
Test infrastructure only. No production code is touched. Callers that need the scheme use
baseUri().Risk Level
low
Aligns this harness with
HoodieCommonTestHarness,CLIFunctionalTestHarnessand the examples harness, all of which already return unqualified paths. The change is toward the more permissive form: Hadoop andStoragePathaccept both qualified and unqualified paths, whilejava.nio.file.Pathsaccepts only unqualified.Verification:
TestSparkClientFunctionalTestHarness(new)TestSparkRDDWriteClientTestRollingMetadataTestUpgradeDowngradeTestSparkSampleWritesUtilsTestHoodieSparkCopyOnWriteTableRollbackTableVersionSixTestHoodieSparkMergeOnReadTableRollbackNegative control: reverting only the harness body while keeping the new tests fails 3 of the 4, including the end-to-end case asserting partition metadata lands under the table directory. The fourth (storage resolution) passes either way and is a regression guard rather than a bug detector.
The two rollback classes were also run against an all-master baseline (source and installed artifacts both at master) and pass 30/30 on both sides, confirming no regression.
Since the compiler cannot catch a change of
Stringcontent, all 77 direct and transitive subclasses of the harness were swept for every syntactic way a caller could depend on the scheme:substringon a base path,replace/startsWith/contains("file:"),split/indexOfon a colon,URI/URLconstruction,length()arithmetic, regex, and concatenation without a separator. None remain.TestGcsEventsHoodieIncrSourceoverridesbasePath()to return the qualified form (pre-existing, from HUDI-4850) and is therefore unaffected.That sweep cannot see indirect dependencies, where no
"file:"literal appears and abasePath()-derived path is instead compared against one the filesystem has qualified. CI found exactly one such case,TestHoodieSparkMergeOnReadTableCompaction#validateFileListingInMetadataTable, now fixed by building its partition paths frombaseUri(). The remainingnew StoragePath(basePath(), ...)sites in that class feedexists(),listFiles()anddeleteFile(), which do not compare path strings.Documentation Update
none
Contributor's checklist