ORC-1813: [C++] Fix has_null forward compatibility#2086
Closed
suxiaogang223 wants to merge 1 commit intoapache:branch-2.0from
Closed
ORC-1813: [C++] Fix has_null forward compatibility#2086suxiaogang223 wants to merge 1 commit intoapache:branch-2.0from
suxiaogang223 wants to merge 1 commit intoapache:branch-2.0from
Conversation
close: apache#2079 relate pr: apache#2055 Introduce fallback logic in the C++ reader to set hasNull to true when the field is missing, similar to the Java implementation. The Java implementation includes the following logic: ```java if (stats.hasHasNull()) { hasNull = stats.getHasNull(); } else { hasNull = true; } ``` In contrast, the C++ implementation directly uses the has_null value without any fallback logic: ```c++ ColumnStatisticsImpl::ColumnStatisticsImpl(const proto::ColumnStatistics& pb) { stats_.setNumberOfValues(pb.number_of_values()); stats_.setHasNull(pb.has_null()); } ``` We encountered an issue with the C++ implementation of the ORC reader when handling ORC files written with version 0.12. Specifically, files written in this version do not include the hasNull field in the column statistics metadata. While the Java implementation of the ORC reader handles this gracefully by defaulting hasNull to true when the field is absent, the C++ implementation does not handle this scenario correctly. **This issue prevents predicates like IS NULL from being pushed down to the ORC reader!!! As a result, all rows in the file are filtered out, leading to incorrect query results :(** I have tested this using [Doris](https://github.com/apache/doris) external pipeline: apache/doris#45104 apache/doris-thirdparty#259 No Closes apache#2082 from suxiaogang223/fix_has_null. Authored-by: Socrates <suxiaogang223@icloud.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
wgtmac
approved these changes
Dec 16, 2024
Member
|
Thank you for this backporting, @suxiaogang223 and @wgtmac . |
dongjoon-hyun
pushed a commit
that referenced
this pull request
Dec 20, 2024
close: #2079 relate pr: #2055 Introduce fallback logic in the C++ reader to set hasNull to true when the field is missing, similar to the Java implementation. The Java implementation includes the following logic: ```java if (stats.hasHasNull()) { hasNull = stats.getHasNull(); } else { hasNull = true; } ``` In contrast, the C++ implementation directly uses the has_null value without any fallback logic: ```c++ ColumnStatisticsImpl::ColumnStatisticsImpl(const proto::ColumnStatistics& pb) { stats_.setNumberOfValues(pb.number_of_values()); stats_.setHasNull(pb.has_null()); } ``` We encountered an issue with the C++ implementation of the ORC reader when handling ORC files written with version 0.12. Specifically, files written in this version do not include the hasNull field in the column statistics metadata. While the Java implementation of the ORC reader handles this gracefully by defaulting hasNull to true when the field is absent, the C++ implementation does not handle this scenario correctly. **This issue prevents predicates like IS NULL from being pushed down to the ORC reader!!! As a result, all rows in the file are filtered out, leading to incorrect query results :(** I have tested this using [Doris](https://github.com/apache/doris) external pipeline: apache/doris#45104 apache/doris-thirdparty#259 No Closes #2082 from suxiaogang223/fix_has_null. Authored-by: Socrates <suxiaogang223icloud.com> ### What changes were proposed in this pull request? ### Why are the changes needed? ### How was this patch tested? ### Was this patch authored or co-authored using generative AI tooling? Closes #2086 from suxiaogang223/cherry_pick_fix_has_null. Authored-by: Socrates <suxiaogang223@icloud.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
close: #2079
relate pr: #2055
Introduce fallback logic in the C++ reader to set hasNull to true when the field is missing, similar to the Java implementation. The Java implementation includes the following logic:
In contrast, the C++ implementation directly uses the has_null value without any fallback logic:
We encountered an issue with the C++ implementation of the ORC reader when handling ORC files written with version 0.12. Specifically, files written in this version do not include the hasNull field in the column statistics metadata. While the Java implementation of the ORC reader handles this gracefully by defaulting hasNull to true when the field is absent, the C++ implementation does not handle this scenario correctly. This issue prevents predicates like IS NULL from being pushed down to the ORC reader!!! As a result, all rows in the file are filtered out, leading to incorrect query results :( I have tested this using Doris external pipeline: apache/doris#45104
apache/doris-thirdparty#259 No
Closes #2082 from suxiaogang223/fix_has_null.
Authored-by: Socrates suxiaogang223@icloud.com
What changes were proposed in this pull request?
Why are the changes needed?
How was this patch tested?
Was this patch authored or co-authored using generative AI tooling?