joyhaldar commented on code in PR #16638:
URL: https://github.com/apache/iceberg/pull/16638#discussion_r3368944776
##########
data/src/test/java/org/apache/iceberg/data/DataGenerators.java:
##########
@@ -89,4 +95,67 @@ public Schema schema() {
return schema;
}
}
+
+ // Generator for reader default-value tests across primitive types. TIME and
FIXED are left out
+ // because they fail on the engine read path, not because of default-value
handling.
+ // TODO: include TIME once the engine readers support it.
+ // TODO: include FIXED once Spark supports it.
Review Comment:
I was looking into the `FIXED` `ClassCastException`, the
[TCK's](https://github.com/apache/iceberg/pull/15441)
[InternalRowConverter](https://github.com/apache/iceberg/blob/main/spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/data/InternalRowConverter.java)
assumed a `FIXED` value is always a `ByteBuffer` and cast it directly. But a
generic Record actually holds `FIXED` as a `byte[]` IIUC, so casting `byte[]`
to `ByteBuffer` throws the exception. It never showed up before because the TCK
had no `FIXED` column, so that branch never ran on a real `FIXED` value, adding
one as part of this PR is what surfaced it.
The fix checks the value type, handling `byte[]`, `ByteBuffer`, and
`GenericData.Fixed`, all yielding `byte[]`, which is what Spark's `InternalRow`
expects IIUC, mirroring what
[TestHelpers](https://github.com/apache/iceberg/blob/main/spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/data/TestHelpers.java)
does. I have tested it on `Spark 4.1/4.0/3.5`.
Should I keep
[this](https://github.com/apache/iceberg/pull/16638/commits/507ab461a3122a327c7addf1ecaa0ef423a33146)
here and drop the `TODO`, or put it into a seperate PR and keep the `TODO` in
this one?
--
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]
