hudi-agent commented on code in PR #19569:
URL: https://github.com/apache/hudi/pull/19569#discussion_r3747854830
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/DefaultSource.scala:
##########
@@ -292,6 +305,17 @@ object DefaultSource {
private val log = LoggerFactory.getLogger(classOf[DefaultSource])
+ /**
+ * What to do instead, appended to every rejection in the 3-arg
createRelation so the user is
Review 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.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/DefaultSource.scala:
##########
@@ -94,19 +94,32 @@ class DefaultSource extends RelationProvider
val path = optParams.get("path")
val readPathsStr = optParams.get(DataSourceReadOptions.READ_PATHS.key)
- if (path.isEmpty && readPathsStr.isEmpty) {
- throw new HoodieException(s"'path' or '${READ_PATHS.key()}' or both must
be specified.")
+ // These are three independent problems and each gets its own message.
They used to share one
+ // condition, so a user passing plain partition paths through read.paths
was told that their
+ // non-glob paths were unsupported glob paths, which is neither true nor
actionable.
+ //
+ // Checked before any storage handle is built, so a doomed call does not
open one first.
+ //
+ // read.paths is reported ahead of a glob in 'path' because it is the more
fundamental of the
+ // two: the option is gone entirely, and its replacement resolves the glob
case as well.
+ if (readPathsStr.isDefined) {
+ // Deliberately fires for an explicitly empty value too: setting the key
at all is asking for
+ // 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}")
Review 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`?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
