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