0lai0 commented on code in PR #6502:
URL: https://github.com/apache/datafusion-comet/pull/6502#discussion_r4169721890
##########
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:
Thanks @andygrove, both addressed.
- `storageScheme` no longer lowercases, so it matches `scheme_of` exactly
and `S3://` now falls back. I dropped the notes about the difference and added
an `S3://` case to both the Scala and Rust tables.
- The `data location URI scheme` row in `iceberg-writes.md` now covers case
sensitivity and the bucket requirement for `s3`, `s3a` and `gs`.
--
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]
