ORC-1147: Use isNaN instead of isFinite to determine the contain NaN values
#1080
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dongjoon-hyun
previously requested changes
Apr 5, 2022
There was a problem hiding this comment.
Oh, @guiyanakuang . This is not correct, @guiyanakuang .
There exists multiple NaN values. Double.NaN is just one of them.
|
Please note that IEEE Standard defines NaN as range, not a single value. |
Maybe I should use the JDK's own method |
|
Thank sounds better. |
guiyanakuang
changed the title
Objects.equals(dstas.getSum(), Double.NaN) instead of isFinite to determine if there is a NaN writeisNaN instead of isFinite to determine the contain NaN values
| // First two rows of data cause sum overflow, sum is not a finite value, | ||
| // but this does not prevent pushing down (range comparisons work fine) | ||
| // The same applies to the middle stripe | ||
| fcol.vector[0] = dbcol.vector[0] = Double.MAX_VALUE / 2 + Double.MAX_VALUE / 4; |
There was a problem hiding this comment.
Please define a constant variable once for Double.MAX_VALUE / 2 + Double.MAX_VALUE / 4 and use it.
| assertEquals(1000, batch.size); | ||
|
|
||
| rows.nextBatch(batch); | ||
| // Last strip should not be read, even if sum is not finite |
|
|
||
| // First two rows of data cause sum overflow, sum is not a finite value, | ||
| // but this does not prevent pushing down (range comparisons work fine) | ||
| // The same applies to the middle stripe |
There was a problem hiding this comment.
Could you add some more illustration about how many stripes are used here?
|
cc @williamhyun |
|
|
||
| // Here we are writing 3500 rows of data, with stripeSize set to 400000 | ||
| // and rowIndexStride set to 1000, so 1 stripe will be written, | ||
| // indexed in 4 strides. |
There was a problem hiding this comment.
Thank you for the details.
dongjoon-hyun
approved these changes
Apr 6, 2022
dongjoon-hyun
pushed a commit
that referenced
this pull request
Apr 6, 2022
…values ### What changes were proposed in this pull request? This pr is aimed at using `isNaN` instead of `isFinite` to determine the contain NaN values. I want to exclude Double.POSITIVE_INFINITY / Double.NEGATIVE_INFINITY both cases, and only match NaN. ### Why are the changes needed? In the case of a sum overflow we can also predicate down to skip the corresponding strip. ### How was this patch tested? Added unit test. Closes #1080 Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit 6b053d4) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
dongjoon-hyun
pushed a commit
that referenced
this pull request
Apr 6, 2022
…values ### What changes were proposed in this pull request? This pr is aimed at using `isNaN` instead of `isFinite` to determine the contain NaN values. I want to exclude Double.POSITIVE_INFINITY / Double.NEGATIVE_INFINITY both cases, and only match NaN. ### Why are the changes needed? In the case of a sum overflow we can also predicate down to skip the corresponding strip. ### How was this patch tested? Added unit test. Closes #1080 Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit 6b053d4) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit 7763697) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
cxzl25
pushed a commit
to cxzl25/orc
that referenced
this pull request
Jan 11, 2024
…values ### What changes were proposed in this pull request? This pr is aimed at using `isNaN` instead of `isFinite` to determine the contain NaN values. I want to exclude Double.POSITIVE_INFINITY / Double.NEGATIVE_INFINITY both cases, and only match NaN. ### Why are the changes needed? In the case of a sum overflow we can also predicate down to skip the corresponding strip. ### How was this patch tested? Added unit test. Closes apache#1080 Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
This pr is aimed at using
isNaNinstead ofisFiniteto determine the contain NaN values.I want to exclude Double.POSITIVE_INFINITY / Double.NEGATIVE_INFINITY both cases, and only match NaN.
Why are the changes needed?
In the case of a sum overflow we can also predicate down to skip the corresponding strip.
How was this patch tested?
Added unit test.