mixermt opened a new pull request, #5898: URL: https://github.com/apache/datafusion-comet/pull/5898
> [!IMPORTANT] > **NOT FOR MERGE — pending apache/iceberg-rust#3111.** > > This PR pins `iceberg` / `iceberg-storage-opendal` to a **personal fork** > (`mixermt/iceberg-rust@9d7d2d89`) because the HDFS backend it depends on has not merged > upstream yet. That pin must not be merged into Comet. It is opened now for early review of > the Comet-side design; once apache/iceberg-rust#3111 lands and Comet's pinned rev includes > it, the dependency change drops out and the diff becomes feature-only. > > Upstream status: apache/iceberg-rust#3111 is open with changes requested. ## Which issue does this PR close? Part of #5894. ## Rationale for this change Comet's native Iceberg scan cannot read a table whose data lives on HDFS: `hdfs://` is absent from the scheme allowlist that mirrors `storage_factory_for`, so every Iceberg table on HDFS falls back to the JVM reader. That is a gap relative to the plain-Parquet native scan, which does read HDFS through libhdfs/JNI (`fs.comet.libhdfs.schemes`). On-premise Iceberg deployments are commonly HDFS-backed and get no native Iceberg acceleration today. iceberg-rust is gaining a pure-Rust HDFS backend (`hdfs-native`, no JNI or libhdfs) in apache/iceberg-rust#3111. This PR is the Comet side of that. Worth stating plainly, because it surprises: this introduces a **second, independent HDFS client** into the same process. The plain-Parquet path reaches HDFS through libhdfs/JNI; an Iceberg table is opened over pure-Rust RPC. Both link into `libcomet`, they read the same `$HADOOP_CONF_DIR` XML, but they hold separate connections and separate Kerberos state — the Rust client does not reuse the JVM's Kerberos subject. ## What changes are included in this PR? Native (`iceberg_common.rs`): - `storage_factory_for`: route `hdfs` to `OpenDalStorageFactory::HdfsNative`. - `STORAGE_PROPERTY_PREFIXES`: forward `hdfs.` and `hadoop.` to the native `FileIO`. Without these the NameNode list never reaches iceberg-rust. JVM gates: - `CometScanRule.icebergReadableSchemes` and `CometIcebergNativeWrite.SupportedStorageSchemes` admit `hdfs`, keeping both in lockstep with `storage_factory_for` as their docstrings require. Only `hdfs` itself — a libhdfs alias scheme has no iceberg-rust arm. NameNode resolution (`CometIcebergNativeScan.hadoopToIcebergHdfsProperties`) — the part that is not obvious: - opendal's `HdfsNativeBuilder` never dials the authority written in the path. It builds one client against a synthetic authority and synthesizes the HA config from the comma-separated `hdfs.name-node` value (`init_hdfs_config` in `opendal-service-hdfs-native`). iceberg-rust falls back to the path authority only when that property is absent, which is correct just for a real `host:port`. - An HA location reads `hdfs://<nameservice>/...`, and a nameservice is not a routable host. So without a resolved NameNode list, **every HA table would fail to connect at execution time** — after the planner had already committed to the native scan. - The endpoints are therefore derived from the session Hadoop configuration (`dfs.ha.namenodes.<ns>` plus each `dfs.namenode.rpc-address.<ns>.<nn>`), joined in declaration order, so a standard HDFS client configuration needs no new settings. An explicit catalog `hdfs.name-node` still wins. A partially resolved list yields nothing rather than a short failover list, which would silently turn a failover into an outage. Single-NameNode-per-scan gate: - One `hdfs.name-node` overrides the authority of *every* path the FileIO opens, so a scan whose data/delete files span more than one HDFS authority now falls back. Otherwise the second nameservice would be read from the first one's NameNode at the same relative path — wrong data rather than an error. This mirrors the existing multi-bucket S3 check. ## How are these changes tested? - `CometIcebergHdfsSuite`: end-to-end reads against an in-process `MiniDFSCluster` — plain read, pushed-down filter, and a partitioned table spanning multiple data files. Each asserts a single `CometIcebergNativeScanExec` in the plan and result parity with Spark, and the first asserts the resolved data location really is `hdfs://`, so the suite cannot silently degrade into duplicate local-filesystem coverage. - `CometIcebergNativeScanSuite`: five cases pinning the HA translation — declaration order, non-HA authority yielding nothing, all-or-nothing on a partial list, no double `hdfs://` prefix, and non-hdfs/authority-less inputs ignored. - `CometScanSchemeFallbackSuite`: `hdfs` admitted by the Iceberg gate; hostless `hdfs:///` declined. - Rust unit tests in `iceberg_common`: the `hdfs` arm resolves for read and write, and the `hdfs.`/`hadoop.` prefixes survive the property narrowing. - Full Rust workspace suite (1442 tests) and the affected JVM suites pass locally. ### Known gaps, stated rather than hidden - **Writes are gated and property-forwarded but have no functional test.** Only reads are covered end to end. Reviewers may reasonably prefer `hdfs` be dropped from `SupportedStorageSchemes` until a write test exists. - **The HA path is unit-tested only.** `MiniDFSCluster` is single-NameNode, so `hdfs.name-node` has never resolved a live nameservice; the translation logic is pinned by tests, real failover is not. - **`CometIcebergHdfsSuite` cannot run on the Spark 4.x profiles.** `hadoop-client-minicluster` is pinned at 3.3.4 (`pom.xml`) while Spark supplies `hadoop-client-api`/`runtime` 3.4.2, so `HttpServer2` resolves a shaded Jetty class the older jar lacks and the NameNode web server fails to start. The suite detects this and cancels rather than aborting, so CI stays green, but that means the end-to-end coverage does not execute there. Aligning the minicluster version is a pre-existing fix worth its own PR — `WithHdfsCluster` is unusable on Spark 4.x today regardless of this change. - The dependency pin advances iceberg-rust 29 commits beyond the previous rev in addition to adding HDFS. When this is rebased for merge, that bump belongs in its own `deps:` PR. ## AI Disclosure Developed with AI assistance (Claude Code): drafting the implementation, tests, and this description. I reviewed the changes and ran all verification locally. -- 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]
