sunchao commented on code in PR #6838:
URL: https://github.com/apache/datafusion-comet/pull/6838#discussion_r4266138898
##########
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:
[P2] Preserve fallback when the newly admitted filesystem needs reader-only
Hadoop options. With native scanning enabled, no startup scheme override, and
reader options `fs.comet.libhdfs.schemes=reviewfake,hdfs`,
`fs.reviewfake.impl=<RawLocalFileSystem subclass>`, and
`fs.reviewfake.impl.disable.cache=true`, Spark reads the Parquet file
successfully. The base gate declines this scheme, but this change admits it.
`extractObjectStoreOptions` forwards only the scheme list for this URI, while
`create_hdfs_operator` supplies only `name_node` and libhdfs constructs a fresh
`Configuration`, so execution fails with `UnsupportedFileSystemException: No
FileSystem for scheme "reviewfake"`. The Delta change shares this problem.
Please retain fallback for configurations the backend cannot reproduce, or pass
the necessary scan Hadoop options to libhdfs before claiming these scans.
Evidence: On Spark 4.1.3, the disposable reproduction read all ten Parquet
rows using the reader-only options, reported `PR base admits scheme: false; PR
head admits scheme: true`, and serialized only `fs.comet.libhdfs.schemes`. A
JNI harness compiled from the pinned hdfs-sys 0.3.1 libhdfs sources called
hdfsNewBuilder → hdfsBuilderSetNameNode → hdfsBuilderConnect, matching Comet's
constructor path. It failed with `No FileSystem for scheme "reviewfake"`.
Supplying fs.reviewfake.impl and its cache setting through
hdfsBuilderConfSetStr made the same constructor succeed. Reproduce with `python
/tmp/comet6838-review-1791944090-mzpkzdf3/run-backend-probe.py`. Source and
output are preserved beside it in BackendProbe.scala, backend_probe.c, and
backend-probe.log. This validates Spark reading and the native constructor
separately, not a complete Comet query.
--
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]
