Tests: Add unit tests for InternalRecordWrapper, RowDataWrapper, InternalRowWrapper #2683
Conversation
| public <T> T get(int pos, Class<T> javaClass) { | ||
| if (transforms[pos] != null) { | ||
| return javaClass.cast(transforms[pos].apply(wrapped.get(pos, Object.class))); | ||
| Object value = wrapped.get(pos, Object.class); |
There was a problem hiding this comment.
I don't think this should change the behavior of the else case where the java class is passed to wrapped.get. For that case, the right thing to do is to delegate the operation to wrapped. The reason to use Object.class is when we have a transform function and know that the java class passed in won't work.
| Object value = wrapped.get(pos, Object.class); | ||
|
|
||
| if (value == null) { | ||
| return null; |
There was a problem hiding this comment.
Why is it incorrect to call javaClass.cast(null)?
There was a problem hiding this comment.
If we don't make this change, when running this unit tests we will encounter the exception:
temporal
java.lang.NullPointerException: temporal
at java.util.Objects.requireNonNull(Objects.java:228)
at java.time.LocalDate.from(LocalDate.java:364)
at java.time.LocalDate.until(LocalDate.java:1602)
at java.time.temporal.ChronoUnit.between(ChronoUnit.java:272)
at org.apache.iceberg.util.DateTimeUtil.daysFromDate(DateTimeUtil.java:42)
at org.apache.iceberg.data.InternalRecordWrapper.lambda$converter$2(InternalRecordWrapper.java:48)
at org.apache.iceberg.data.InternalRecordWrapper.get(InternalRecordWrapper.java:84)
at org.apache.iceberg.types.Comparators$StructLikeComparator.compare(Comparators.java:122)
at org.apache.iceberg.types.Comparators$StructLikeComparator.compare(Comparators.java:102)
at org.apache.iceberg.util.StructLikeWrapper.equals(StructLikeWrapper.java:76)
at org.junit.Assert.isEquals(Assert.java:131)
at org.junit.Assert.equalsRegardingNull(Assert.java:127)
at org.junit.Assert.assertEquals(Assert.java:111)
at org.apache.iceberg.spark.source.TestInternalRowWrapper.generateAndValidate(TestInternalRowWrapper.java:71)
at org.apache.iceberg.RecordWrapperTest.generateAndValidate(RecordWrapperTest.java:112)
at org.apache.iceberg.RecordWrapperTest.testSimpleStructWithoutTime(RecordWrapperTest.java:62)
at java.lang.Thread.run(Thread.java:748)As you said , javaClass.cast(null) is OK , but the key problem is the transforms[pos].apply(null). we have many transforms which don't allow to handle null values.
|
|
||
| int expectedMilliseconds = (int) ((long) expected / 1000_000); | ||
| int actualMilliseconds = (int) ((long) actual / 1000_000); | ||
| Assert.assertEquals(message, expectedMilliseconds, actualMilliseconds); |
There was a problem hiding this comment.
I don't think that this is valid. Millisecond truncation should never be allowed, except in Flink where it is unavoidable. In the data module, this is incorrect.
There was a problem hiding this comment.
You are correct, my. original idea was to put this method in the flink test class , but forget to move it to...
| return javaClass.cast(transforms[pos].apply(wrapped.get(pos, Object.class))); | ||
| Object value = wrapped.get(pos, Object.class); | ||
| if (value == null) { | ||
| // transforms function don't allow to handle null values, so just return null here. |
There was a problem hiding this comment.
Thanks for adding a comment to clarify why you're making this change!
2 participants
Add unit tests to address the introduced
InternalRecordWrapper,RowDataWrapper,InternalRowWrapper. The intention of writing this test is : I find that theInternalRecordWrapperdid not handle the nullable value correct ( I mean we will cast anullto the target value, that is incorrect), so I think preparing an unit tests for this is necessary.