dwsmith1983 commented on code in PR #6065:
URL: https://github.com/apache/datafusion-comet/pull/6065#discussion_r4074048719
##########
spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala:
##########
@@ -1192,20 +1192,22 @@ object CometScanRule extends Logging {
org.apache.spark.sql.catalyst.trees.TreeNodeTag[Unit]("comet.skipCometScan")
/**
- * Schemes Comet's native Iceberg scan can actually open, mirroring the
match arms in
- *
`native/core/src/execution/operators/iceberg_common.rs::storage_factory_for`.
Deliberately
- * NOT delegated to `isNativelyReadableScheme`: object_store recognizes
schemes (http/https,
- * azure, memory) that iceberg-rust's OpenDAL storage factory cannot build,
and admitting them
- * here turns a clean JVM fallback into a native runtime "Unsupported
storage scheme" error. Add
- * here what you add to `storage_factory_for` (currently Aliyun `oss` and
GCS `gs`).
- * S3-compliant aliases like `blob` are opt-in via
`fs.comet.s3Compliant.schemes` (see
- * `isIcebergReadableScheme`), not hardcoded, since the native planner opens
them via S3. The
- * write path keeps its own list
(`CometIcebergNativeWrite.SupportedStorageSchemes`), which
- * differs deliberately: it excludes `oss` (fails closed, see
`storage_factory_for`) and
- * includes `memory`.
+ * Schemes Comet's native Iceberg scan can open, loaded from the native
storage factory over JNI
+ * so this gate cannot drift from `storage_factory_for`. Lazy so that
constructing the rule does
+ * not touch the native library before `isCometLoaded` has been consulted.
Opt-in aliases from
+ * `fs.comet.s3Compliant.schemes` are additive (see
`isIcebergReadableScheme`); the write path
+ * loads its own set (`CometIcebergNativeWrite.SupportedStorageSchemes`).
*/
- private val icebergReadableSchemes: Set[String] =
- Set("file", "s3", "s3a", "gs", "oss")
+ private lazy val icebergReadableSchemes: Set[String] =
IcebergStorageSchemes.read
+
+ /**
+ * True when the Iceberg scan gate admits `scheme`. The built-in set matches
verbatim because
+ * native opens a location by its raw scheme and OpenDAL strips that prefix
case-sensitively, so
+ * `S3://` is not `s3://`; the opt-in alias list is matched
case-insensitively on both sides.
+ */
+ private def isAdmittedIcebergScheme(scheme: String, s3CompliantSchemes:
Set[String]): Boolean =
+ icebergReadableSchemes.contains(scheme) ||
+ s3CompliantSchemes.contains(scheme.toLowerCase(Locale.ROOT))
Review Comment:
> Does that match what you see? If so, should aliases be matched verbatim as
well, with a test for a mixed-case alias location?
It matches. The S3 arm builds `{scheme}://{bucket}/` from `Url::parse`,
whose scheme is lowercase, and calls `starts_with` on the raw path, so
`BLOB://bucket/key` with `blob` opted in passed the gate and the native check
and then failed at open. There is no reason for aliases to stay
case-insensitive on this path; the Parquet path keeps it because it rewrites
alias locations to `s3://` before anything opens them.
1d3f95d6b matches aliases verbatim on the Iceberg path. The gate looks up
the scheme as written against the lowercased list, and native admits an alias
only when the location's scheme has no uppercase, on the scan, write and
region-default paths. One consequence: a hostless mixed-case location such as
`BLOB:///bucket/key`, which the host promotion used to rewrite and open, is
declined too, so aliases follow the same rule as the built-ins everywhere.
Tests cover the mixed-case location on both sides, and the data sources guide
says the location must spell the alias in lowercase.
--
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]
