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: [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]
