fix(spark): report read.paths, a missing path and a glob path separately - #19569
fix(spark): report read.paths, a missing path and a glob path separately#19569deepakpanda93 wants to merge 2 commits into
Conversation
Removing glob support in 1.2.0 left one condition covering three different
mistakes, so the message rarely matched what the caller had done.
- read.paths without any wildcard was answered with "Glob paths are not
supported for read paths". A caller passing plain partition paths was told
their non-glob paths were unsupported globs, which is neither true nor
something they can act on. read.paths now reports itself.
- The missing-path check offered read.paths as the alternative, an option that
throws a few lines further down. It now asks for 'path' alone.
- A glob in 'path' keeps its own message.
Each message names the supported replacement: load the table base path and
filter on the partition columns, which prunes partitions before files are
listed. The checks run before the storage handle is built, so a call that
cannot succeed no longer opens one first.
READ_PATHS gains deprecatedAfter("1.2.0"), and its documentation says the
option now fails the read rather than describing it as usable.
Behaviour is unchanged: read.paths and glob paths are still rejected, an empty
read.paths value is still rejected, and only the wording differs.
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR splits a single conflated guard in DefaultSource's 3-arg createRelation into three independent rejections, each with its own accurate message, and marks READ_PATHS deprecated. I traced the logic and the success/failure condition is preserved exactly (original succeeded iff path set, non-glob, and read.paths unset — same as the new guards), so this reads as a behavior-preserving refactor with good test coverage. One interaction with internal callers is worth double-checking in the inline comment. Please take a look at any inline comments, and this should be ready for a Hudi committer or PMC member to take it from here. One minor nit in the Javadoc; code is otherwise clean and readable.
| // an option that no longer exists. 1.2.0 already rejected "" here, since Some("") made the | ||
| // old readPaths.nonEmpty check true, so this keeps the rejection and only reworks the wording. | ||
| throw new HoodieException( | ||
| s"'${READ_PATHS.key()}' is no longer supported as of Hudi 1.2.0. ${DefaultSource.LOAD_BASE_PATH_INSTEAD}") |
There was a problem hiding this comment.
🤖 Heads up: Hudi's own IncrementalRelationV1/V2.buildScan (used by HoodieStreamSourceV1/V2) still sets READ_PATHS internally for meta-bootstrap files and re-enters here via format("hudi_v1").load() with no path, so a streaming read of a metadata-bootstrapped table hits this throw and now surfaces "'…read.paths' is no longer supported… load the base path and filter" — advice the user can't act on since Hudi set the option, not them. It's pre-existing (the old readPaths.nonEmpty glob check threw here too), so no regression, but since this PR cements read.paths as removed and the doc now says "setting it now fails the read", have you verified this streaming/bootstrap path is actually dead — or should those internal callers be migrated off READ_PATHS?
There was a problem hiding this comment.
Verified, and you are right about the streaming half — I had traced this to the incremental query
path only, and missed that streaming reaches the same relations. HoodieStreamSourceV1:186 and
HoodieStreamSourceV2:162 construct IncrementalRelationV1 / IncrementalRelationV2 directly, so a
streaming read reaches it too.
It is reachable, not dead. The bootstrap branch is entered when a commit in the scanned range is
the metadata bootstrap instant (IncrementalRelationV1:179-181):
if (HoodieTimeline.METADATA_BOOTSTRAP_INSTANT_TS == commit.requestedTime) {
metaBootstrapFileIdToFullPath ++= metadata.getFileIdAndFullPaths(basePath)...
}
...
if (metaBootstrapFileIdToFullPath.nonEmpty) {
df = sqlContext.sparkSession.read.format("hudi_v1").schema(prunedSchema)
.option(DataSourceReadOptions.READ_PATHS.key, filteredMetaBootstrapFullPaths.mkString(","))
...
.load()
}METADATA_BOOTSTRAP_INSTANT_TS is "00000000000001", so this fires when an incremental or streaming
read spans the bootstrap instant of a metadata-bootstrapped table. Narrow, but a real scenario rather
than unreachable code. DefaultSource.shortName() is "hudi_v1" and .schema(...) routes through
SchemaRelationProvider, so it lands in the method this PR touches, with READ_PATHS set and no
path.
Not a regression, and not fixed here. It threw before this PR (readPaths.nonEmpty in the old
combined guard) and it throws after; only the wording moves from the glob message to the read.paths
message. BaseFileOnlyRelation:162 reads READ_PATHS too and has been equally unreachable since
1.2.0, for the same reason.
Your sharpest point is the one I had not put into words: on that path Hudi set the option, not the
user, so "load the base path and filter on the partition columns" is advice the caller cannot act on.
That was equally true of the old glob message, so no user is worse off, but it does mean this PR
documents the option as fatal while Hudi still sets it internally.
Migrating those callers is the right fix and it is more than a message change: they need rerouting to
the file group reader, which is the work tracked by #17327 / HUDI-8747 for removing read paths
properly. Keeping it out of this PR so that this one stays a behaviour-preserving message and docs
change, and filing it separately with the trace above.
| private val log = LoggerFactory.getLogger(classOf[DefaultSource]) | ||
|
|
||
| /** | ||
| * What to do instead, appended to every rejection in the 3-arg createRelation so the user is |
There was a problem hiding this comment.
🤖 nit: "the 3-arg createRelation" is a bit opaque here — future readers have to know the overload structure to parse it. Something like "appended to every rejection in createRelation(sqlContext, parameters)" (or just "appended to every user-facing rejection") would be clearer without the internal counting.
There was a problem hiding this comment.
Fair, reworded — though not to the suggested text, because that names the wrong overload.
createRelation(sqlContext, parameters) is the 2-arg one. The constant is used in the 3-arg
createRelation(sqlContext, optParams, schema), so I have used that signature:
/**
* What to do instead, appended to every rejection in
* `createRelation(sqlContext, optParams, schema)` so the user is never told only what does not
* work. ...
*/Same intent as your suggestion, no overload counting, and it points at the method that actually
throws.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19569 +/- ##
=========================================
Coverage 76.93% 76.93%
+ Complexity 32423 32422 -1
=========================================
Files 2522 2522
Lines 139106 139111 +5
Branches 16713 16715 +2
=========================================
+ Hits 107020 107025 +5
- Misses 24510 24511 +1
+ Partials 7576 7575 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
"the 3-arg createRelation" asks the reader to know the overload structure. Name the signature instead.
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR splits a conflated rejection guard in DefaultSource.createRelation into three independent checks so each failure names what actually happened, and updates the READ_PATHS config doc to match. I traced the refactor against the old behavior and it preserves the same rejections, including the empty-value case, while building storage only after the path check, and confirmed no code depends on the old message strings. The one substantive concern, internal READ_PATHS usage in IncrementalRelationV1/V2 for meta-bootstrap flowing into this throw, was already flagged in the prior round and is pre-existing, so this PR does not regress it. No issues flagged from this automated pass, a Hudi committer or PMC member can take it from here for a final review.
cc @yihua
4 participants
Describe the issue this Pull Request addresses
Closes #15174.
#14060 removed glob path support and deprecated
hoodie.datasource.read.pathsin 1.2.0, replacingthe globbing logic with an unconditional throw. The three rejections in
DefaultSource's 3-argcreateRelationended up sharing one condition and one message, so the message rarely describeswhat the caller actually did.
A non-glob value is reported as an unsupported glob. The guard is:
readPaths.nonEmptyhas nothing to do with wildcards, so a caller passing plain partition paths:is told their non-glob paths are unsupported glob paths. There is no wildcard anywhere in that call
and nothing in the message to act on.
The missing-path message advertises an option that throws. The guard above it says:
A caller who follows that and sets
hoodie.datasource.read.pathsreaches the throw quoted above.The config documentation still describes the option as usable.
READ_PATHScarries@Deprecatedbut nodeprecatedAfter(...), and reads "Comma separated list of file paths to readwithin a Hudi table", with nothing to say that setting it now fails the read.
Summary and Changelog
The rejections are unchanged. What changes is that each one now says what happened and what to do
instead.
read.pathsis set, nopathwas given,pathcontains a glob. Each throws its own message.the table base path and filter on the partition columns. Those predicates are pushed down and
prune partitions before any file is listed, via
HoodieFileIndex#prunePartitionsAndGetFileSlices,which is what selecting paths by hand was for.
read.pathsas the alternative, since setting it throws.HoodieStorageUtils.getStorage(...), so a call that cannot succeed nolonger opens a storage handle first. The now-dead
readPathsandallPathslocals are gone, andallPaths.headbecomespath.get, which is safe becausepath.isEmptyis rejected above it.READ_PATHSgainsdeprecatedAfter("1.2.0"), and its documentation states that the option nowfails the read and what to use instead.
Two cases that needed a deliberate choice rather than falling out of the code:
When both
read.pathsis set andpathcontains a glob,read.pathsis reported. It is themore fundamental of the two: the option is gone outright, and the replacement it points at resolves
the glob case as well. Pinned by
testReadPathsWinsOverGlobInPath.An explicitly empty
read.paths=""is still rejected.optParams.getyieldsSome(""), so thekey counts as set. 1.2.0 already rejected it, since
Some("")made the oldreadPaths.nonEmptycheck true, and treating it as unset here would turn a throw into a successful read. Pinned by
testEmptyReadPathsIsStillRejected.Impact
None on behaviour.
read.pathsand glob paths are rejected before this change and after it, anempty
read.pathsis rejected in both, and every read that worked before still works. Only thewording of the three exceptions and the
READ_PATHSdocumentation change.Users who hit these errors now get a message that matches their input and names the replacement.
Risk Level
low
Confined to three error paths in one method plus a config doc string. No change to relation
construction, planning or reads. Covered by a new suite of 13 tests, and reverting the change fails
exactly the 7 of them that assert the messages while the 6 no-regression tests keep passing.
Documentation Update
The
hoodie.datasource.read.pathsconfig description is updated in this PR to say the option nowfails the read and to point at loading the base path with partition predicates instead. It also
gains
deprecatedAfter("1.2.0"), so it is rendered as deprecated since that release. No Hudi websitechange is needed.
Contributor's checklist