0lai0 opened a new pull request, #6502: URL: https://github.com/apache/datafusion-comet/pull/6502
## Which issue does this PR close? Closes #6140. ## Rationale for this change The native Iceberg write gate and the native storage factory disagree on the scheme of a data location. `CometIcebergNativeWrite.storageScheme` takes the text before `://` and treats a location without `://` as `file`. The native `scheme_of` in `iceberg_common.rs` takes the text before the first `:`. Hadoop normalises `hdfs:///warehouse/t` to `hdfs:/warehouse/t`. For that location the gate sees `file` and admits the write. The native writer sees `hdfs`, which has no storage backend, so every task fails with `Unsupported storage scheme: hdfs` instead of the write falling back to iceberg-java. The same happens for any `scheme:/path` form. Aligning the scheme rule exposes a second case the gate admits but native cannot open. iceberg-rust's S3 and GCS backends take the bucket from the URL host and never from the path (`s3_config_build` and `gcs_config_build` call `url.host_str()` and fail with a missing-bucket error). So a hostless `s3:/bucket/key`, and also `s3:///bucket/key`, which main already reads as `s3` and admits, fail natively. I read this in iceberg-rust at `665c64e`, which is the newest checkout I had locally. Comet pins `bb1e4a48`, which I did not have, so this part is from reading the code rather than from running a native write against S3. ## What changes are included in this PR? - `storageScheme` now follows the `scheme_of` rule. It splits on the first `:`, and an empty prefix or one containing `/` means there is no scheme. It stays string-based rather than using `java.net.URI`, because `URI` throws on characters an Iceberg location may carry unencoded and its scheme grammar is not the first-`:` split. `hdfs:/...` is now declined with `unsupported storage scheme: hdfs`. - A new `hasBucketAuthority` check declines a supported location with no host, unless its scheme is local (`file` or `memory`), with the reason `<scheme> data location has no bucket in its authority: <location>`. Today that covers `s3`, `s3a` and `gs`. This is a behaviour change for `s3:///bucket/key`, which main admitted and which then failed natively. It is the write-side counterpart of `CometScanRule.hasOpenableAuthority`, without the alias exception, since the write gate admits no S3-compliant aliases. - The check names the two local schemes rather than the bucket-bearing ones, and `SupportedStorageSchemes` is left untouched. That way it stays correct when the supported list comes from the native factory, as #6065 does, and a new bucket-bearing backend gets the host check without a JVM edit. - Comments on `storageScheme` and `scheme_of` point at each other so the two rules change together. One difference is left on purpose. The gate still lowercases the scheme and `scheme_of` does not, so `S3://bucket/key` is admitted here and rejected natively. #6065 settles case handling by matching verbatim on both sides, so this PR does not touch it. The two PRs touch the same lines of `storageScheme`. Whichever lands second should keep this PR's first-`:` split and #6065's verbatim matching, and drop the comment here that describes the lowercase difference. A side effect worth noting: `CometIcebergNativeWrite` says an S3-compliant alias scheme never reaches the write serde. On main that was not quite true, because a hostless `blob:/bucket/key` was read as `file` and admitted. It is now read as `blob` and declined. ## How are these changes tested? New tests in `CometIcebergWriteDetectionSuite`: - `fall-back: hostless hdfs:/ data location is read as hdfs, not file` creates a table with `write.data.path` set to `hdfs:/...` and asserts the planned write is declined with `unsupported storage scheme: hdfs`. - `fall-back: s3 data location without a bucket in its authority` does the same for `s3:/nonexistent-bucket/...`. - `storageScheme follows the native scheme_of rule` covers `hdfs:/`, `hdfs:///`, `hdfs://nn:8020`, `s3://`, `s3:/`, `blob:/`, `memory:/`, `file:///`, `file:/`, a schemeless path and `/tmp/a:b`. - `hasBucketAuthority requires a non-empty host after //` covers host-bearing and hostless forms. The fall-back tests only plan the write and do not execute it, since there is no HDFS or S3 in the test environment. `scheme_of_extracts_scheme_from_all_uri_forms` in `iceberg_common.rs` gains the same `hdfs`, `s3:/`, `memory:/` and `file:/` cases, so both sides pin the rule they must agree on. Run locally with the default profile (Spark 4.1, Scala 2.13): - `./mvnw test -Dtest=none -Dsuites="org.apache.comet.CometIcebergWriteDetectionSuite"`: 56 succeeded, 0 failed. - `cargo test -p datafusion-comet --lib iceberg_common`: 4 passed. - `make format PROFILES="-Pspark-4.0"`: clean, no changes. The default spark-4.1 profile uses Scala 2.13.17, for which `semanticdb-scalac` 4.13.6 is not published, so scalafix runs under spark-4.0 as CI does. This touches the Iceberg write path, so it should get a `run-iceberg-tests` run before it is queued. -- 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]
