Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ORC-1813: [C++] Fix has_null forward compatibility #2082

Closed
wants to merge 6 commits into from

Conversation

suxiaogang223
Copy link
Contributor

@suxiaogang223 suxiaogang223 commented Dec 10, 2024

What changes were proposed in this pull request?

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:

if (stats.hasHasNull()) {
    hasNull = stats.getHasNull();
} else {
    hasNull = true;
}

In contrast, the C++ implementation directly uses the has_null value without any fallback logic:

ColumnStatisticsImpl::ColumnStatisticsImpl(const proto::ColumnStatistics& pb) {
    stats_.setNumberOfValues(pb.number_of_values());
    stats_.setHasNull(pb.has_null());
}

Why are the changes needed?

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 :(

How was this patch tested?

I have tested this using Doris external pipeline:
apache/doris#45104
apache/doris-thirdparty#259

Was this patch authored or co-authored using generative AI tooling?

No

@github-actions github-actions bot added the CPP label Dec 10, 2024
@dongjoon-hyun
Copy link
Member

Thank you for making a PR, @suxiaogang223 .

@suxiaogang223
Copy link
Contributor Author

suxiaogang223 commented Dec 11, 2024

I have fix some orc c++ uts because the orc file orc_index_int_string.orc is written by old orc version which don't write the has_null statistics.

c++/src/sargs/PredicateLeaf.cc Outdated Show resolved Hide resolved
c++/src/sargs/PredicateLeaf.cc Outdated Show resolved Hide resolved
c++/src/sargs/PredicateLeaf.cc Outdated Show resolved Hide resolved
c++/src/sargs/PredicateLeaf.cc Outdated Show resolved Hide resolved
c++/test/TestPredicateLeaf.cc Outdated Show resolved Hide resolved
@wgtmac
Copy link
Member

wgtmac commented Dec 11, 2024

Thanks for fixing this! Could you create a JIRA issue? We use JIRA instead of GitHub issue for tracking purpose.

@suxiaogang223
Copy link
Contributor Author

Thanks for fixing this! Could you create a JIRA issue? We use JIRA instead of GitHub issue for tracking purpose.

How to create jira? it seems that your jira system cannot be used :(

@wgtmac
Copy link
Member

wgtmac commented Dec 11, 2024

You may request a JIRA account via: https://selfserve.apache.org/jira-account.html

It was open for registration. Unfortunately it has been disabled due to spam :(

@suxiaogang223 suxiaogang223 changed the title fix has_null forward compatibility ORC-1813: [C++] fix has_null forward compatibility Dec 11, 2024
@dongjoon-hyun
Copy link
Member

Thank you for fling JIRA and update the PR title, @suxiaogang223 .

BTW, does it affect all ORC versions like 1.9.x and 1.8.x? The JIRA issue seems to be reported on Apache ORC 2.0.x only.

@dongjoon-hyun dongjoon-hyun added this to the 2.0.4 milestone Dec 11, 2024
@dongjoon-hyun
Copy link
Member

For now, I set the milestone, 2.0.4, based on Affected Versions of ORC-1813.

fix
Copy link
Contributor

@ffacs ffacs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wgtmac wgtmac changed the title ORC-1813: [C++] fix has_null forward compatibility ORC-1813: [C++] Fix has_null forward compatibility Dec 12, 2024
Copy link
Member

@wgtmac wgtmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@dongjoon-hyun
Copy link
Member

Thank you all, @suxiaogang223 , @wgtmac , @ffacs !

Merged to main.

Could you make a backporting PR to branch-2.0? There was a conflict.

@suxiaogang223 suxiaogang223 deleted the fix_has_null branch December 12, 2024 15:57
suxiaogang223 added a commit to suxiaogang223/orc that referenced this pull request Dec 16, 2024
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>
@suxiaogang223
Copy link
Contributor Author

Thank you all, @suxiaogang223 , @wgtmac , @ffacs !

Merged to main.

Could you make a backporting PR to branch-2.0? There was a conflict.

There is a conflict fixed pr : #2086

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>
@dongjoon-hyun dongjoon-hyun modified the milestones: 2.0.4, 2.1.0 Dec 20, 2024
@dongjoon-hyun
Copy link
Member

For the record, the following landed to branch-2.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incompatibility with ORC files written in version 0.12 due to missing hasNull field in C++ Reader
4 participants