yyanyy commented on a change in pull request #1790:
URL: https://github.com/apache/iceberg/pull/1790#discussion_r529045452
##########
File path:
spark/src/main/java/org/apache/iceberg/spark/source/StructInternalRow.java
##########
@@ -115,12 +120,30 @@ public short getShort(int ordinal) {
@Override
public int getInt(int ordinal) {
- return struct.get(ordinal, Integer.class);
+ Object integer = struct.get(ordinal, Object.class);
Review comment:
I did a similar change in this class in an earlier pr, and I think this
change is specific to the test itself so I'm bit reluctant to move it out of
the current context, as without this context this change will be confusing to
understand:
- currently spark supports dates and time the way this class already
handles, as spark represents dates/time with internal structures of long/int
[ref1](https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/types/DateType.scala#L35-L38)
[ref2](https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/types/TimestampType.scala#L37-L40),
so the new code introduced by this won't affect the behavior of this class, as
Spark will continue to write and get these types with the same internal numeric
types. And this class is only used for loading metadata tables so performance
with the extra if-else check probably is not a big concern too.
- However in `TestMergingMetrics` itself, since data are created by random
record generator which uses the actual `LocalDate` to populate the fields, and
we are wrapping the generated records with this class for writing, we have to
handle them specially here. But I don't think this usage pattern (of writing
spark rows by wrapping them with iceberg record) will be used in production
since the spark engine will use the actual spark `InternalRow`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
