GH-43712: [C++][Parquet] Dataset: Handle num-nulls in Parquet correctly when !HasNullCount() #43726

mapleFU · 2024-08-16T10:23:02Z

Rationale for this change

See issue. When !HasNullCount, we cannot gurantee null exists

What changes are included in this PR?

Handle HasNullCount in dataset expr

Are these changes tested?

Yes

Are there any user-facing changes?

Merely

GitHub Issue: [C++][Parquet] Dataset: ParquetFileFragment::EvaluateStatisticsAsExpression should better checks Statistics::HasNullCount #43712

github-actions · 2024-08-16T10:23:28Z

⚠️ GitHub issue #43712 has been automatically assigned in GitHub to PR creator.

cpp/src/arrow/dataset/file_parquet.cc

mapleFU · 2024-08-22T10:57:41Z

@pitrou @bkietz Would you mind take a look? I've add test in this case now

pitrou

Sorry for the delay @mapleFU .

cpp/src/arrow/dataset/file_parquet.cc

pitrou · 2024-09-03T13:17:12Z

cpp/src/arrow/dataset/file_parquet.cc

+    // If there are no values and no nulls, it might be empty or contains
+    // only null.
+    return std::nullopt;


This seems a bit weird (though it's certainly safe to return std::nullopt).
If num_values() == 0, there can be only nulls, so we can just return is_null(std::move(field_expr)) too?

I don't know. This might means "no-values", like an empty-page. I'm not sure should an empty page return is_null, it might be ok but a bit-weird for me( is_null for null or empty data)

@bkietz What do you think?

I think it's simpler and more consistent to return is_null(std::move(field_expr)) if we can; in general I think it usually makes the most sense to construct the most explicit/precise guarantees which are easily available (and therefore to avoid making this special case which will result in less specific guarantees).

cpp/src/arrow/dataset/file_parquet.cc

pitrou · 2024-09-03T13:25:20Z

cpp/src/arrow/dataset/file_parquet_test.cc

+  }
+  {
+    // Special case: when num_value is 0, if has_null, it would return
+    // "is_null", otherwise it cannot gurantees anything


I don't understand why it cannot guarantee anything. It knows that there are no non-null values, so is_null is true for all values (even if there are no values at all).

Replied in https://github.com/apache/arrow/pull/43726/files#r1742087940 . I can also change to is_null. I've no trending here

Co-authored-by: Antoine Pitrou <[email protected]>

mapleFU · 2024-09-05T12:20:31Z

@pitrou @bkietz I've change to return is_null when value count is 0

pitrou · 2024-09-05T15:00:39Z

cpp/src/arrow/dataset/file_parquet.cc

+    // If `statistics.HasNullCount()`, it means the all the values are nulls.
+    //
+    // If there are no values and no nulls, it might be empty or all values
+    // are nulls. In this case, we also return a null expression.


Can you update this comment?

Oh, it might be mistake

// If there are no values and `!statistics.HasNullCount()`, it might be // empty or all values are nulls. In this case, we also return a null // expression.

Change to this

cpp/src/arrow/dataset/file_parquet.cc

pitrou · 2024-09-05T16:01:59Z

Thanks for the fix and tests @mapleFU !

mapleFU · 2024-09-06T07:49:36Z

Enter primary author in the format of "name <email>" [mwish <[[email protected]](mailto:[email protected])>]: 
Traceback (most recent call last):
  File "/Users/mwish/workspace/CMakeLibs/arrow/dev/merge_arrow_pr.py", line 790, in <module>
    cli()
  File "/Users/mwish/workspace/CMakeLibs/arrow/dev/merge_arrow_pr.py", line 771, in cli
    pr.merge()
  File "/Users/mwish/workspace/CMakeLibs/arrow/dev/merge_arrow_pr.py", line 644, in merge
    self.cmd.fail(f'Failed to merge pull request: {message}')
  File "/Users/mwish/workspace/CMakeLibs/arrow/dev/merge_arrow_pr.py", line 458, in fail
    raise Exception(msg)
Exception: Failed to merge pull request: Not Found: https://api.github.com/repos/apache/arrow/pulls/43726/merge

@kou Have you meet this problem when merging?

kou · 2024-09-06T08:50:16Z

I haven't seen it. Could you try it again? Or can I try it?

kou · 2024-09-06T08:52:44Z

Is your GitHub Personal Access Token valid?

mapleFU · 2024-09-06T08:53:35Z

I would check this

mapleFU · 2024-09-06T13:39:32Z

Aha merged, sorry for distrubing

conbench-apache-arrow · 2024-09-06T21:22:57Z

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit ab0a40e.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 17 possible false positives for unstable benchmarks that are known to sometimes produce them.

…orrectly when !HasNullCount() (apache#43726) ### Rationale for this change See issue. When `!HasNullCount`, we cannot gurantee null exists ### What changes are included in this PR? Handle HasNullCount in dataset expr ### Are these changes tested? Yes ### Are there any user-facing changes? Merely * GitHub Issue: apache#43712 Lead-authored-by: mwish <[email protected]> Co-authored-by: mwish <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Signed-off-by: mwish <[email protected]>

Handle num-nulls in Parquet correctly

b6a4c15

github-actions bot added Component: C++ awaiting review Awaiting review labels Aug 16, 2024

mapleFU requested a review from pitrou August 16, 2024 10:23

mapleFU commented Aug 16, 2024

View reviewed changes

cpp/src/arrow/dataset/file_parquet.cc Outdated Show resolved Hide resolved

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Aug 16, 2024

mapleFU added 2 commits August 22, 2024 18:23

Merge branch 'main' into handle-num-nulls-correctly

f7929f3

add test

132363c

mapleFU requested a review from bkietz August 28, 2024 03:59

pitrou requested changes Sep 3, 2024

View reviewed changes

Update cpp/src/arrow/dataset/file_parquet.cc

49f41e2

Co-authored-by: Antoine Pitrou <[email protected]>

mapleFU requested review from assignUser, jonkeane, kou, raulcd, thisisnic, domoritz, trxcllnt, lidavidm, zeroshade, CurtHagenlocher and westonpace as code owners September 3, 2024 13:45

github-actions bot added Component: R Component: Java Component: Go Component: Python Component: JavaScript labels Sep 3, 2024

apply suggestions

caad9a1

mapleFU requested a review from pitrou September 3, 2024 14:55

fix build

0060abf

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Sep 4, 2024

mapleFU added 2 commits September 5, 2024 08:38

Merge branch 'main' into handle-num-nulls-correctly

c92d31f

apply suggestions

374b5f7

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 5, 2024

bkietz approved these changes Sep 5, 2024

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Sep 5, 2024

pitrou requested changes Sep 5, 2024

View reviewed changes

update comment

b2c5e9a

pitrou reviewed Sep 5, 2024

View reviewed changes

cpp/src/arrow/dataset/file_parquet.cc Outdated Show resolved Hide resolved

apply suggestions

daad33e

pitrou approved these changes Sep 5, 2024

View reviewed changes

mapleFU merged commit ab0a40e into apache:main Sep 6, 2024
40 of 41 checks passed

mapleFU removed the awaiting merge Awaiting merge label Sep 6, 2024

mapleFU mentioned this pull request Sep 6, 2024

[C++][Parquet] Dataset: ParquetFileFragment::EvaluateStatisticsAsExpression should better checks Statistics::HasNullCount #43712

Closed

mapleFU deleted the handle-num-nulls-correctly branch September 6, 2024 13:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-43712: [C++][Parquet] Dataset: Handle num-nulls in Parquet correctly when !HasNullCount() #43726

GH-43712: [C++][Parquet] Dataset: Handle num-nulls in Parquet correctly when !HasNullCount() #43726

mapleFU commented Aug 16, 2024 •

edited

Loading

github-actions bot commented Aug 16, 2024

mapleFU commented Aug 22, 2024 •

edited

Loading

pitrou left a comment

pitrou Sep 3, 2024

mapleFU Sep 3, 2024 •

edited

Loading

pitrou Sep 3, 2024

bkietz Sep 4, 2024

pitrou Sep 3, 2024

mapleFU Sep 3, 2024

mapleFU commented Sep 5, 2024

pitrou Sep 5, 2024

mapleFU Sep 5, 2024

mapleFU Sep 5, 2024

pitrou commented Sep 5, 2024

mapleFU commented Sep 6, 2024

kou commented Sep 6, 2024

kou commented Sep 6, 2024

mapleFU commented Sep 6, 2024

mapleFU commented Sep 6, 2024

conbench-apache-arrow bot commented Sep 6, 2024

GH-43712: [C++][Parquet] Dataset: Handle num-nulls in Parquet correctly when !HasNullCount() #43726

GH-43712: [C++][Parquet] Dataset: Handle num-nulls in Parquet correctly when !HasNullCount() #43726

Conversation

mapleFU commented Aug 16, 2024 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Aug 16, 2024

mapleFU commented Aug 22, 2024 • edited Loading

pitrou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mapleFU Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mapleFU commented Sep 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitrou commented Sep 5, 2024

mapleFU commented Sep 6, 2024

kou commented Sep 6, 2024

kou commented Sep 6, 2024

mapleFU commented Sep 6, 2024

mapleFU commented Sep 6, 2024

conbench-apache-arrow bot commented Sep 6, 2024

mapleFU commented Aug 16, 2024 •

edited

Loading

mapleFU commented Aug 22, 2024 •

edited

Loading

mapleFU Sep 3, 2024 •

edited

Loading