jordepic commented on code in PR #5361:
URL: https://github.com/apache/datafusion-comet/pull/5361#discussion_r3915786761
##########
spark/src/main/scala/org/apache/comet/serde/operator/CometIcebergNativeWrite.scala:
##########
@@ -172,21 +169,23 @@ object CometIcebergNativeWrite extends
CometOperatorSerde[IcebergWriteExec] with
PropertyKeys.WriteLocationProviderImpl,
"custom location provider unsupported"),
requireFormatVersionAtMostTwo,
+ requireNoUuidColumns,
requireNoEncryptionPrefix,
- requireSupportedMetricsModes,
requireNoBloomFilterColumnsEnabled,
requireRowGroupCheckMinRecordCountAtDefault,
requireRowGroupCheckMaxRecordCountAtDefault,
requireParquetPageVersionDefault,
requireShredVariantsDisabled,
+ requireParseableCompressionLevel,
requireOnlyVettedParquetWriteProperties,
requirePropertyAbsent(
PropertyKeys.ParquetEnableDictionary,
"dictionary override unsupported"),
requireNoUnvettedParquetMrProperties,
requirePropertyAbsent(PropertyKeys.FileIOImpl, "custom FileIO
unsupported"),
Review Comment:
Right — `getFileIOProperties` covers vended/table FileIO properties, but
`HadoopFileIO` carries its S3A configuration in the Hadoop Configuration, so
the JVM writer would honour settings the native writer never received (the
custom-endpoint case is the genuinely scary one). Done exactly as you
suggested: the write proto assembly now extracts the object-store options for
the data location from the session Hadoop configuration
(`NativeConfig.extractObjectStoreOptions`), translates them with the scan
side's `hadoopToIcebergS3Properties`, and merges them UNDER the FileIO
properties (`hadoopDerived ++ fileIO`), so table/vended FileIO properties take
precedence as you asked. A new detection test sets `fs.s3a.endpoint` /
`fs.s3a.access.key` / `fs.s3a.path.style.access` on the session, converts an
s3a-location insert, and asserts the built proto's `catalog_properties` carry
the translated `s3.*` keys.
##########
native/core/src/execution/operators/iceberg_common.rs:
##########
@@ -0,0 +1,142 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Helpers shared between the Iceberg scan and Iceberg write operators.
+
+use std::collections::HashMap;
+use std::sync::Arc;
+
+use datafusion::common::DataFusionError;
+use iceberg::io::{FileIO, FileIOBuilder, StorageFactory};
+use iceberg_storage_opendal::{CustomAwsCredentialLoader,
OpenDalStorageFactory};
+
+use crate::cloud::s3::credential_bridge::{AccessMode, CometS3CredentialBridge};
+
+/// Activation key for the `CometS3CredentialProvider` SPI, read from a
catalog's `s3.*` property
+/// bag.
+const ICEBERG_PROVIDER_CLASS_PROPERTY: &str =
"s3.comet.credential.provider.class";
+
+/// Key prefixes forwarded to iceberg-rust's `FileIO`. The full unfiltered
catalog bag (catalog
+/// URI, OAuth tokens, credentials.uri, tenant-id, etc.) is kept upstream so
+/// `CometS3CredentialBridge` can read whatever the vendor needs.
+const STORAGE_PROPERTY_PREFIXES: &[&str] = &["s3.", "gcs.", "adls.",
"client."];
Review Comment:
You are right — I over-rotated. The factory is shared, and the scan side
does admit `oss://` (HadoopFileIO passes the scan FileIO check and
`CometScanRule` lists oss as a supported scheme), so dropping the arm regressed
an existing read capability. Restored exactly as you propose:
`OpenDalStorageFactory::Oss` for `AccessMode::Read`, an explicit error for
`AccessMode::Write` (a native-side backstop behind the JVM write gate, which
already declines oss locations). Added rust unit tests on `storage_factory_for`
pinning oss read-ok / write-err, the common schemes in both modes, and
unknown-scheme rejection.
--
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]
