You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "yunchipang (via GitHub)" <gi...@apache.org> on 2026/10/18 02:07:37 UTC

[PR] feat: write top-level Iceberg unknown columns natively [datafusion-comet]

yunchipang opened a new pull request, #7100:
URL: https://github.com/apache/datafusion-comet/pull/7100

   ## 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 #6959.
   
   ## 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.
   -->
   
   Today the native writer falls back whenever the write schema has a v3 `unknown` column. iceberg-java writes no Parquet column for a top-level `unknown` field, so the native writer can support it by doing the same.
   
   ## 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: drop top-level `unknown` fields when parsing the schema, and skip the matching `Null` columns before writing.
   - JVM: a top-level `unknown` no longer falls back. A nested `unknown` (in a struct, list or map) still falls back:
     - struct: Comet shuffle panics on a `Null` field inside a struct (the field-major path in `row.rs`). Looks like https://github.com/apache/datafusion-comet/pull/7099 will fix this bug.
     - list/map: iceberg-java cannot write these either.
   - Docs: update the supported column types.
   
   ## 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)?
   -->
   
   - Rust unit test: `decorate_batch_drops_null_columns`.
   - `CometIcebergWriteActionSuite`: writes the same v3 table with the native writer and with iceberg-java, then checks that the rows, the Parquet footer columns and the manifest metrics match.
   - `CometIcebergWriteDetectionSuite`: an `unknown` inside a struct falls back.
   - Verified on Spark 4.1 / Iceberg 1.11. The other Iceberg versions are left to the `run-iceberg-tests` label.


-- 
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: write top-level Iceberg unknown columns natively [datafusion-comet]

Posted by "yunchipang (via GitHub)" <gi...@apache.org>.
yunchipang commented on PR #7100:
URL: https://github.com/apache/datafusion-comet/pull/7100#issuecomment-6244572841

   looks like https://github.com/apache/iceberg-rust/pull/3072 is resolving the issue from the root though.


-- 
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: write top-level Iceberg unknown columns natively [datafusion-comet]

Posted by "yunchipang (via GitHub) via github" <gi...@datafusion.apache.org>.
yunchipang commented on PR #7100:
URL: https://github.com/apache/datafusion-comet/pull/7100#issuecomment-6562647613

   failed CI root cause: iceberg-java 1.10 has no comparator for an `unknown` partition value, so the fallback write fails with "Cannot determine comparator for type: unknown" and both unknown-partition tests failed on Spark 4.0. fix:
   - Detection suite: allow the write to fail; the test only checks the decline reason.
   - Write action suite: on Iceberg 1.11+ keep asserting the fallback write succeeds; on 1.10 assert it fails the way Spark's own write does.


-- 
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: write top-level Iceberg unknown columns natively [datafusion-comet]

Posted by "yunchipang (via GitHub)" <gi...@apache.org>.
yunchipang commented on code in PR #7100:
URL: https://github.com/apache/datafusion-comet/pull/7100#discussion_r4307216707


##########
native/core/src/execution/operators/iceberg_write.rs:
##########
@@ -1488,6 +1488,22 @@ fn parse_iceberg_schema(json: &str) -> DFResult<IcebergSchemaRef> {
     let schema: IcebergSchema = serde_json::from_str(json).map_err(|e| {
         DataFusionError::Internal(format!("Failed to parse iceberg schema JSON: {e}"))
     })?;
+    // iceberg-java writes no column for a top-level `unknown` field.

Review Comment:
   thanks for the review! added the test in `CometIcebergWriteDetectionSuite.scala` L301



-- 
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: write top-level Iceberg unknown columns natively [datafusion-comet]

Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #7100:
URL: https://github.com/apache/datafusion-comet/pull/7100#discussion_r4417472804


##########
spark/src/test/scala/org/apache/comet/CometIcebergWriteDetectionSuite.scala:
##########
@@ -277,24 +277,50 @@ class CometIcebergWriteDetectionSuite extends CometTestBase with CometIcebergTes
     }
   }
 
-  test("fall-back: unknown column in the write schema") {
+  test("fall-back: unknown field nested in a struct") {
     assume(icebergVersionAtLeast(1, 10), "The unknown type requires Iceberg 1.10+")
     withDetectionCatalog { _ =>
-      // Spark DDL cannot declare `unknown` (Iceberg plans it as Spark's NullType), so evolve the
-      // schema through the Iceberg API.
+      // Spark DDL cannot declare `unknown`, so add it through the Iceberg API.
       createTable("unknown_col", partitionSpec = "", properties = Some("'format-version'='3'"))
+      spark.sql(s"ALTER TABLE $catalog.$ns.unknown_col ADD COLUMN s STRUCT<a: INT>")
       addIcebergColumn(
         loadIcebergTable(spark, catalog, ns, "unknown_col"),
         "u",
-        icebergUnknownType())
+        icebergUnknownType(),
+        parent = Some("s"))
       spark.sql(s"REFRESH TABLE $catalog.$ns.unknown_col")
       val writeExec = captureWriteExec("unknown_col", allowWriteFailure = true) {
-        spark.sql(s"INSERT INTO $catalog.$ns.unknown_col VALUES (1, 'us', 1.0, NULL)")
+        spark.sql(
+          s"INSERT INTO $catalog.$ns.unknown_col " +
+            "VALUES (1, 'us', 1.0, named_struct('a', 1, 'u', NULL))")
       }
       assertUnsupportedContains(writeExec, "unknown_col", "column u has Iceberg type unknown")
     }
   }
 
+  test("fall-back: every column is unknown") {

Review Comment:
   `requireNoUnknownPartitionSource` returns `partition field $name is sourced from a column of Iceberg type unknown`, but the only test for it is the Action test, which checks that native does not engage. I could not find a test that checks that reason. Could a detection case next to this one assert it, as the nested and all-`unknown` cases do, so a rule that stops returning it fails?



##########
spark/src/main/scala/org/apache/comet/serde/operator/CometIcebergNativeWrite.scala:
##########
@@ -553,6 +563,24 @@ object CometIcebergNativeWrite extends CometOperatorSerde[IcebergWriteExec] {
         }
     }
 
+  // Top-level `unknown` columns are pruned from the native schema, so a partition field sourced
+  // from one would fail the native partition calculator with `Field not found`.
+  private val requireNoUnknownPartitionSource: TriggerRule = ctx =>

Review Comment:
   This repeats `requireNoVoidFieldWithDroppedSource` above: the same spec resolution, `None` reason and `try/catch`, with only the field check and the reason text differing. #7198 adds a third spec-resolving rule. Could a small helper take the field check as a function?



##########
docs/source/contributor-guide/iceberg-writes.md:
##########
@@ -276,7 +276,9 @@ Points where Comet adapts iceberg-rust to match iceberg-java:
   held rows then go through `RowPacer`, so the roll grid does not move.
 - **Field ids and casting.** `decorate_batch_with_field_ids` casts each batch to the
   field-id-annotated Arrow schema derived from the Iceberg schema, with `safe: false`, so a type
-  mismatch fails the task instead of writing NULLs.
+  mismatch fails the task instead of writing NULLs. It first drops top-level `unknown` columns,
+  whose fields `parse_iceberg_schema` prunes, so the gate declines a partition field sourced from
+  one.

Review Comment:
   The "so" does not follow: the gate declines such a partition field because the partition calculator resolves its source by field id in the pruned schema and would fail with `Field not found`. Could this say that `parse_iceberg_schema` prunes the fields and the calculator therefore cannot resolve a pruned source?



-- 
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: write top-level Iceberg unknown columns natively [datafusion-comet]

Posted by "sam-1112 (via GitHub) via github" <gi...@datafusion.apache.org>.
sam-1112 commented on PR #7100:
URL: https://github.com/apache/datafusion-comet/pull/7100#issuecomment-6555349678

   Hi @yunchipang. For the writer overlap, whichever PR lands second can integrate the top-level unknown/`Null` filtering into `UuidAlignment::convert` and replace #7198’s fallback test with a UUID + unknown native-write parity test. Does that work for you?


-- 
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: write top-level Iceberg unknown columns natively [datafusion-comet]

Posted by "yunchipang (via GitHub)" <gi...@apache.org>.
yunchipang commented on code in PR #7100:
URL: https://github.com/apache/datafusion-comet/pull/7100#discussion_r4319239552


##########
native/core/src/execution/operators/iceberg_write.rs:
##########
@@ -1488,6 +1488,22 @@ fn parse_iceberg_schema(json: &str) -> DFResult<IcebergSchemaRef> {
     let schema: IcebergSchema = serde_json::from_str(json).map_err(|e| {
         DataFusionError::Internal(format!("Failed to parse iceberg schema JSON: {e}"))
     })?;
+    // iceberg-java writes no column for a top-level `unknown` field.
+    let fields = schema.as_struct().fields();
+    let kept: Vec<_> = fields
+        .iter()
+        .filter(|f| *f.field_type != Type::Primitive(PrimitiveType::Unknown))

Review Comment:
   thanks for catching this! fixed in the latest commit: the gate now falls back to `iceberg-java` when a partition field is sourced from a top-level `unknown` column, added a regression test (it fails with `Field not found` without the fix), and noted the fallback in the user and contributor guides. could you take another look when you have time? thanks!



-- 
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: write top-level Iceberg unknown columns natively [datafusion-comet]

Posted by "yunchipang (via GitHub) via github" <gi...@datafusion.apache.org>.
yunchipang commented on code in PR #7100:
URL: https://github.com/apache/datafusion-comet/pull/7100#discussion_r4429540587


##########
spark/src/main/scala/org/apache/comet/serde/operator/CometIcebergNativeWrite.scala:
##########
@@ -553,6 +563,24 @@ object CometIcebergNativeWrite extends CometOperatorSerde[IcebergWriteExec] {
         }
     }
 
+  // Top-level `unknown` columns are pruned from the native schema, so a partition field sourced
+  // from one would fail the native partition calculator with `Field not found`.
+  private val requireNoUnknownPartitionSource: TriggerRule = ctx =>

Review Comment:
   good idea! done: `requireOutputPartitionSpec(check)` resolves the spec and catches reflection errors; each rule passes only its check. If #7198 lands first, I'll widen `check` to take `ctx`.



-- 
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: write top-level Iceberg unknown columns natively [datafusion-comet]

Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #7100:
URL: https://github.com/apache/datafusion-comet/pull/7100#issuecomment-6253001440

   Heads up: #7090 merged the Parquet and Iceberg user docs into `docs/source/user-guide/latest/parquet.md` and `iceberg.md`, and deleted `iceberg-writes.md`. This PR edits the deleted file, which is part of why it conflicts. Could you rebase and re-apply that change to the Writes section of `iceberg.md`? The contributor-guide `iceberg-writes.md` is unchanged.


-- 
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: write top-level Iceberg unknown columns natively [datafusion-comet]

Posted by "yunchipang (via GitHub)" <gi...@apache.org>.
yunchipang commented on code in PR #7100:
URL: https://github.com/apache/datafusion-comet/pull/7100#discussion_r4307224555


##########
spark/src/main/scala/org/apache/comet/serde/operator/CometIcebergNativeWrite.scala:
##########
@@ -524,6 +524,9 @@ object CometIcebergNativeWrite extends CometOperatorSerde[IcebergWriteExec] {
       case Some(schema) =>
         IcebergReflection
           .findFieldWithTypeIds(schema, UnsupportedWriteTypeIds)
+          // Nested `unknown` fails in Comet shuffle; top-level is fine.

Review Comment:
   fixed. thanks!



-- 
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: write top-level Iceberg unknown columns natively [datafusion-comet]

Posted by "yunchipang (via GitHub)" <gi...@apache.org>.
yunchipang commented on code in PR #7100:
URL: https://github.com/apache/datafusion-comet/pull/7100#discussion_r4307261142


##########
spark/src/test/scala/org/apache/comet/CometIcebergWriteActionSuite.scala:
##########
@@ -1524,6 +1524,48 @@ class CometIcebergWriteActionSuite
     }
   }
 
+  test("native acceleration: a top-level unknown column matches iceberg-java") {

Review Comment:
   added!



-- 
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: write top-level Iceberg unknown columns natively [datafusion-comet]

Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #7100:
URL: https://github.com/apache/datafusion-comet/pull/7100#discussion_r4315153683


##########
native/core/src/execution/operators/iceberg_write.rs:
##########
@@ -1488,6 +1488,22 @@ fn parse_iceberg_schema(json: &str) -> DFResult<IcebergSchemaRef> {
     let schema: IcebergSchema = serde_json::from_str(json).map_err(|e| {
         DataFusionError::Internal(format!("Failed to parse iceberg schema JSON: {e}"))
     })?;
+    // iceberg-java writes no column for a top-level `unknown` field.
+    let fields = schema.as_struct().fields();
+    let kept: Vec<_> = fields
+        .iter()
+        .filter(|f| *f.field_type != Type::Primitive(PrimitiveType::Unknown))

Review Comment:
   [P2] Preserve or decline `unknown` partition sources before pruning them. Iceberg permits a v3 schema `(id INT, u unknown)` with an identity partition on `u`. With native writes enabled and a native child, the widened gate admits this schema, but this filter removes `u` from the schema subsequently passed to `PartitionValueCalculator::try_new`. Its field-ID projector then fails with `Field not found`, aborting an insert that previously fell back successfully to iceberg-java. Spark writes the rows under `u=null`. Could the gate decline specs referencing a pruned field, or could partition calculation retain the logical source while pruning only the Parquet input? Please include a regression partitioned on `u` itself.
   
   Evidence: Created a v3 table using `new Schema(optional(1, "id", IntegerType.get()), optional(2, "u", UnknownType.get()))` and `PartitionSpec.builderFor(schema).identity("u").build()`, with `write.distribution-mode=none` and `write.spark.fanout.enabled=true`. Stock Spark 4.1.3/Iceberg 1.11 completed `INSERT INTO review.default.t VALUES (1, NULL), (2, NULL)`, returned `[[1,null], [2,null]]`, and produced files under `u=null/`. Java API probes also accepted the spec on Iceberg 1.10. A Rust probe compiled against this checkout's dependencies used the exact `parse_iceberg_schema` implementation and included the actual `iceberg_partition_value.rs`: calculator construction returned `Ok(())` with the original schema and `Err(Unexpected => Field not found; field_id: 2)` after pruning. `run_write_task` passes that pruned schema into the same calculator through `PartitionSplitter`. Before this PR, `UNKNOWN` in `UnsupportedWriteTypeIds` kept this write on iceberg-java.



-- 
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: write top-level Iceberg unknown columns natively [datafusion-comet]

Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #7100:
URL: https://github.com/apache/datafusion-comet/pull/7100#discussion_r4306516988


##########
spark/src/main/scala/org/apache/comet/serde/operator/CometIcebergNativeWrite.scala:
##########
@@ -524,6 +524,9 @@ object CometIcebergNativeWrite extends CometOperatorSerde[IcebergWriteExec] {
       case Some(schema) =>
         IcebergReflection
           .findFieldWithTypeIds(schema, UnsupportedWriteTypeIds)
+          // Nested `unknown` fails in Comet shuffle; top-level is fine.

Review Comment:
   This comment says nested `unknown` is declined because Comet shuffle panics. That panic is fixed in #7099. But `parse_iceberg_schema` only prunes top-level fields, so a nested `unknown` would still reach the Parquet writer as a `Null` field and get a column that iceberg-java doesn't write. Could the comment say that instead, so the rule doesn't get lifted once #7099 merges?



##########
native/core/src/execution/operators/iceberg_write.rs:
##########
@@ -1488,6 +1488,22 @@ fn parse_iceberg_schema(json: &str) -> DFResult<IcebergSchemaRef> {
     let schema: IcebergSchema = serde_json::from_str(json).map_err(|e| {
         DataFusionError::Internal(format!("Failed to parse iceberg schema JSON: {e}"))
     })?;
+    // iceberg-java writes no column for a top-level `unknown` field.

Review Comment:
   What happens when every top-level column is `unknown`? `kept` is empty, and I think parquet-mr refuses to write an empty schema, so iceberg-java would fail the write. Could you add a test for that table shape? If iceberg-java does fail, declining it in the gate would keep us from writing a zero-column file it can't produce.



##########
spark/src/test/scala/org/apache/comet/CometIcebergWriteActionSuite.scala:
##########
@@ -1524,6 +1524,48 @@ class CometIcebergWriteActionSuite
     }
   }
 
+  test("native acceleration: a top-level unknown column matches iceberg-java") {

Review Comment:
   Could you add a partitioned variant, partitioned on `tail`, so the partition source sits after `u`? Dropping `u` shifts every later column, and that's the case most likely to break if the field-id projection ever changes.



-- 
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: write top-level Iceberg unknown columns natively [datafusion-comet]

Posted by "yunchipang (via GitHub) via github" <gi...@datafusion.apache.org>.
yunchipang commented on code in PR #7100:
URL: https://github.com/apache/datafusion-comet/pull/7100#discussion_r4429609351


##########
spark/src/test/scala/org/apache/comet/CometIcebergWriteDetectionSuite.scala:
##########
@@ -277,24 +277,50 @@ class CometIcebergWriteDetectionSuite extends CometTestBase with CometIcebergTes
     }
   }
 
-  test("fall-back: unknown column in the write schema") {
+  test("fall-back: unknown field nested in a struct") {
     assume(icebergVersionAtLeast(1, 10), "The unknown type requires Iceberg 1.10+")
     withDetectionCatalog { _ =>
-      // Spark DDL cannot declare `unknown` (Iceberg plans it as Spark's NullType), so evolve the
-      // schema through the Iceberg API.
+      // Spark DDL cannot declare `unknown`, so add it through the Iceberg API.
       createTable("unknown_col", partitionSpec = "", properties = Some("'format-version'='3'"))
+      spark.sql(s"ALTER TABLE $catalog.$ns.unknown_col ADD COLUMN s STRUCT<a: INT>")
       addIcebergColumn(
         loadIcebergTable(spark, catalog, ns, "unknown_col"),
         "u",
-        icebergUnknownType())
+        icebergUnknownType(),
+        parent = Some("s"))
       spark.sql(s"REFRESH TABLE $catalog.$ns.unknown_col")
       val writeExec = captureWriteExec("unknown_col", allowWriteFailure = true) {
-        spark.sql(s"INSERT INTO $catalog.$ns.unknown_col VALUES (1, 'us', 1.0, NULL)")
+        spark.sql(
+          s"INSERT INTO $catalog.$ns.unknown_col " +
+            "VALUES (1, 'us', 1.0, named_struct('a', 1, 'u', NULL))")
       }
       assertUnsupportedContains(writeExec, "unknown_col", "column u has Iceberg type unknown")
     }
   }
 
+  test("fall-back: every column is unknown") {

Review Comment:
   added `"fall-back: partition field sourced from an unknown column"` detection case that asserts this reason.



-- 
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: write top-level Iceberg unknown columns natively [datafusion-comet]

Posted by "yunchipang (via GitHub) via github" <gi...@datafusion.apache.org>.
yunchipang commented on code in PR #7100:
URL: https://github.com/apache/datafusion-comet/pull/7100#discussion_r4429642987


##########
docs/source/contributor-guide/iceberg-writes.md:
##########
@@ -276,7 +276,9 @@ Points where Comet adapts iceberg-rust to match iceberg-java:
   held rows then go through `RowPacer`, so the roll grid does not move.
 - **Field ids and casting.** `decorate_batch_with_field_ids` casts each batch to the
   field-id-annotated Arrow schema derived from the Iceberg schema, with `safe: false`, so a type
-  mismatch fails the task instead of writing NULLs.
+  mismatch fails the task instead of writing NULLs. It first drops top-level `unknown` columns,
+  whose fields `parse_iceberg_schema` prunes, so the gate declines a partition field sourced from
+  one.

Review Comment:
   updated. please lmk if it's making more sense now. thanks!



-- 
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: write top-level Iceberg unknown columns natively [datafusion-comet]

Posted by "sam-1112 (via GitHub) via github" <gi...@datafusion.apache.org>.
sam-1112 commented on code in PR #7100:
URL: https://github.com/apache/datafusion-comet/pull/7100#discussion_r4434871309


##########
spark/src/main/scala/org/apache/comet/serde/operator/CometIcebergNativeWrite.scala:
##########
@@ -553,6 +563,24 @@ object CometIcebergNativeWrite extends CometOperatorSerde[IcebergWriteExec] {
         }
     }
 
+  // Top-level `unknown` columns are pruned from the native schema, so a partition field sourced
+  // from one would fail the native partition calculator with `Field not found`.
+  private val requireNoUnknownPartitionSource: TriggerRule = ctx =>

Review Comment:
   Thanks, @yunchipang! If #7100 lands first, I’ll adapt the UUID check to the shared helper when rebasing #7198.
   



-- 
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