Skip to content

fix: read fs.comet.libhdfs.schemes for the scan rule from the Hadoop conf - #6838

Open
dwsmith1983 wants to merge 6 commits into
apache:mainfrom
dwsmith1983:fix/6837-libhdfs-schemes-hadoop-conf
Open

dwsmith1983 wants to merge 6 commits into
apache:mainfrom
dwsmith1983:fix/6837-libhdfs-schemes-hadoop-conf

Conversation

@dwsmith1983

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes #6837.

Rationale for this change

CometScanRule read fs.comet.libhdfs.schemes 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's SessionState.newHadoopConf copies SQLConf keys as they are, without dropping the spark.hadoop. prefix, so the two could disagree. A list set only in core-site.xml made the rule fall back on a scheme native could read, and a runtime SET spark.hadoop.fs.comet.libhdfs.schemes made it claim a scan native then failed to open ("Unable to recognise URL").

What changes are included in this PR?

  • CometScanRule takes the libhdfs list from the scan's Hadoop conf (newHadoopConfWithOptions(relation.options), the conf the scan serializes for native) through a new NativeConfig.resolveLibhdfsSchemes, which defaults to hdfs when the key is unset or blank, as native is_hdfs_scheme does. fs.comet.s3Compliant.schemes was already read this way.
  • The spark.hadoop.fs.comet.libhdfs.schemes doc 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):

  • A list set only in the Hadoop conf (core-site.xml, a reader or table option, or the bare key fs.comet.libhdfs.schemes) now gets the scan run natively instead of falling back.
  • A runtime 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.
  • A runtime SET of that key that leaves out hdfs no longer makes hdfs:// 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.1 has the same code, so this is a backport candidate.

resolveLibhdfsSchemes and 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, applying CometScanRule to the Spark plan of a fake:// scan:

  • With the list as a reader option, and as the bare Hadoop key, the rule claims the scan. Both fail on main, which declines it with "Unsupported filesystem schemes: fake".
  • With the list set at runtime under the spark.hadoop. key, the rule declines the scan with that reason. On main it claims it.

NativeConfigSuite covers 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.

…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.
@github-actions github-actions Bot added bug Something isn't working area:scan Parquet scan / data reading labels Oct 10, 2026
@dwsmith1983

Copy link
Copy Markdown
Contributor Author

@andygrove this one is a backport candidate for branch-1.1, which has the same SQLConf read in CometScanRule. branch-1.0 differs: its native side compares the scheme list without trimming or case folding, so a 1.0 backport would need its own resolver.

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: CometScanRule read 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, native is_hdfs_scheme, and root-path classification. Missing and blank values correctly produce the native hdfs default 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 against branch-1.1 at e9efd9f764ee0a59b7898ff028d6985d4a7a28e1, 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 from CometScanRule, 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:scan Parquet scan / data reading bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CometScanRule reads fs.comet.libhdfs.schemes from SQLConf, but native reads it from the Hadoop conf

2 participants