dwsmith1983 commented on code in PR #6838:
URL: https://github.com/apache/datafusion-comet/pull/6838#discussion_r4266944934
##########
spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala:
##########
@@ -252,26 +252,13 @@ case class CometScanRule(session: SparkSession)
// itself (`NativeBase.isObjectStoreSchemeSupported`) rather than a
hardcoded list, so the
// planner can't drift from object_store's actual support.
//
- // EXCEPT schemes the user routes through libhdfs via
`spark.hadoop.fs.comet.libhdfs.schemes`
- // (e.g. `hdfs`, or a test `fake`): those ARE natively readable through
the libhdfs object_store
- // bridge, so they must NOT be declined here. The claim decision is
guarded in CI by
- // CometScanSchemeFallbackSuite; end-to-end execution through libhdfs is
guarded by
- // ParquetReadFromFakeHadoopFsSuite, which is a manual suite (see its
scaladoc).
- //
- // The default mirrors the native side: when the config is unset,
`is_hdfs_scheme`
- // (native/core/src/parquet/parquet_support.rs) treats `hdfs` as natively
readable, and
- // `create_hdfs_object_store` is in the default build (`default =
["hdfs-opendal"]`). If we
- // defaulted to an empty set here, a plain `hdfs://` V1 scan would be
declined and fall back to
- // Spark even though native can read it -- a silent regression for HDFS
users in the default
- // configuration. So default to `Set("hdfs")` to stay in lockstep with the
native default.
- val libhdfsSchemes: Set[String] = COMET_LIBHDFS_SCHEMES.get() match {
- case Some(s) => NativeConfig.parseSchemeSet(s)
- case None => Set("hdfs")
- }
+ // EXCEPT schemes native routes through libhdfs. The list comes from the
scan's Hadoop conf, as
+ // native reads it, and defaults to `hdfs`.
+ val libhdfsSchemes = NativeConfig.resolveLibhdfsSchemes(hadoopConf)
Review Comment:
> Please retain fallback for configurations the backend cannot reproduce, or
pass the necessary scan Hadoop options to libhdfs before claiming these scans.
Kept the fallback. Both gates now get the libhdfs list from one helper,
`NativeConfig.resolveScanLibhdfsSchemes`, which compares the session's Hadoop
conf with the scan's. A scheme counts as libhdfs only when the session lists it
too, so a reader option alone cannot add one. A scheme listed only through
reader options always falls back, even one object_store could read, because
native `is_hdfs_scheme` would still send it to libhdfs from the forwarded list.
A reader option that drops a session-listed scheme makes it non-libhdfs, as it
is natively, so that scheme goes through the object_store check.
libhdfs builds its configuration from the executor classpath, so the
settings of a libhdfs scheme such as `fs.<scheme>.impl` have to be in the
executor's `core-site.xml`. The config doc now says that. The same holds for a
list set at startup: `spark.hadoop.fs.<scheme>.impl` in the Spark conf does not
reach libhdfs either, and main already claimed that case. Passing the scan's
Hadoop options through to libhdfs is #6967.
The table tests in `CometScanSchemeFallbackSuite` and
`DeltaScanContribSuite` now cover your reader-option case, a reader option that
removes a session-listed scheme, and a reader-only `file` list on a `file:`
read of the table.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
