Repository navigation
fix: read fs.comet.libhdfs.schemes for the scan rule from the Hadoop conf - #6838
dwsmith1983 wants to merge 6 commits into
Conversation
…conf CometScanRule read the libhdfs scheme list from the SQLConf entry spark.hadoop.fs.comet.libhdfs.schemes, while native routes a scheme through libhdfs from the Hadoop conf the scan forwards. Spark copies SQLConf keys into the Hadoop conf with the spark.hadoop. prefix kept, so a list set only in core-site.xml made the rule fall back on a scheme native reads, and a runtime SET of the prefixed key made the rule claim a scan native could not open. The rule now takes the list from the scan's Hadoop conf, as it already does for fs.comet.s3Compliant.schemes, defaulting to hdfs like native.
|
@andygrove this one is a backport candidate for |
sunchao
left a comment
There was a problem hiding this comment.
Reviewed the full five-file diff from bfacc43a50aba13da4fa8a828db52309541191b4 to 82855b53317aeabc8a8535a521e574cb2ea4090f. The PR is not a draft. Read the existing discussion and linked issue. No introduced P1/P2 issues found within this review, and no substantiated existing P1/P2 concerns remain.
Routed skills: .ai/skills/review-comet-pr/SKILL.md. No sibling review skill applies to this configuration-routing change.
Summary
- Prior state and problem:
CometScanRuleread the prefixed SQLConf entry while native serialization read the bare Hadoop key. This could reject configured filesystems or admit scans that subsequently failed to open. - Design approach: Resolve the libhdfs scheme list from the scan's existing Hadoop configuration, matching the configuration source used by serialization.
- Correctness: Traced
newHadoopConfWithOptions,extractObjectStoreOptions, nativeis_hdfs_scheme, and root-path classification. Missing and blank values correctly produce the nativehdfsdefault because serialization omits blank values. Explicit lists retain trimming and case normalization. No P1/P2 correctness issue identified. - Compatibility analysis: Checked Spark sources for 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0. All strip
spark.hadoop.during startup configuration processing, copy SQLConf keys verbatim into session Hadoop configurations, and apply reader options afterward. Compared the affected routing againstbranch-1.1ate9efd9f764ee0a59b7898ff028d6985d4a7a28e1, which has the old SQLConf read and identical native scheme parsing. The routing corrections are intended release-relative changes. - Key design decisions: Keep the registered configuration entry for generated documentation, but use the Hadoop key for routing. Reuse the existing parser and preserve native's default behavior.
- Implementation sketch: Add
resolveLibhdfsSchemes, call it fromCometScanRule, clarify configuration documentation, and extend existing suites with resolver and planner regressions for bare Hadoop keys, runtime prefixed settings, and reader options. - Performance: The change reuses an already-created Hadoop configuration. It adds no filesystem I/O, configuration cloning, or per-row work. No evidence-backed P1/P2 overhead issue identified.
- Design: Aligning planning with execution's configuration source directly addresses both mismatch directions without changing backend construction or adding version-dependent branches.
- Abstraction & complexity: The small resolver fits alongside existing scheme-resolution helpers and removes configuration-policy logic from the scan rule. No unnecessary abstraction or complexity issue identified.
- Behavioral changes worth calling out: Hadoop-only and reader-option lists can now admit native scans. Runtime changes to the prefixed SQLConf key no longer incorrectly control routing or exclude default
hdfs. Startup settings continue to work. The patch changes scan selection, without changing decoding logic. - Suggested improvements: No additional change meeting the P1/P2 reporting bar was identified.
Exact-head CI: Comet CI, CodeQL, and Check PR Title report action_required. Only labeling passed. There is no build or test verdict for this head.
Validation: Compiled the exact NativeConfig.scala in a disposable harness using the checkout's extracted constants and comma parser. All 17 checks passed with Spark 3.5.9, covering startup/runtime configuration, reader-option precedence, Hadoop XML, substitution, blank values, explicit lists, and serialized routing agreement. git diff --check passed.
Validation limits: The bounded native build failed because the installed JDK lacks jni.h. Consequently, the repository ScalaTest suites, Spark SQL suite, and libhdfs end-to-end execution were not run. Other supported Spark versions were checked against source rather than executed. The isolated probe is not a full Comet build or native execution test.
Which issue does this PR close?
Closes #6837.
Rationale for this change
CometScanRulereadfs.comet.libhdfs.schemesfrom the SQLConf entryspark.hadoop.fs.comet.libhdfs.schemes, while native routes a scheme through libhdfs from the Hadoop conf the scan forwards. Spark'sSessionState.newHadoopConfcopies SQLConf keys as they are, without dropping thespark.hadoop.prefix, so the two could disagree. A list set only incore-site.xmlmade the rule fall back on a scheme native could read, and a runtimeSET spark.hadoop.fs.comet.libhdfs.schemesmade it claim a scan native then failed to open ("Unable to recognise URL").What changes are included in this PR?
CometScanRuletakes the libhdfs list from the scan's Hadoop conf (newHadoopConfWithOptions(relation.options), the conf the scan serializes for native) through a newNativeConfig.resolveLibhdfsSchemes, which defaults tohdfswhen the key is unset or blank, as nativeis_hdfs_schemedoes.fs.comet.s3Compliant.schemeswas already read this way.spark.hadoop.fs.comet.libhdfs.schemesdoc says it is read from the Hadoop configuration and must be set before the SparkSession is created, as the s3Compliant entry already says.What users see, only where the two sources disagreed (query results do not change, only native or fallback):
core-site.xml, a reader or table option, or the bare keyfs.comet.libhdfs.schemes) now gets the scan run natively instead of falling back.SET spark.hadoop.fs.comet.libhdfs.schemes=...no longer changes what the planner claims. A scheme it added now falls back instead of failing at execution.SETof that key that leaves outhdfsno longer makeshdfs://scans fall back, since native never saw it. Set the list at startup to change it.The list set in the Spark conf at startup reaches both places and behaves as before.
branch-1.1has the same code, so this is a backport candidate.resolveLibhdfsSchemesand its unit test are the same as in #6753, so whichever of the two lands second resolves only a small conflict.How are these changes tested?
CometScanSchemeFallbackSuite, applyingCometScanRuleto the Spark plan of afake://scan:main, which declines it with "Unsupported filesystem schemes: fake".spark.hadoop.key, the rule declines the scan with that reason. Onmainit claims it.NativeConfigSuitecovers the resolver: unset, blank, and a list with spaces and mixed case. Reverting the rule change fails all three rule tests. CometScanSchemeFallbackSuite and NativeConfigSuite pass, and the build compiles on the Spark 3.4 profile and with strict warnings on 3.5.