parthchandra commented on PR #4752: URL: https://github.com/apache/datafusion-comet/pull/4752#issuecomment-5145625663
> This PR's new comments say the Iceberg native reader does its I/O through `object_store`. That's not accurate. The Iceberg scan path goes through `iceberg-storage-opendal` / `opendal` (`native/core/src/execution/operators/iceberg_scan.rs` imports `iceberg_storage_opendal::OpenDalStorageFactory` and `iceberg::io::FileIO`, and `native/Cargo.toml` pulls in `iceberg-storage-opendal` with the `opendal-*` feature flags). `object_store` is the crate Comet's _non-Iceberg_ native Parquet/CSV scan uses (`native/core/src/parquet/`, `native/core/src/execution/operators/csv_scan.rs`) -- a different code path entirely. My own earlier review comment used "object_store" loosely when describing the Iceberg case, and that imprecision carried into the code comments below. Sorry for the bad steer. > > Rather than swap `object_store` for `opendal` in these comments, I'd rather we not name either backend at all: iceberg-rust's storage layer could eventually replace `opendal` to `object_store` upstream, so a comment naming either one will go stale on the next `iceberg` dependency bump. Describe the behavior instead of the backend: Comet's native reader bypasses Iceberg Java's `FileIO` and reads through iceberg-rust's own storage layer. > > Five spots to fix: > > **`spark/src/main/scala/org/apache/comet/iceberg/IcebergReflection.scala:71`** > > ```scala > // Iceberg FileIO implementations whose underlying storage object_store can reach. > ``` > > to something like: > > ```scala > // Iceberg FileIO implementations whose backing storage Comet's native reader can reach. > ``` > > **`spark/src/main/scala/org/apache/comet/iceberg/IcebergReflection.scala:87`** > > ```scala > // and reads the ciphertext via object_store, so any EncryptingFileIO variant is compatible. > ``` > > to something like: > > ```scala > // and reads the ciphertext through iceberg-rust's own storage layer, so any EncryptingFileIO > // variant is compatible. > ``` > > **`spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala:476-478`** > > ```scala > // Comet's native reader uses object_store (Rust) for I/O, bypassing Iceberg Java's > // FileIO entirely. Only allow known-compatible implementations whose underlying > // storage object_store can reach via standard URL schemes. > ``` > > to something like: > > ```scala > // Comet's native reader bypasses Iceberg Java's FileIO entirely and reads through > // iceberg-rust's own storage layer instead. Only allow known-compatible implementations > // whose backing storage that layer can reach via standard URL schemes. > ``` > > **`spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala:484`** > > ```scala > "Comet's native reader (object_store bypasses Iceberg Java FileIO)" > ``` > > to something like: > > ```scala > "Comet's native reader (bypasses Iceberg Java FileIO)" > ``` > > Not asking for a search-and-replace to `opendal` here, just to drop the backend name from these five comments so they describe the behavior rather than an implementation detail that's expected to change. Reworded all five spots. Left the non-Iceberg V1 Parquet/CSV comments -- 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]
