Fix the edge case when handling non numeric values of double type in delta stats #526
Conversation
|
Hi, Can I have your help to rerun the Ci build pipeline. I couldn't replay the same error locally: |
xtable-core/src/main/java/org/apache/xtable/delta/DeltaValueConverter.java
Outdated
Show resolved
Hide resolved
| @@ -82,6 +82,15 @@ void parseWrongDateTime() throws ParseException { | |||
| assertThrows(ParseException.class, () -> strictDateFormat.parse(wrongDateTime)); | |||
| } | |||
|
|
|||
| @ParameterizedTest | |||
| @MethodSource("nonNumericValuesForColStats") | |||
| public void formattedDifferentNonNumericValuesFromDeltaColumnStat( | |||
There was a problem hiding this comment.
nitpick: public can be removed
There was a problem hiding this comment.
For info, I use public by following these tests: https://github.com/apache/incubator-xtable/blob/main/xtable-core/src/test/java/org/apache/xtable/delta/TestDeltaValueConverter.java#L44
but happy to remove all of them
There was a problem hiding this comment.
We have a linter that flags this stuff which is why I selfishly flagged this for my own internal cherry-pick later :) I am fine with just doing this on new tests for now. Maybe later we can integrate a linter into the the ASF toolkit as well
xtable-core/src/test/java/org/apache/xtable/delta/TestDeltaValueConverter.java
Show resolved
Hide resolved
@emilie-wang I re-ran it for you and everything is passing. Just had some minor comments and questions. Thank you for the quick fix! |
|
@emilie-wang one final request is for you to squash down to a single commit for me so I can approve and merge this into the main branch. Thanks again for the contribution! |
|
Hi @the-other-tim-brown, I am back from vacation and just updated my PR based on your comments. Thank you for your review! |
@emilie-wang hope you had a good vacation! Can you squash this all down to 1 commit for me? I've used a command like |
…delta stats When reading the delta snapshot and load the information into Delta object AddFile, the non-numeric values of float or double type (example, "NaN", "-Infinity") from col stats become string type. These special values need special handling and see how delta handled: https://github.com/delta-io/delta/blob/master/kernel/kernel-defaults/src/main/java/io/delta/kernel/defaults/internal/data/DefaultJsonRow.java#L210
emilie-wang
force-pushed
the
fix-non-numeric-value-issue
branch
from
72696e7
to
71208b2
Compare
|
@the-other-tim-brown sorry I forgot this mesage and just squashed down to 1 single commit. Thank you for the quick review again. |
Important Read
What is the purpose of the pull request
The pr aims to fix: #524.
When reading the delta snapshot and load the information into Delta object AddFile, the non-numeric values of float or double type (example, "NaN", "-Infinity") from col stats become string type. These special values need special handling and this pr used the same idea how delta new API handled this: https://github.com/delta-io/delta/blob/master/kernel/kernel-defaults/src/main/java/io/delta/kernel/defaults/internal/data/DefaultJsonRow.java#L210
Brief change log
Verify this pull request
This change added tests and can be verified as follows: