andygrove commented on code in PR #6502:
URL: https://github.com/apache/datafusion-comet/pull/6502#discussion_r4155763649
##########
spark/src/main/scala/org/apache/comet/serde/operator/CometIcebergNativeWrite.scala:
##########
@@ -331,20 +334,52 @@ object CometIcebergNativeWrite extends
CometOperatorSerde[IcebergWriteExec] {
.find(k => !IgnoredHadoopParquetConfKeys.contains(k))
.map(k => s"Hadoop configuration sets $k (reaches iceberg-java's writer
but not native)")
- private def storageScheme(location: String): String =
- if (location.contains("://")) {
- location.substring(0, location.indexOf("://")).toLowerCase(Locale.ROOT)
- } else {
- "file"
- }
+ /**
+ * The scheme the native writer picks its storage backend from. Must follow
the same rule as
+ * `scheme_of` in `native/core/src/execution/operators/iceberg_common.rs`:
split on the first
+ * `:`, not `://`, so a hostless `hdfs:/warehouse/t` (as Hadoop normalises
`hdfs:///...`) is
+ * read as `hdfs` rather than admitted as `file`. An empty prefix, or one
containing `/` (a `:`
+ * inside a path segment such as `/tmp/a:b`), means there is no scheme.
+ *
+ * Unlike `scheme_of`, this lowercases the scheme, so `S3://bucket/key` is
admitted here but
+ * rejected natively.
+ *
+ * String-based rather than `java.net.URI` (`NativeConfig.lowerScheme`):
`URI` throws on
+ * characters an Iceberg location may carry unencoded, and its scheme
grammar is not the
+ * first-`:` split that `scheme_of` uses.
+ */
+ private[comet] def storageScheme(location: String): String = {
+ val colon = location.indexOf(':')
+ val prefix = if (colon > 0) location.substring(0, colon) else ""
+ if (prefix.isEmpty || prefix.contains('/')) "file" else
prefix.toLowerCase(Locale.ROOT)
Review Comment:
This still lowercases the scheme and `scheme_of` does not.
`storage_factory_for` matches `file`, `memory`, `gs`, `s3` and `s3a`
case-sensitively, so an `S3://bucket/key` location passes this gate, passes
`hasBucketAuthority`, and then fails every task with `Unsupported storage
scheme: S3`. That is the same gate versus native mismatch as #6140, and this PR
is marked as closing it.
Could we drop the `toLowerCase(Locale.ROOT)` so the gate matches `scheme_of`
exactly? Then `S3://` falls back with `unsupported storage scheme: S3`. It
would also let you remove the sentence in the doc comment above that says the
two differ, and the matching note on `scheme_of` in `iceberg_common.rs`. A
`"S3://bucket/key" -> "S3"` case in `storageScheme follows the native scheme_of
rule` would pin it. `Locale` is still used elsewhere in this file, so the
import stays. #6065 also matches verbatim, so the overlap on these lines should
be easy to resolve.
--
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]
