You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "mbutrovich (via GitHub)" <gi...@apache.org> on 2026/07/21 14:43:05 UTC
[PR] feat: [DO NOT MERGE] preliminary Iceberg V3 support: native table decryption, fall back for other V3 features [datafusion-comet]
mbutrovich opened a new pull request, #4991:
URL: https://github.com/apache/datafusion-comet/pull/4991
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Part of #3756 (Iceberg feature-matrix epic). Closes #.
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
Comet's native Iceberg scan supported only table format V1 and V2 and rejected V3 tables outright. This adds native reads of **encrypted** Iceberg tables, which is a V3 feature, making it the first V3 read feature Comet supports natively.
The key enabler is Iceberg's encryption model. Iceberg-Java performs all KMS work (unwrapping the KEK and manifest-list keys) on the driver during scan planning, and stores each data file's data encryption key (DEK) in **plaintext** inside the manifest's `key_metadata` field (the manifest itself is encrypted at rest). By the time Comet extracts `FileScanTask`s, `DataFile.keyMetadata()` already holds the plaintext DEK, so Comet can forward those bytes verbatim to iceberg-rust, which decodes the same `StandardKeyMetadata` format and hands the key straight to Parquet decryption. No key management service and no JNI callback are required on the native side.
This is distinct from Parquet Modular Encryption for plain Parquet tables (#2447), where the reader must call a KMS to unwrap per-file keys at read time. Iceberg's planning phase makes that unnecessary here.
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
Native dependency:
- Bump iceberg-rust to `3f163f25` to pick up the merged encryption read path (apache/iceberg-rust#2584 for encrypted Parquet data files and the `FileScanTask::key_metadata` field, plus the manifest and manifest-list read support in #2453, #2586, #2402). iceberg-rust's `build_arrow_reader_options` decodes `StandardKeyMetadata` and uses `encryption_key()` directly as the Parquet decryption key, with no KMS unwrap.
Key-metadata passthrough:
- proto (`operator.proto`): add `optional bytes key_metadata` to `IcebergFileScanTask` (field 6) and `IcebergDeleteFile` (field 8; fields 5 to 7 are left reserved for the deletion-vector coordinates used on a separate in-flight branch, so the two can merge in either order).
- serde (`CometIcebergNativeScan`): reflectively read `ContentFile.keyMetadata()` for data files and delete files and forward the bytes. Null (unencrypted) leaves the field unset.
- planner (`planner.rs`): populate `FileScanTask::key_metadata` and `FileScanTaskDeleteFile::key_metadata` from the proto.
Format-version gate and fallbacks (`CometScanRule`, `IcebergReflection`):
- Allow Iceberg format version 3 (still rejecting V4+).
- Fall back for V3 column default values, which iceberg-rust does not synthesize for columns absent from a data file. This check runs only for V3 tables, since V1/V2 cannot carry default values.
- Fall back for column types the native reader does not support (for example variant), applying Comet's existing `DataTypeSupport` allow-list to the full table schema Comet serializes, not just the projected columns.
- Table encryption is **not** a fallback reason. Encrypted tables now flow to the native scan.
- The delete-file gate stays Parquet-only, so V3 tables with deletion vectors (Puffin delete files) continue to fall back to Spark, since native deletion-vector reads are not part of this PR.
Test infrastructure:
- `CometTestKMS` (spark-4.1 source set): an in-memory `KeyManagementClient` for tests. It lives in the spark-4.1 source set because Iceberg's `KeyManagementClient` interface is public only from Iceberg 1.10, and table encryption requires Iceberg 1.11.
- `dev/diffs/iceberg/1.11.0.diff`: tolerate Comet's exception wrapping in `testMissingRequiredWithoutDefault` (Comet surfaces the underlying "Missing required field" error wrapped in `SparkException`/`CometNativeException`).
## How are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
Unit tests that lock down the design assumptions and act as regression guards:
- Rust `standard_key_metadata_roundtrips` (native/core/src/execution/operators/iceberg_scan.rs): confirms `iceberg::encryption::StandardKeyMetadata` is available after the rev bump and that its encode/decode round-trips. API-stability guard.
- Rust `decodes_java_produced_key_metadata`: decodes a real Iceberg-Java `StandardKeyMetadata` blob (captured from the Scala suite below) and asserts iceberg-rust recovers the identical 16-byte DEK and a 16-byte AAD prefix. This is the cross-language wire-format gate that proves Java's bytes decode in Rust without any KMS unwrap.
- Scala `CometIcebergEncryptionSuite` (spark-4.1 source set): drives Iceberg's `StandardEncryptionManager` directly and asserts that a data file's `key_metadata` carries a plaintext DEK (not a wrapped one) and that the serialized blob round-trips. Regression guard for the plaintext-passthrough assumption. No catalog is needed for this test.
End-to-end coverage:
- Apache Iceberg's own `TestTableEncryption` suite runs under Comet in the iceberg-spark CI via `dev/diffs/iceberg/1.11.0.diff`, which enables `spark.comet.scan.icebergNative.enabled`. It creates a real encrypted V3 table with a Hive catalog and Iceberg's test KMS, so with this change its reads flow through `CometIcebergNativeScanExec` and exercise the encrypted native read path for correctness.
Follow-up:
- An in-Comet end-to-end test that asserts an encrypted table is actually read natively (rather than only asserting correctness under the Iceberg suite) requires a Hive catalog, since Hive is the only Iceberg catalog that wires the KMS-backed `EncryptionManager` in 1.11. That test infrastructure (an embedded Hive metastore in Comet's test harness) is planned as a follow-up.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: preliminary Iceberg V3 support: native table decryption, fall back for other V3 features [datafusion-comet]
Posted by "hsiang-c (via GitHub)" <gi...@apache.org>.
hsiang-c commented on code in PR #4991:
URL: https://github.com/apache/datafusion-comet/pull/4991#discussion_r3625351572
##########
spark/src/test/spark-4.1/org/apache/comet/CometIcebergEncryptionSuite.scala:
##########
@@ -0,0 +1,211 @@
+/*
+ * 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.
+ */
+
+package org.apache.comet
+
+import java.nio.ByteBuffer
+import java.util.Collections
+
+import org.apache.iceberg.Files
+import org.apache.iceberg.encryption.{EncryptedKey, NativeEncryptionKeyMetadata, StandardEncryptionManager}
+import org.apache.iceberg.util.ByteBuffers
+import org.apache.spark.SparkConf
+import org.apache.spark.sql.CometTestBase
+import org.apache.spark.sql.comet.CometIcebergNativeScanExec
+import org.apache.spark.sql.execution.adaptive.AdaptiveSparkPlanHelper
+
+import org.apache.comet.iceberg.CometTestKMS
+
+/**
+ * Encryption-specific Iceberg tests. Lives in the spark-4.1 source set because Iceberg table
+ * encryption is a V3 feature and the encryption APIs (KeyManagementClient, StandardEncryption
+ * manager) are only public from Iceberg 1.11, which Comet pairs with Spark 4.1.
+ */
+class CometIcebergEncryptionSuite
+ extends CometTestBase
+ with CometIcebergTestBase
+ with AdaptiveSparkPlanHelper {
+
+ // A Hive catalog is required for an actual encrypted table: it is the only Iceberg catalog that
+ // wires the KMS-backed EncryptionManager in 1.11 (Hadoop/REST write plaintext). Point Iceberg's
+ // type=hive catalog at an in-memory Derby metastore so no external HMS is needed. Set at session
+ // creation because Iceberg builds its HiveConf from the Hadoop configuration, which reflects
+ // spark.hadoop.* only at context startup, not from runtime withSQLConf.
+ //
+ // lazy so the temp dir is created at session init, not at construction: CI runs tests with
+ // -DwildcardSuites, which puts ScalaTest in discovery mode and instantiates every suite before
+ // any test executes. createTempDirectory does not create its parent (java.io.tmpdir, pinned to
+ // target/tmp by the pom), which does not exist yet on a fresh checkout, so an eager val aborts
+ // discovery with NoSuchFileException. createDirectories makes the parent up front.
+ private lazy val hiveWarehouse = {
+ val base = java.nio.file.Paths.get(System.getProperty("java.io.tmpdir"))
+ java.nio.file.Files.createDirectories(base)
+ java.nio.file.Files.createTempDirectory(base, "comet-hive-warehouse").toFile
+ }
+
+ override protected def sparkConf: SparkConf = {
+ super.sparkConf
+ .set(
+ "spark.hadoop.javax.jdo.option.ConnectionURL",
+ "jdbc:derby:memory:comet_iceberg_encryption;create=true")
+ .set("spark.hadoop.datanucleus.schema.autoCreateAll", "true")
+ .set("spark.hadoop.hive.metastore.schema.verification", "false")
+ // Hive's default metastore connection pool is BoneCP, which Spark's Hive fork does not ship
+ // (NoClassDefFoundError: com/jolbox/bonecp/BoneCPConfig). This config drives both the
+ // DataNucleus ObjectStore and the TxnHandler pool, so DBCP steers the embedded metastore off
+ // BoneCP. Mirrors Iceberg's TestHiveMetastore.initConf.
+ .set("spark.hadoop.datanucleus.connectionPoolingType", "DBCP")
+ .set("spark.hadoop.hive.in.test", "true")
+ // Skip Iceberg's HMS table lock. The embedded metastore's ACID TxnHandler rejects Iceberg's
+ // lock request ("Bug: operationType=UNSET"); disabling it uses the lock-free commit path
+ // (atomic alter-table with an expected-parameter check), which is all this single-writer test
+ // needs.
+ .set("spark.hadoop.iceberg.engine.hive.lock-enabled", "false")
+ .set("spark.hadoop.hive.metastore.warehouse.dir", hiveWarehouse.toURI.toString)
+ .set("spark.sql.catalog.hive_cat", "org.apache.iceberg.spark.SparkCatalog")
+ .set("spark.sql.catalog.hive_cat.type", "hive")
+ .set("spark.sql.catalog.hive_cat.warehouse", hiveWarehouse.toURI.toString)
+ .set("spark.sql.catalog.hive_cat.encryption.kms-impl", classOf[CometTestKMS].getName)
+ }
+
+ // End-to-end: an encrypted V3 table is read through Comet's native Iceberg scan and matches
+ // Spark. This is the guard that the encrypted path stays native; Iceberg's own TestTableEncryption
+ // (run under Comet in CI) only asserts correctness, not that the native scan was used.
+ test("encrypted V3 table is read natively and matches Spark") {
+ assume(icebergAvailable, "Iceberg not available in classpath")
+ assume(icebergVersionAtLeast(1, 11), "Iceberg table encryption requires Iceberg 1.11+")
+
+ withSQLConf(
+ CometConf.COMET_ENABLED.key -> "true",
+ CometConf.COMET_EXEC_ENABLED.key -> "true",
+ CometConf.COMET_ICEBERG_NATIVE_ENABLED.key -> "true") {
+
+ val table = "hive_cat.db.encrypted"
+ // The Hive catalog does not auto-create namespaces the way the Hadoop catalog does.
+ spark.sql("CREATE NAMESPACE IF NOT EXISTS hive_cat.db")
+ spark.sql(s"DROP TABLE IF EXISTS $table")
+ spark.sql(s"""
+ CREATE TABLE $table (id INT, name STRING, value DOUBLE) USING iceberg
+ TBLPROPERTIES ('format-version' = '3', 'encryption.key-id' = '${CometTestKMS.MasterKeyId}')
+ """)
+ spark.sql(s"""
+ INSERT INTO $table VALUES (1, 'Alice', 10.5), (2, 'Bob', 20.3), (3, 'Charlie', 30.7)
+ """)
+
+ // Guard against a silently-plaintext table (e.g. catalog not wiring encryption): a genuinely
+ // encrypted table records a plaintext DEK blob per data file in key_metadata.
+ val keyMetadatas = spark
+ .sql(s"SELECT key_metadata FROM $table.files WHERE content = 0")
Review Comment:
The `file_path` columns is part of the `.files` table and one check I have is reading the first 4 bytes of the Parquet file see if it is the `PARE` magic string.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: preliminary Iceberg V3 support: native table decryption, fall back for other V3 features [datafusion-comet]
Posted by "hsiang-c (via GitHub)" <gi...@apache.org>.
hsiang-c commented on code in PR #4991:
URL: https://github.com/apache/datafusion-comet/pull/4991#discussion_r3625326768
##########
spark/pom.xml:
##########
@@ -107,6 +107,13 @@ under the License.
<artifactId>spark-hadoop-cloud_${scala.binary.version}</artifactId>
<classifier>tests</classifier>
</dependency>
+ <dependency>
+ <!-- Test-only: provides an embedded Hive metastore so Iceberg's type=hive catalog can
+ create encrypted tables (only Hive wires the KMS-backed EncryptionManager). Version and
+ scope come from the parent pom's dependencyManagement. -->
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-hive_${scala.binary.version}</artifactId>
Review Comment:
Do we need
```xml
<scope>test</scope>
```
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Iceberg table format V3: native table decryption, fall back for other V3 features [datafusion-comet]
Posted by "hsiang-c (via GitHub)" <gi...@apache.org>.
hsiang-c commented on code in PR #4991:
URL: https://github.com/apache/datafusion-comet/pull/4991#discussion_r3640078350
##########
spark/src/test/spark-4.1/org/apache/comet/CometIcebergEncryptionSuite.scala:
##########
@@ -0,0 +1,261 @@
+/*
+ * 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.
+ */
+
+package org.apache.comet
+
+import java.io.File
+import java.nio.ByteBuffer
+import java.util.Collections
+
+import org.apache.iceberg.Files
+import org.apache.iceberg.encryption.{EncryptedKey, NativeEncryptionKeyMetadata, StandardEncryptionManager}
+import org.apache.iceberg.util.ByteBuffers
+import org.apache.spark.SparkConf
+import org.apache.spark.sql.CometTestBase
+import org.apache.spark.sql.comet.CometIcebergNativeScanExec
+import org.apache.spark.sql.execution.adaptive.AdaptiveSparkPlanHelper
+
+import org.apache.comet.iceberg.CometTestKMS
+
+/**
+ * Encryption-specific Iceberg tests. Lives in the spark-4.1 source set because Iceberg table
+ * encryption is a V3 feature and the encryption APIs (KeyManagementClient, StandardEncryption
+ * manager) are only public from Iceberg 1.11, which Comet pairs with Spark 4.1.
+ */
+class CometIcebergEncryptionSuite
+ extends CometTestBase
+ with CometIcebergTestBase
+ with AdaptiveSparkPlanHelper {
+
+ // A Hive catalog is required for an actual encrypted table: it is the only Iceberg catalog that
+ // wires the KMS-backed EncryptionManager in 1.11 (Hadoop/REST write plaintext). Point Iceberg's
+ // type=hive catalog at an in-memory Derby metastore so no external HMS is needed. Set at session
+ // creation because Iceberg builds its HiveConf from the Hadoop configuration, which reflects
+ // spark.hadoop.* only at context startup, not from runtime withSQLConf.
+ //
+ // lazy so the temp dir is created at session init, not at construction: CI runs tests with
+ // -DwildcardSuites, which puts ScalaTest in discovery mode and instantiates every suite before
+ // any test executes. createTempDirectory does not create its parent (java.io.tmpdir, pinned to
+ // target/tmp by the pom), which does not exist yet on a fresh checkout, so an eager val aborts
+ // discovery with NoSuchFileException. createDirectories makes the parent up front.
+ private lazy val hiveWarehouse = {
+ val base = java.nio.file.Paths.get(System.getProperty("java.io.tmpdir"))
+ java.nio.file.Files.createDirectories(base)
+ val dir = java.nio.file.Files.createTempDirectory(base, "comet-hive-warehouse").toFile
+ createdWarehouse = dir
+ dir
+ }
+
+ // Captured on init rather than read from the lazy val so a run that never touched it (all tests
+ // skipped) creates nothing to delete.
+ private var createdWarehouse: File = _
+
+ override protected def afterAll(): Unit = {
+ try if (createdWarehouse != null) deleteRecursively(createdWarehouse)
+ finally super.afterAll()
+ }
+
+ override protected def sparkConf: SparkConf = {
+ super.sparkConf
+ .set(
+ "spark.hadoop.javax.jdo.option.ConnectionURL",
+ "jdbc:derby:memory:comet_iceberg_encryption;create=true")
+ .set("spark.hadoop.datanucleus.schema.autoCreateAll", "true")
+ .set("spark.hadoop.hive.metastore.schema.verification", "false")
+ // Hive's default metastore connection pool is BoneCP, which Spark's Hive fork does not ship
+ // (NoClassDefFoundError: com/jolbox/bonecp/BoneCPConfig). This config drives both the
+ // DataNucleus ObjectStore and the TxnHandler pool, so DBCP steers the embedded metastore off
+ // BoneCP. Mirrors Iceberg's TestHiveMetastore.initConf.
+ .set("spark.hadoop.datanucleus.connectionPoolingType", "DBCP")
+ .set("spark.hadoop.hive.in.test", "true")
+ // Skip Iceberg's HMS table lock. The embedded metastore's ACID TxnHandler rejects Iceberg's
+ // lock request ("Bug: operationType=UNSET"); disabling it uses the lock-free commit path
+ // (atomic alter-table with an expected-parameter check), which is all this single-writer test
+ // needs.
+ .set("spark.hadoop.iceberg.engine.hive.lock-enabled", "false")
+ .set("spark.hadoop.hive.metastore.warehouse.dir", hiveWarehouse.toURI.toString)
+ .set("spark.sql.catalog.hive_cat", "org.apache.iceberg.spark.SparkCatalog")
+ .set("spark.sql.catalog.hive_cat.type", "hive")
+ .set("spark.sql.catalog.hive_cat.warehouse", hiveWarehouse.toURI.toString)
+ .set("spark.sql.catalog.hive_cat.encryption.kms-impl", classOf[CometTestKMS].getName)
+ }
+
+ // End-to-end: an encrypted V3 table is read through Comet's native Iceberg scan and matches
+ // Spark. This is the guard that the encrypted path stays native; Iceberg's own TestTableEncryption
+ // (run under Comet in CI) only asserts correctness, not that the native scan was used.
+ test("encrypted V3 table is read natively and matches Spark") {
+ assume(icebergAvailable, "Iceberg not available in classpath")
+ assume(icebergVersionAtLeast(1, 11), "Iceberg table encryption requires Iceberg 1.11+")
+
+ withSQLConf(
+ CometConf.COMET_ENABLED.key -> "true",
+ CometConf.COMET_EXEC_ENABLED.key -> "true",
+ CometConf.COMET_ICEBERG_NATIVE_ENABLED.key -> "true") {
+
+ val table = "hive_cat.db.encrypted"
+ // The Hive catalog does not auto-create namespaces the way the Hadoop catalog does.
+ spark.sql("CREATE NAMESPACE IF NOT EXISTS hive_cat.db")
+ spark.sql(s"DROP TABLE IF EXISTS $table")
+ spark.sql(s"""
+ CREATE TABLE $table (id INT, name STRING, value DOUBLE) USING iceberg
+ TBLPROPERTIES ('format-version' = '3', 'encryption.key-id' = '${CometTestKMS.MasterKeyId}')
+ """)
+ spark.sql(s"""
+ INSERT INTO $table VALUES (1, 'Alice', 10.5), (2, 'Bob', 20.3), (3, 'Charlie', 30.7)
+ """)
+
+ // Guard against a silently-plaintext table (e.g. catalog not wiring encryption): a genuinely
+ // encrypted table records a plaintext DEK blob per data file in key_metadata.
+ val keyMetadatas = spark
+ .sql(s"SELECT key_metadata FROM $table.files WHERE content = 0")
+ .collect()
+ .flatMap(r => Option(r.getAs[Array[Byte]]("key_metadata")))
+ assert(
+ keyMetadatas.nonEmpty,
+ "expected encryption to populate key_metadata; the Hive catalog may not have wired the KMS")
+
+ val (_, cometPlan) = checkSparkAnswer(s"SELECT * FROM $table ORDER BY id")
+ assert(
+ collect(cometPlan) { case s: CometIcebergNativeScanExec => s }.nonEmpty,
+ "expected the encrypted read to run natively but found no CometIcebergNativeScanExec:\n" +
+ cometPlan)
+
+ spark.sql(s"DROP TABLE $table")
+ }
+ }
+
+ // Confirms the assumption that native encrypted reads rely on: an Iceberg data file's
+ // key_metadata carries a PLAINTEXT data encryption key, not a KMS-wrapped one. Iceberg mints a
+ // fresh DEK per file in StandardEncryptionManager.encrypt() and records it plaintext in
+ // key_metadata (the manifest that holds it is itself encrypted). The KMS only wraps the KEK /
+ // manifest-list keys, never the per-file DEK. Runs without a catalog.
+ test("data file key_metadata carries a plaintext DEK that round-trips") {
+ assume(icebergAvailable, "Iceberg not available in classpath")
+ assume(icebergVersionAtLeast(1, 11), "Iceberg table encryption requires Iceberg 1.11+")
+
+ val dataKeyLength = 16
+ val mgr = new StandardEncryptionManager(
+ Collections.emptyList[EncryptedKey](),
+ CometTestKMS.MasterKeyId,
+ dataKeyLength,
+ new CometTestKMS)
+
+ withTempDir { dir =>
+ // encrypt() lazily generates the per-file DEK + AAD prefix; no bytes are written.
+ val keyMetadata =
+ mgr.encrypt(Files.localOutput(new java.io.File(dir, "data.parquet"))).keyMetadata()
+
+ val dek = keyMetadata.encryptionKey()
+ assert(
+ dek.remaining() == dataKeyLength,
+ s"expected a plaintext $dataKeyLength-byte DEK but got ${dek.remaining()} bytes")
+ val aad = keyMetadata.aadPrefix()
+ assert(aad != null && aad.remaining() > 0, "expected a non-empty AAD prefix")
+
+ // buffer() is the serialized StandardKeyMetadata blob stored in the manifest's key_metadata
+ // field, and the exact bytes Comet would forward to iceberg-rust. It must start with the V1
+ // marker and round-trip back to the same DEK. StandardKeyMetadata.parse is package-private,
+ // so reach it reflectively, but read the result through the public NativeEncryptionKeyMetadata
+ // interface (the concrete class is not accessible). iceberg-rust decodes this same format.
+ val blob = ByteBuffers.toByteArray(keyMetadata.buffer())
+ assert(blob(0) == 1, s"unexpected key_metadata version byte: ${blob(0)}")
+
+ val kmClass = Class.forName("org.apache.iceberg.encryption.StandardKeyMetadata")
+ val parse = kmClass.getDeclaredMethod("parse", classOf[ByteBuffer])
+ parse.setAccessible(true)
+ val parsed =
+ parse.invoke(null, ByteBuffer.wrap(blob)).asInstanceOf[NativeEncryptionKeyMetadata]
+ val roundTripped = ByteBuffers.toByteArray(parsed.encryptionKey())
+
+ assert(
+ java.util.Arrays.equals(ByteBuffers.toByteArray(dek), roundTripped),
+ "DEK did not survive the StandardKeyMetadata encode/parse round-trip")
+ }
+ }
+
+ // A 256-bit (32-byte) AES-GCM data key is supported natively, so the read must run through Comet
+ // and match Spark.
+ test("encrypted V3 table with a 256-bit data key is read natively and matches Spark") {
Review Comment:
This updated test LGTM, thanks @mbutrovich
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: preliminary Iceberg V3 support: native table decryption, fall back for other V3 features [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #4991:
URL: https://github.com/apache/datafusion-comet/pull/4991#discussion_r3625406928
##########
spark/src/test/spark-4.1/org/apache/comet/CometIcebergEncryptionSuite.scala:
##########
@@ -0,0 +1,211 @@
+/*
+ * 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.
+ */
+
+package org.apache.comet
+
+import java.nio.ByteBuffer
+import java.util.Collections
+
+import org.apache.iceberg.Files
+import org.apache.iceberg.encryption.{EncryptedKey, NativeEncryptionKeyMetadata, StandardEncryptionManager}
+import org.apache.iceberg.util.ByteBuffers
+import org.apache.spark.SparkConf
+import org.apache.spark.sql.CometTestBase
+import org.apache.spark.sql.comet.CometIcebergNativeScanExec
+import org.apache.spark.sql.execution.adaptive.AdaptiveSparkPlanHelper
+
+import org.apache.comet.iceberg.CometTestKMS
+
+/**
+ * Encryption-specific Iceberg tests. Lives in the spark-4.1 source set because Iceberg table
+ * encryption is a V3 feature and the encryption APIs (KeyManagementClient, StandardEncryption
+ * manager) are only public from Iceberg 1.11, which Comet pairs with Spark 4.1.
+ */
+class CometIcebergEncryptionSuite
+ extends CometTestBase
+ with CometIcebergTestBase
+ with AdaptiveSparkPlanHelper {
+
+ // A Hive catalog is required for an actual encrypted table: it is the only Iceberg catalog that
+ // wires the KMS-backed EncryptionManager in 1.11 (Hadoop/REST write plaintext). Point Iceberg's
+ // type=hive catalog at an in-memory Derby metastore so no external HMS is needed. Set at session
+ // creation because Iceberg builds its HiveConf from the Hadoop configuration, which reflects
+ // spark.hadoop.* only at context startup, not from runtime withSQLConf.
+ //
+ // lazy so the temp dir is created at session init, not at construction: CI runs tests with
+ // -DwildcardSuites, which puts ScalaTest in discovery mode and instantiates every suite before
+ // any test executes. createTempDirectory does not create its parent (java.io.tmpdir, pinned to
+ // target/tmp by the pom), which does not exist yet on a fresh checkout, so an eager val aborts
+ // discovery with NoSuchFileException. createDirectories makes the parent up front.
+ private lazy val hiveWarehouse = {
+ val base = java.nio.file.Paths.get(System.getProperty("java.io.tmpdir"))
+ java.nio.file.Files.createDirectories(base)
+ java.nio.file.Files.createTempDirectory(base, "comet-hive-warehouse").toFile
+ }
+
+ override protected def sparkConf: SparkConf = {
+ super.sparkConf
+ .set(
+ "spark.hadoop.javax.jdo.option.ConnectionURL",
+ "jdbc:derby:memory:comet_iceberg_encryption;create=true")
+ .set("spark.hadoop.datanucleus.schema.autoCreateAll", "true")
+ .set("spark.hadoop.hive.metastore.schema.verification", "false")
+ // Hive's default metastore connection pool is BoneCP, which Spark's Hive fork does not ship
+ // (NoClassDefFoundError: com/jolbox/bonecp/BoneCPConfig). This config drives both the
+ // DataNucleus ObjectStore and the TxnHandler pool, so DBCP steers the embedded metastore off
+ // BoneCP. Mirrors Iceberg's TestHiveMetastore.initConf.
+ .set("spark.hadoop.datanucleus.connectionPoolingType", "DBCP")
+ .set("spark.hadoop.hive.in.test", "true")
+ // Skip Iceberg's HMS table lock. The embedded metastore's ACID TxnHandler rejects Iceberg's
+ // lock request ("Bug: operationType=UNSET"); disabling it uses the lock-free commit path
+ // (atomic alter-table with an expected-parameter check), which is all this single-writer test
+ // needs.
+ .set("spark.hadoop.iceberg.engine.hive.lock-enabled", "false")
+ .set("spark.hadoop.hive.metastore.warehouse.dir", hiveWarehouse.toURI.toString)
+ .set("spark.sql.catalog.hive_cat", "org.apache.iceberg.spark.SparkCatalog")
+ .set("spark.sql.catalog.hive_cat.type", "hive")
+ .set("spark.sql.catalog.hive_cat.warehouse", hiveWarehouse.toURI.toString)
+ .set("spark.sql.catalog.hive_cat.encryption.kms-impl", classOf[CometTestKMS].getName)
+ }
+
+ // End-to-end: an encrypted V3 table is read through Comet's native Iceberg scan and matches
+ // Spark. This is the guard that the encrypted path stays native; Iceberg's own TestTableEncryption
+ // (run under Comet in CI) only asserts correctness, not that the native scan was used.
+ test("encrypted V3 table is read natively and matches Spark") {
+ assume(icebergAvailable, "Iceberg not available in classpath")
+ assume(icebergVersionAtLeast(1, 11), "Iceberg table encryption requires Iceberg 1.11+")
+
+ withSQLConf(
+ CometConf.COMET_ENABLED.key -> "true",
+ CometConf.COMET_EXEC_ENABLED.key -> "true",
+ CometConf.COMET_ICEBERG_NATIVE_ENABLED.key -> "true") {
+
+ val table = "hive_cat.db.encrypted"
+ // The Hive catalog does not auto-create namespaces the way the Hadoop catalog does.
+ spark.sql("CREATE NAMESPACE IF NOT EXISTS hive_cat.db")
+ spark.sql(s"DROP TABLE IF EXISTS $table")
+ spark.sql(s"""
+ CREATE TABLE $table (id INT, name STRING, value DOUBLE) USING iceberg
+ TBLPROPERTIES ('format-version' = '3', 'encryption.key-id' = '${CometTestKMS.MasterKeyId}')
+ """)
+ spark.sql(s"""
+ INSERT INTO $table VALUES (1, 'Alice', 10.5), (2, 'Bob', 20.3), (3, 'Charlie', 30.7)
+ """)
+
+ // Guard against a silently-plaintext table (e.g. catalog not wiring encryption): a genuinely
+ // encrypted table records a plaintext DEK blob per data file in key_metadata.
+ val keyMetadatas = spark
+ .sql(s"SELECT key_metadata FROM $table.files WHERE content = 0")
Review Comment:
Thanks, that is a clean way to confirm encryption at rest. I left it out here because the suite already proves the same thing end to end: it asserts key_metadata is populated and that the read goes through CometIcebergNativeScanExec while matching Spark. For that native result to be correct, arrow-rs had to decrypt the file with the DEK from key_metadata. A plaintext PAR1 file read with decryption properties set would error rather than return correct rows, so a passing native answer already implies the bytes were encrypted and decrypted correctly. Reading the PARE magic would check the same property one layer lower, inside a detail Iceberg and arrow-rs own, so I would rather keep the test at the API boundary.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Iceberg table format V3: native table decryption, fall back for other V3 features [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich merged PR #4991:
URL: https://github.com/apache/datafusion-comet/pull/4991
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: preliminary Iceberg V3 support: native table decryption, fall back for other V3 features [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #4991:
URL: https://github.com/apache/datafusion-comet/pull/4991#discussion_r3625403635
##########
docs/source/user-guide/latest/iceberg.md:
##########
@@ -63,7 +63,14 @@ The native Iceberg reader supports the following features:
**Table specifications:**
-- Iceberg table spec v1 and v2 (v3 will fall back to Spark)
+- Iceberg table spec v1, v2, and v3
+
+**Encryption:**
+
+- Encrypted v3 tables using 128-bit AES-GCM data keys (requires Iceberg 1.11 or newer). Iceberg-Java
Review Comment:
Good call. Updated the docs to note that Iceberg-Java also permits 192- and 256-bit data keys, and that those tables fall back to Spark for now since the native Parquet reader decrypts only 128-bit AES-GCM until arrow-rs 58.4.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: preliminary Iceberg V3 support: native table decryption, fall back for other V3 features [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #4991:
URL: https://github.com/apache/datafusion-comet/pull/4991#issuecomment-5038619034
> LGTM
Thanks for the review @hsiang-c!
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: preliminary Iceberg V3 support: native table decryption, fall back for other V3 features [datafusion-comet]
Posted by "hsiang-c (via GitHub)" <gi...@apache.org>.
hsiang-c commented on code in PR #4991:
URL: https://github.com/apache/datafusion-comet/pull/4991#discussion_r3625329819
##########
spark/src/test/spark-4.1/org/apache/comet/CometIcebergEncryptionSuite.scala:
##########
@@ -0,0 +1,211 @@
+/*
+ * 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.
+ */
+
+package org.apache.comet
+
+import java.nio.ByteBuffer
+import java.util.Collections
+
+import org.apache.iceberg.Files
+import org.apache.iceberg.encryption.{EncryptedKey, NativeEncryptionKeyMetadata, StandardEncryptionManager}
+import org.apache.iceberg.util.ByteBuffers
+import org.apache.spark.SparkConf
+import org.apache.spark.sql.CometTestBase
+import org.apache.spark.sql.comet.CometIcebergNativeScanExec
+import org.apache.spark.sql.execution.adaptive.AdaptiveSparkPlanHelper
+
+import org.apache.comet.iceberg.CometTestKMS
+
+/**
+ * Encryption-specific Iceberg tests. Lives in the spark-4.1 source set because Iceberg table
+ * encryption is a V3 feature and the encryption APIs (KeyManagementClient, StandardEncryption
+ * manager) are only public from Iceberg 1.11, which Comet pairs with Spark 4.1.
+ */
+class CometIcebergEncryptionSuite
+ extends CometTestBase
+ with CometIcebergTestBase
+ with AdaptiveSparkPlanHelper {
+
+ // A Hive catalog is required for an actual encrypted table: it is the only Iceberg catalog that
+ // wires the KMS-backed EncryptionManager in 1.11 (Hadoop/REST write plaintext). Point Iceberg's
+ // type=hive catalog at an in-memory Derby metastore so no external HMS is needed. Set at session
+ // creation because Iceberg builds its HiveConf from the Hadoop configuration, which reflects
+ // spark.hadoop.* only at context startup, not from runtime withSQLConf.
+ //
+ // lazy so the temp dir is created at session init, not at construction: CI runs tests with
+ // -DwildcardSuites, which puts ScalaTest in discovery mode and instantiates every suite before
+ // any test executes. createTempDirectory does not create its parent (java.io.tmpdir, pinned to
+ // target/tmp by the pom), which does not exist yet on a fresh checkout, so an eager val aborts
+ // discovery with NoSuchFileException. createDirectories makes the parent up front.
+ private lazy val hiveWarehouse = {
+ val base = java.nio.file.Paths.get(System.getProperty("java.io.tmpdir"))
+ java.nio.file.Files.createDirectories(base)
+ java.nio.file.Files.createTempDirectory(base, "comet-hive-warehouse").toFile
+ }
+
+ override protected def sparkConf: SparkConf = {
+ super.sparkConf
+ .set(
+ "spark.hadoop.javax.jdo.option.ConnectionURL",
+ "jdbc:derby:memory:comet_iceberg_encryption;create=true")
+ .set("spark.hadoop.datanucleus.schema.autoCreateAll", "true")
+ .set("spark.hadoop.hive.metastore.schema.verification", "false")
+ // Hive's default metastore connection pool is BoneCP, which Spark's Hive fork does not ship
+ // (NoClassDefFoundError: com/jolbox/bonecp/BoneCPConfig). This config drives both the
+ // DataNucleus ObjectStore and the TxnHandler pool, so DBCP steers the embedded metastore off
+ // BoneCP. Mirrors Iceberg's TestHiveMetastore.initConf.
+ .set("spark.hadoop.datanucleus.connectionPoolingType", "DBCP")
+ .set("spark.hadoop.hive.in.test", "true")
+ // Skip Iceberg's HMS table lock. The embedded metastore's ACID TxnHandler rejects Iceberg's
+ // lock request ("Bug: operationType=UNSET"); disabling it uses the lock-free commit path
+ // (atomic alter-table with an expected-parameter check), which is all this single-writer test
+ // needs.
+ .set("spark.hadoop.iceberg.engine.hive.lock-enabled", "false")
+ .set("spark.hadoop.hive.metastore.warehouse.dir", hiveWarehouse.toURI.toString)
+ .set("spark.sql.catalog.hive_cat", "org.apache.iceberg.spark.SparkCatalog")
+ .set("spark.sql.catalog.hive_cat.type", "hive")
+ .set("spark.sql.catalog.hive_cat.warehouse", hiveWarehouse.toURI.toString)
+ .set("spark.sql.catalog.hive_cat.encryption.kms-impl", classOf[CometTestKMS].getName)
+ }
+
+ // End-to-end: an encrypted V3 table is read through Comet's native Iceberg scan and matches
+ // Spark. This is the guard that the encrypted path stays native; Iceberg's own TestTableEncryption
+ // (run under Comet in CI) only asserts correctness, not that the native scan was used.
+ test("encrypted V3 table is read natively and matches Spark") {
+ assume(icebergAvailable, "Iceberg not available in classpath")
+ assume(icebergVersionAtLeast(1, 11), "Iceberg table encryption requires Iceberg 1.11+")
+
+ withSQLConf(
+ CometConf.COMET_ENABLED.key -> "true",
+ CometConf.COMET_EXEC_ENABLED.key -> "true",
+ CometConf.COMET_ICEBERG_NATIVE_ENABLED.key -> "true") {
+
+ val table = "hive_cat.db.encrypted"
+ // The Hive catalog does not auto-create namespaces the way the Hadoop catalog does.
+ spark.sql("CREATE NAMESPACE IF NOT EXISTS hive_cat.db")
+ spark.sql(s"DROP TABLE IF EXISTS $table")
+ spark.sql(s"""
+ CREATE TABLE $table (id INT, name STRING, value DOUBLE) USING iceberg
+ TBLPROPERTIES ('format-version' = '3', 'encryption.key-id' = '${CometTestKMS.MasterKeyId}')
+ """)
+ spark.sql(s"""
+ INSERT INTO $table VALUES (1, 'Alice', 10.5), (2, 'Bob', 20.3), (3, 'Charlie', 30.7)
+ """)
+
+ // Guard against a silently-plaintext table (e.g. catalog not wiring encryption): a genuinely
+ // encrypted table records a plaintext DEK blob per data file in key_metadata.
+ val keyMetadatas = spark
+ .sql(s"SELECT key_metadata FROM $table.files WHERE content = 0")
+ .collect()
+ .flatMap(r => Option(r.getAs[Array[Byte]]("key_metadata")))
+ assert(
+ keyMetadatas.nonEmpty,
+ "expected encryption to populate key_metadata; the Hive catalog may not have wired the KMS")
+
+ val (_, cometPlan) = checkSparkAnswer(s"SELECT * FROM $table ORDER BY id")
+ assert(
+ collect(cometPlan) { case s: CometIcebergNativeScanExec => s }.nonEmpty,
+ "expected the encrypted read to run natively but found no CometIcebergNativeScanExec:\n" +
+ cometPlan)
+
+ spark.sql(s"DROP TABLE $table")
+ }
+ }
+
+ // Confirms the assumption that native encrypted reads rely on: an Iceberg data file's
+ // key_metadata carries a PLAINTEXT data encryption key, not a KMS-wrapped one. Iceberg mints a
+ // fresh DEK per file in StandardEncryptionManager.encrypt() and records it plaintext in
+ // key_metadata (the manifest that holds it is itself encrypted). The KMS only wraps the KEK /
+ // manifest-list keys, never the per-file DEK. Runs without a catalog.
+ test("data file key_metadata carries a plaintext DEK that round-trips") {
+ assume(icebergAvailable, "Iceberg not available in classpath")
+ assume(icebergVersionAtLeast(1, 11), "Iceberg table encryption requires Iceberg 1.11+")
+
+ val dataKeyLength = 16
+ val mgr = new StandardEncryptionManager(
+ Collections.emptyList[EncryptedKey](),
+ CometTestKMS.MasterKeyId,
+ dataKeyLength,
+ new CometTestKMS)
+
+ withTempDir { dir =>
+ // encrypt() lazily generates the per-file DEK + AAD prefix; no bytes are written.
+ val keyMetadata =
+ mgr.encrypt(Files.localOutput(new java.io.File(dir, "data.parquet"))).keyMetadata()
+
+ val dek = keyMetadata.encryptionKey()
+ assert(
+ dek.remaining() == dataKeyLength,
+ s"expected a plaintext $dataKeyLength-byte DEK but got ${dek.remaining()} bytes")
+ val aad = keyMetadata.aadPrefix()
+ assert(aad != null && aad.remaining() > 0, "expected a non-empty AAD prefix")
+
+ // buffer() is the serialized StandardKeyMetadata blob stored in the manifest's key_metadata
+ // field, and the exact bytes Comet would forward to iceberg-rust. It must start with the V1
+ // marker and round-trip back to the same DEK. StandardKeyMetadata.parse is package-private,
+ // so reach it reflectively, but read the result through the public NativeEncryptionKeyMetadata
+ // interface (the concrete class is not accessible). iceberg-rust decodes this same format.
+ val blob = ByteBuffers.toByteArray(keyMetadata.buffer())
+ assert(blob(0) == 1, s"unexpected key_metadata version byte: ${blob(0)}")
+
+ val kmClass = Class.forName("org.apache.iceberg.encryption.StandardKeyMetadata")
+ val parse = kmClass.getDeclaredMethod("parse", classOf[ByteBuffer])
+ parse.setAccessible(true)
+ val parsed =
+ parse.invoke(null, ByteBuffer.wrap(blob)).asInstanceOf[NativeEncryptionKeyMetadata]
+ val roundTripped = ByteBuffers.toByteArray(parsed.encryptionKey())
+
+ assert(
+ java.util.Arrays.equals(ByteBuffers.toByteArray(dek), roundTripped),
+ "DEK did not survive the StandardKeyMetadata encode/parse round-trip")
+ }
+ }
+
+ // The native Parquet reader (arrow-rs 58.3) decrypts only 128-bit AES-GCM keys, so a table with a
+ // larger data key must fall back to Spark and still return correct results. Flip this to a native
+ // assertion when arrow-rs 58.4 (256-bit support, arrow-rs#10349) is picked up.
+ test("encrypted V3 table with a 256-bit data key falls back to Spark") {
Review Comment:
👍
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Iceberg table format V3: native table decryption, fall back for other V3 features [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #4991:
URL: https://github.com/apache/datafusion-comet/pull/4991#discussion_r3638658134
##########
native/proto/src/proto/operator.proto:
##########
@@ -277,6 +282,13 @@ message IcebergDeleteFile {
// Equality field IDs (empty for positional deletes)
repeated int32 equality_ids = 4;
+
+ // Serialized StandardKeyMetadata for an encrypted delete file, from DeleteFile.keyMetadata().
+ // Absent for unencrypted tables. Decoded directly by iceberg-rust; no KMS unwrap needed.
+ // Field 8: fields 5-7 are reserved for the deletion-vector coordinates on the dv-read branch
Review Comment:
Should we add an explicit `reserved 5, 6, 7` ?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: preliminary Iceberg V3 support: native table decryption, fall back for other V3 features [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #4991:
URL: https://github.com/apache/datafusion-comet/pull/4991#discussion_r3625405109
##########
spark/pom.xml:
##########
@@ -107,6 +107,13 @@ under the License.
<artifactId>spark-hadoop-cloud_${scala.binary.version}</artifactId>
<classifier>tests</classifier>
</dependency>
+ <dependency>
+ <!-- Test-only: provides an embedded Hive metastore so Iceberg's type=hive catalog can
+ create encrypted tables (only Hive wires the KMS-backed EncryptionManager). Version and
+ scope come from the parent pom's dependencyManagement. -->
+ <groupId>org.apache.spark</groupId>
+ <artifactId>spark-hive_${scala.binary.version}</artifactId>
Review Comment:
The scope is inherited from the parent pom's dependencyManagement, which declares spark-hive as test scope. That is why it is omitted here, matching the spark-sql and spark-hadoop-cloud test deps just above. The inline comment notes this. Happy to make it explicit if you would prefer it spelled out on the dependency itself.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: preliminary Iceberg V3 support: native table decryption, fall back for other V3 features [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #4991:
URL: https://github.com/apache/datafusion-comet/pull/4991#discussion_r3631213270
##########
.github/actions/java-test/action.yaml:
##########
@@ -50,6 +50,18 @@ runs:
cd native
cargo build --release
+ # Evaluate hashFiles now, on the clean checkout, and reuse the result in the
+ # cache key below. The cache step's inputs are re-evaluated when its post
+ # (cache-save) phase runs, and by then the test run has left trees under the
+ # workspace (e.g. the Hive-catalog warehouse from CometIcebergEncryptionSuite)
+ # that make a fresh '**/pom.xml' walk throw "Fail to hash files under
+ # directory". A precomputed string keeps the save phase from re-walking them.
Review Comment:
I also added some cleanup logic for the Hive-catalog, but just in case I'm also addressing the footgun here.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: preliminary Iceberg V3 support: native table decryption, fall back for other V3 features [datafusion-comet]
Posted by "hsiang-c (via GitHub)" <gi...@apache.org>.
hsiang-c commented on code in PR #4991:
URL: https://github.com/apache/datafusion-comet/pull/4991#discussion_r3625311250
##########
docs/source/user-guide/latest/iceberg.md:
##########
@@ -63,7 +63,14 @@ The native Iceberg reader supports the following features:
**Table specifications:**
-- Iceberg table spec v1 and v2 (v3 will fall back to Spark)
+- Iceberg table spec v1, v2, and v3
+
+**Encryption:**
+
+- Encrypted v3 tables using 128-bit AES-GCM data keys (requires Iceberg 1.11 or newer). Iceberg-Java
Review Comment:
(nit) I think we could mention 192-bit and 256-bit are allowed in Iceberg Java: https://github.com/apache/iceberg/blob/apache-iceberg-1.11.0/core/src/main/java/org/apache/iceberg/encryption/EncryptionUtil.java#L117-L120
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: preliminary Iceberg V3 support: native table decryption, fall back for other V3 features [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #4991:
URL: https://github.com/apache/datafusion-comet/pull/4991#discussion_r3631213270
##########
.github/actions/java-test/action.yaml:
##########
@@ -50,6 +50,18 @@ runs:
cd native
cargo build --release
+ # Evaluate hashFiles now, on the clean checkout, and reuse the result in the
+ # cache key below. The cache step's inputs are re-evaluated when its post
+ # (cache-save) phase runs, and by then the test run has left trees under the
+ # workspace (e.g. the Hive-catalog warehouse from CometIcebergEncryptionSuite)
+ # that make a fresh '**/pom.xml' walk throw "Fail to hash files under
+ # directory". A precomputed string keeps the save phase from re-walking them.
Review Comment:
I also added some cleanup logic for the Hive-catalog, but just in case I'm also addressing the footgun here. It fixed a failure in the `Post Java test steps`:
```
Post job cleanup.
Error: The template is not valid. /home/runner/work/datafusion-comet/datafusion-comet/./.github/actions/java-test/action.yaml (Line: 63, Col: 14): hashFiles('**/pom.xml') failed. Fail to hash files under directory '/home/runner/work/datafusion-comet/datafusion-comet'
```
that I saw after adding the Hive catalog https://github.com/apache/datafusion-comet/actions/runs/29912607672/job/88905543943
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org