ORC-1796: [C++] fix return wrong result if lack of hasnull #2055

shuai-xu · 2024-10-25T09:37:15Z

What changes were proposed in this pull request?

This pr fix the bug that if the column statistics in a orc file is not fully written, and lack of hasnull field, user may get a wrong result using c++ to read it.
For example, a file struct<string col1, string col2>, has 10 lines, col1 all has value, col2 all is null. the column 1's stat written by trino may be
numberOfValues: 10
stringStatistics {
minimum: "10"
maximum: "100"
sum: 565
}. col2's stat is numberOfValues: 0. They all have no hasnull field. When we want to get where col2 is null, we will get nothing.

Why are the changes needed?

User may get a wrong result with this bug.

How was this patch tested?

Add unit tests.

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

No

wgtmac · 2024-10-27T03:46:08Z

Thanks for the fix! It makes sense to me. Please make the CI happy.

BTW, has Trino fixed this issue on its writer side?

shuai-xu · 2024-10-28T03:36:19Z

Thanks for the fix! It makes sense to me. Please make the CI happy.

BTW, has Trino fixed this issue on its writer side?

The ci is fixed, it seems need approval to rerun. We are trying to fix it on Trino.

wgtmac · 2024-10-28T05:52:24Z

Please fix the format check. Thanks!

shuai-xu · 2024-10-28T08:18:02Z

Please fix the format check. Thanks!

Done, CI passed.

wgtmac

Thanks! LGTM

dongjoon-hyun

+1, LGTM.

dongjoon-hyun · 2024-10-28T17:07:36Z

What is the target version for this fix, @shuai-xu and @wgtmac ?

wgtmac · 2024-10-29T01:40:31Z

I think we can backport this to 1.9 and 1.8?

### What changes were proposed in this pull request? This pr fix the bug that if the column statistics in a orc file is not fully written, and lack of hasnull field, user may get a wrong result using c++ to read it. For example, a file struct<string col1, string col2>, has 10 lines, col1 all has value, col2 all is null. the column 1's stat written by trino may be numberOfValues: 10 stringStatistics { minimum: "10" maximum: "100" sum: 565 }. col2's stat is numberOfValues: 0. They all have no hasnull field. When we want to get where col2 is null, we will get nothing. ### Why are the changes needed? User may get a wrong result with this bug. ### How was this patch tested? Add unit tests. ### Was this patch authored or co-authored using generative AI tooling? No Closes #2055 from shuai-xu/2054. Authored-by: shuai-xu <xushuai@bigo.sg> Signed-off-by: Gang Wu <ustcwg@gmail.com> (cherry picked from commit e492bef) Signed-off-by: Gang Wu <ustcwg@gmail.com>

wgtmac · 2024-10-29T09:28:14Z

I just merged it and backported it to branch-2.0. The backport to branch-1.9 failed due to our code style change since 2.0.

dongjoon-hyun · 2024-10-29T14:39:56Z

+1 for backporting decision. Thank you!

dongjoon-hyun · 2024-10-29T14:42:31Z

Ur, @wgtmac and @shuai-xu , the JIRA ID looks wrong to me. Where this ID comes from?

wgtmac · 2024-10-29T15:42:47Z

Sorry that I didn't check the JIRA id in title. :(

shuai-xu · 2024-10-30T01:08:26Z

@dongjoon-hyun sorry, I open the issue on github #2054, need I open one on jira?

dongjoon-hyun · 2024-10-31T01:02:56Z

Since we didn't make any releases or tags yet, it's recoverable if we do reset those branches, master and branch-2.0. Although it could affect the downstream, do you mind resetting them, @wgtmac and @shuai-xu ?

dongjoon-hyun · 2024-10-31T01:04:28Z

BTW, I also missed this. So, you don't need to feel sorry about this. We can recover this in a correct way.

This pr fix the bug that if the column statistics in a orc file is not fully written, and lack of hasnull field, user may get a wrong result using c++ to read it. For example, a file struct<string col1, string col2>, has 10 lines, col1 all has value, col2 all is null. the column 1's stat written by trino may be numberOfValues: 10 stringStatistics { minimum: "10" maximum: "100" sum: 565 }. col2's stat is numberOfValues: 0. They all have no hasnull field. When we want to get where col2 is null, we will get nothing. User may get a wrong result with this bug. Add unit tests. No Closes #2055 from shuai-xu/2054. Authored-by: shuai-xu <xushuai@bigo.sg> Signed-off-by: Gang Wu <ustcwg@gmail.com>

This pr fix the bug that if the column statistics in a orc file is not fully written, and lack of hasnull field, user may get a wrong result using c++ to read it. For example, a file struct<string col1, string col2>, has 10 lines, col1 all has value, col2 all is null. the column 1's stat written by trino may be numberOfValues: 10 stringStatistics { minimum: "10" maximum: "100" sum: 565 }. col2's stat is numberOfValues: 0. They all have no hasnull field. When we want to get where col2 is null, we will get nothing. User may get a wrong result with this bug. Add unit tests. No Closes #2055 from shuai-xu/2054. Authored-by: shuai-xu <xushuai@bigo.sg> Signed-off-by: Gang Wu <ustcwg@gmail.com> (cherry picked from commit e492bef) Signed-off-by: Gang Wu <ustcwg@gmail.com>

wgtmac · 2024-10-31T01:24:26Z

I have created https://issues.apache.org/jira/browse/ORC-1796 and fix the commits in both main and branch-2.0 branches.

### Rationale for this change This PR aims to upgrade ORC to 2.0.3 to bring the following bug fixes. - https://orc.apache.org/news/2024/08/15/ORC-2.0.2/ - apache/orc#1963 - apache/orc#1997 - apache/orc#1981 - https://orc.apache.org/news/2024/11/14/ORC-2.0.3/ - apache/orc#2055 ### What changes are included in this PR? To use the latest bug fixed version. ### Are these changes tested? This should pass the CIs. ### Are there any user-facing changes? No. * GitHub Issue: #44744 Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

### 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: ```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()); } ``` ### 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](https://github.com/apache/doris) external pipeline: apache/doris#45104 apache/doris-thirdparty#259 ### Was this patch authored or co-authored using generative AI tooling? No Closes #2082 from suxiaogang223/fix_has_null. Authored-by: Socrates <suxiaogang223@icloud.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

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>

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>

ORC-2054: [C++] fix return wrong result if lack of hasnull

78700f3

github-actions bot added the CPP label Oct 25, 2024

shuai-xu added 2 commits October 28, 2024 10:16

fix build break

082cd8b

fix build break

af88c40

fix code style

bf7c8d3

wgtmac approved these changes Oct 28, 2024

View reviewed changes

dongjoon-hyun approved these changes Oct 28, 2024

View reviewed changes

wgtmac closed this in e492bef Oct 29, 2024

dongjoon-hyun added this to the 2.0.3 milestone Oct 29, 2024

wgtmac changed the title ~~ORC-2054: [C++] fix return wrong result if lack of hasnull~~ ORC-1796: [C++] fix return wrong result if lack of hasnull Oct 31, 2024

wgtmac mentioned this pull request Oct 31, 2024

Reading orc file which lack of Statistics may give wrong result using C++ #2054

Closed

This was referenced Nov 15, 2024

[C++] Upgrade ORC to 2.0.3 apache/arrow#44744

Closed

GH-44744: [C++] Upgrade ORC to 2.0.3 apache/arrow#44745

Merged

ffacs mentioned this pull request Dec 10, 2024

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

Closed

suxiaogang223 mentioned this pull request Dec 10, 2024

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

Closed

suxiaogang223 mentioned this pull request Dec 16, 2024

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ORC-1796: [C++] fix return wrong result if lack of hasnull #2055

ORC-1796: [C++] fix return wrong result if lack of hasnull #2055

shuai-xu commented Oct 25, 2024

wgtmac commented Oct 27, 2024 •

edited

Loading

shuai-xu commented Oct 28, 2024

wgtmac commented Oct 28, 2024

shuai-xu commented Oct 28, 2024

wgtmac left a comment

dongjoon-hyun left a comment

dongjoon-hyun commented Oct 28, 2024

wgtmac commented Oct 29, 2024

wgtmac commented Oct 29, 2024

dongjoon-hyun commented Oct 29, 2024

dongjoon-hyun commented Oct 29, 2024

wgtmac commented Oct 29, 2024

shuai-xu commented Oct 30, 2024

dongjoon-hyun commented Oct 31, 2024 •

edited

Loading

dongjoon-hyun commented Oct 31, 2024

wgtmac commented Oct 31, 2024

ORC-1796: [C++] fix return wrong result if lack of hasnull #2055

ORC-1796: [C++] fix return wrong result if lack of hasnull #2055

Conversation

shuai-xu commented Oct 25, 2024

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?

wgtmac commented Oct 27, 2024 • edited Loading

shuai-xu commented Oct 28, 2024

wgtmac commented Oct 28, 2024

shuai-xu commented Oct 28, 2024

wgtmac left a comment

Choose a reason for hiding this comment

dongjoon-hyun left a comment

Choose a reason for hiding this comment

dongjoon-hyun commented Oct 28, 2024

wgtmac commented Oct 29, 2024

wgtmac commented Oct 29, 2024

dongjoon-hyun commented Oct 29, 2024

dongjoon-hyun commented Oct 29, 2024

wgtmac commented Oct 29, 2024

shuai-xu commented Oct 30, 2024

dongjoon-hyun commented Oct 31, 2024 • edited Loading

dongjoon-hyun commented Oct 31, 2024

wgtmac commented Oct 31, 2024

wgtmac commented Oct 27, 2024 •

edited

Loading

dongjoon-hyun commented Oct 31, 2024 •

edited

Loading