You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "fenfeng9 (via GitHub)" <gi...@apache.org> on 2026/06/04 09:03:28 UTC
[PR] GH-50043: [C++][Python] Fix hash_any/hash_all on sliced boolean arrays [arrow]
fenfeng9 opened a new pull request, #50094:
URL: https://github.com/apache/arrow/pull/50094
### Rationale for this change
`hash_any` and `hash_all` could return incorrect results for sliced nullable boolean arrays.
The validity bitmap used the slice offset, but the boolean values bitmap did not.
### What changes are included in this PR?
Apply the slice offset when reading boolean values in `hash_any` / `hash_all`.
Add C++ and Python regression tests.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
No.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50043: [C++][Python] Fix hash_any/hash_all on sliced boolean arrays [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on PR #50094:
URL: https://github.com/apache/arrow/pull/50094#issuecomment-4621511134
I wonder if we have the same bug elsewhere in aggregate kernels (e.g. for different types). It's out of scope here, but do you have any input @fenfeng9 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50043: [C++][Python] Fix hash_any/hash_all on sliced boolean arrays [arrow]
Posted by "fenfeng9 (via GitHub)" <gi...@apache.org>.
fenfeng9 commented on PR #50094:
URL: https://github.com/apache/arrow/pull/50094#issuecomment-4622096634
> I wonder if we have the same bug elsewhere in aggregate kernels (e.g. for different types). It's out of scope here, but do you have any input @fenfeng9 ?
I don't see the same kind of offset issue in nearby aggregate kernels for other data types.
I'll take a closer look separately, and if I find another case, I'll file a new issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50043: [C++][Python] Fix hash_any/hash_all on sliced boolean arrays [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok merged PR #50094:
URL: https://github.com/apache/arrow/pull/50094
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50043: [C++][Python] Fix hash_any/hash_all on sliced boolean arrays [arrow]
Posted by "fenfeng9 (via GitHub)" <gi...@apache.org>.
fenfeng9 commented on code in PR #50094:
URL: https://github.com/apache/arrow/pull/50094#discussion_r3355837908
##########
cpp/src/arrow/acero/hash_aggregate_test.cc:
##########
@@ -2157,6 +2157,43 @@ TEST_P(GroupBy, AnyAndAll) {
}
}
+TEST_P(GroupBy, AnyAllSlicedNullableBoolean) {
+ auto table = TableFromJSON(schema({field("any_arg", boolean()),
+ field("all_arg", boolean()), field("key", int64())}),
+ {R"([
+ [true, false, 99],
+ [false, true, 10],
+ [null, null, 10]
+ ])"});
+ auto sliced = table->Slice(1);
+
+ // GH-50043: hash_any/hash_all should respect the slice offset.
+ // After Slice(1), any_arg=[false, null] and all_arg=[true, null].
+ for (bool use_threads : {true, false}) {
+ SCOPED_TRACE(use_threads ? "parallel/merged" : "serial");
+
+ ASSERT_OK_AND_ASSIGN(Datum actual, GroupByTest({sliced->GetColumnByName("any_arg"),
Review Comment:
Updated, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50043: [C++][Python] Fix hash_any/hash_all on sliced boolean arrays [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #50094:
URL: https://github.com/apache/arrow/pull/50094#discussion_r3355389049
##########
cpp/src/arrow/acero/hash_aggregate_test.cc:
##########
@@ -2157,6 +2157,43 @@ TEST_P(GroupBy, AnyAndAll) {
}
}
+TEST_P(GroupBy, AnyAllSlicedNullableBoolean) {
+ auto table = TableFromJSON(schema({field("any_arg", boolean()),
+ field("all_arg", boolean()), field("key", int64())}),
+ {R"([
+ [true, false, 99],
+ [false, true, 10],
+ [null, null, 10]
+ ])"});
+ auto sliced = table->Slice(1);
+
+ // GH-50043: hash_any/hash_all should respect the slice offset.
+ // After Slice(1), any_arg=[false, null] and all_arg=[true, null].
+ for (bool use_threads : {true, false}) {
+ SCOPED_TRACE(use_threads ? "parallel/merged" : "serial");
+
+ ASSERT_OK_AND_ASSIGN(Datum actual, GroupByTest({sliced->GetColumnByName("any_arg"),
Review Comment:
```suggestion
ASSERT_OK_AND_ASSIGN(auto actual, GroupByTest({sliced->GetColumnByName("any_arg"),
```
##########
cpp/src/arrow/acero/hash_aggregate_test.cc:
##########
@@ -2157,6 +2157,43 @@ TEST_P(GroupBy, AnyAndAll) {
}
}
+TEST_P(GroupBy, AnyAllSlicedNullableBoolean) {
+ auto table = TableFromJSON(schema({field("any_arg", boolean()),
+ field("all_arg", boolean()), field("key", int64())}),
+ {R"([
+ [true, false, 99],
+ [false, true, 10],
+ [null, null, 10]
+ ])"});
+ auto sliced = table->Slice(1);
+
+ // GH-50043: hash_any/hash_all should respect the slice offset.
+ // After Slice(1), any_arg=[false, null] and all_arg=[true, null].
+ for (bool use_threads : {true, false}) {
+ SCOPED_TRACE(use_threads ? "parallel/merged" : "serial");
+
+ ASSERT_OK_AND_ASSIGN(Datum actual, GroupByTest({sliced->GetColumnByName("any_arg"),
+ sliced->GetColumnByName("all_arg")},
+ {sliced->GetColumnByName("key")},
+ {
+ {"hash_any", nullptr},
+ {"hash_all", nullptr},
+ },
+ use_threads));
+ ValidateOutput(actual);
+
+ Datum expected = ArrayFromJSON(struct_({
Review Comment:
```suggestion
auto expected = ArrayFromJSON(struct_({
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50043: [C++][Python] Fix hash_any/hash_all on sliced boolean arrays [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #50094:
URL: https://github.com/apache/arrow/pull/50094#discussion_r3355852525
##########
cpp/src/arrow/acero/hash_aggregate_test.cc:
##########
@@ -2157,6 +2157,43 @@ TEST_P(GroupBy, AnyAndAll) {
}
}
+TEST_P(GroupBy, AnyAllSlicedNullableBoolean) {
+ auto table = TableFromJSON(schema({field("any_arg", boolean()),
+ field("all_arg", boolean()), field("key", int64())}),
+ {R"([
+ [true, false, 99],
+ [false, true, 10],
+ [null, null, 10]
+ ])"});
+ auto sliced = table->Slice(1);
+
+ // GH-50043: hash_any/hash_all should respect the slice offset.
+ // After Slice(1), any_arg=[false, null] and all_arg=[true, null].
+ for (bool use_threads : {true, false}) {
+ SCOPED_TRACE(use_threads ? "parallel/merged" : "serial");
+
+ ASSERT_OK_AND_ASSIGN(auto actual, GroupByTest({sliced->GetColumnByName("any_arg"),
+ sliced->GetColumnByName("all_arg")},
+ {sliced->GetColumnByName("key")},
+ {
+ {"hash_any", nullptr},
+ {"hash_all", nullptr},
+ },
+ use_threads));
+ ValidateOutput(actual);
+
+ auto expected = ArrayFromJSON(struct_({
+ field("key_0", int64()),
+ field("hash_any", boolean()),
+ field("hash_all", boolean()),
+ }),
+ R"([
+ [10, false, true]
+ ])");
Review Comment:
Another nit: you could define this outside of the for loop.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50043: [C++][Python] Fix hash_any/hash_all on sliced boolean arrays [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #50094:
URL: https://github.com/apache/arrow/pull/50094#discussion_r3355938075
##########
cpp/src/arrow/acero/hash_aggregate_test.cc:
##########
@@ -2157,6 +2157,43 @@ TEST_P(GroupBy, AnyAndAll) {
}
}
+TEST_P(GroupBy, AnyAllSlicedNullableBoolean) {
+ auto table = TableFromJSON(schema({field("any_arg", boolean()),
+ field("all_arg", boolean()), field("key", int64())}),
+ {R"([
+ [true, false, 99],
+ [false, true, 10],
+ [null, null, 10]
+ ])"});
+ auto sliced = table->Slice(1);
+
+ // GH-50043: hash_any/hash_all should respect the slice offset.
+ // After Slice(1), any_arg=[false, null] and all_arg=[true, null].
+ for (bool use_threads : {true, false}) {
+ SCOPED_TRACE(use_threads ? "parallel/merged" : "serial");
+
+ ASSERT_OK_AND_ASSIGN(auto actual, GroupByTest({sliced->GetColumnByName("any_arg"),
+ sliced->GetColumnByName("all_arg")},
+ {sliced->GetColumnByName("key")},
+ {
+ {"hash_any", nullptr},
+ {"hash_all", nullptr},
+ },
+ use_threads));
+ ValidateOutput(actual);
+
+ auto expected = ArrayFromJSON(struct_({
+ field("key_0", int64()),
+ field("hash_any", boolean()),
+ field("hash_all", boolean()),
+ }),
+ R"([
+ [10, false, true]
+ ])");
Review Comment:
Looks good.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50043: [C++][Python] Fix hash_any/hash_all on sliced boolean arrays [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #50094:
URL: https://github.com/apache/arrow/pull/50094#issuecomment-4620660405
:warning: GitHub issue #50043 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50043: [C++][Python] Fix hash_any/hash_all on sliced boolean arrays [arrow]
Posted by "fenfeng9 (via GitHub)" <gi...@apache.org>.
fenfeng9 commented on code in PR #50094:
URL: https://github.com/apache/arrow/pull/50094#discussion_r3355866362
##########
cpp/src/arrow/acero/hash_aggregate_test.cc:
##########
@@ -2157,6 +2157,43 @@ TEST_P(GroupBy, AnyAndAll) {
}
}
+TEST_P(GroupBy, AnyAllSlicedNullableBoolean) {
+ auto table = TableFromJSON(schema({field("any_arg", boolean()),
+ field("all_arg", boolean()), field("key", int64())}),
+ {R"([
+ [true, false, 99],
+ [false, true, 10],
+ [null, null, 10]
+ ])"});
+ auto sliced = table->Slice(1);
+
+ // GH-50043: hash_any/hash_all should respect the slice offset.
+ // After Slice(1), any_arg=[false, null] and all_arg=[true, null].
+ for (bool use_threads : {true, false}) {
+ SCOPED_TRACE(use_threads ? "parallel/merged" : "serial");
+
+ ASSERT_OK_AND_ASSIGN(auto actual, GroupByTest({sliced->GetColumnByName("any_arg"),
+ sliced->GetColumnByName("all_arg")},
+ {sliced->GetColumnByName("key")},
+ {
+ {"hash_any", nullptr},
+ {"hash_all", nullptr},
+ },
+ use_threads));
+ ValidateOutput(actual);
+
+ auto expected = ArrayFromJSON(struct_({
+ field("key_0", int64()),
+ field("hash_any", boolean()),
+ field("hash_all", boolean()),
+ }),
+ R"([
+ [10, false, true]
+ ])");
Review Comment:
Thanks. will update soon.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50043: [C++][Python] Fix hash_any/hash_all on sliced boolean arrays [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on PR #50094:
URL: https://github.com/apache/arrow/pull/50094#issuecomment-4622104983
> > I wonder if we have the same bug elsewhere in aggregate kernels (e.g. for different types). It's out of scope here, but do you have any input @fenfeng9 ?
>
> I don't see the same kind of offset issue in nearby aggregate kernels for other data types.
>
> I'll take a closer look separately, and if I find another case, I'll file a new issue.
Perfect, thank you!
This looks good to me now. Will merge if green.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50043: [C++][Python] Fix hash_any/hash_all on sliced boolean arrays [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #50094:
URL: https://github.com/apache/arrow/pull/50094#discussion_r3355426673
##########
python/pyarrow/tests/test_table.py:
##########
@@ -2976,6 +2976,37 @@ def sorted_by_keys(d):
}
+@pytest.mark.acero
+def test_group_by_sliced_any_all():
+ # GH-50043: hash_any/hash_all produce incorrect results on sliced boolean arrays
+ # Row 0 will be discarded by slice, should not affect aggregation
+ table = pa.table(
+ {
+ "g": [99, 10, 10],
+ "any_arg": [True, False, None],
+ "all_arg": [False, True, None],
+ }
+ )
+ sliced = table.slice(1)
+
+ # any(False, None) = False, all(True, None) = True
+ result = sliced.group_by("g", use_threads=False).aggregate(
Review Comment:
Why `use_threads=False` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50043: [C++][Python] Fix hash_any/hash_all on sliced boolean arrays [arrow]
Posted by "fenfeng9 (via GitHub)" <gi...@apache.org>.
fenfeng9 commented on PR #50094:
URL: https://github.com/apache/arrow/pull/50094#issuecomment-4622111698
> > > I wonder if we have the same bug elsewhere in aggregate kernels (e.g. for different types). It's out of scope here, but do you have any input @fenfeng9 ?
> >
> >
> > I don't see the same kind of offset issue in nearby aggregate kernels for other data types.
> > I'll take a closer look separately, and if I find another case, I'll file a new issue.
>
> Perfect, thank you!
>
> This looks good to me now. Will merge if green.
Thank you for your quick and patient review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50043: [C++][Python] Fix hash_any/hash_all on sliced boolean arrays [arrow]
Posted by "fenfeng9 (via GitHub)" <gi...@apache.org>.
fenfeng9 commented on code in PR #50094:
URL: https://github.com/apache/arrow/pull/50094#discussion_r3355862056
##########
python/pyarrow/tests/test_table.py:
##########
@@ -2976,6 +2976,37 @@ def sorted_by_keys(d):
}
+@pytest.mark.acero
+def test_group_by_sliced_any_all():
+ # GH-50043: hash_any/hash_all produce incorrect results on sliced boolean arrays
+ # Row 0 will be discarded by slice, should not affect aggregation
+ table = pa.table(
+ {
+ "g": [99, 10, 10],
+ "any_arg": [True, False, None],
+ "all_arg": [False, True, None],
+ }
+ )
+ sliced = table.slice(1)
+
+ # any(False, None) = False, all(True, None) = True
+ result = sliced.group_by("g", use_threads=False).aggregate(
Review Comment:
Good point. `use_threads=False` is not necessary here. I initially followed the reproducer from the issue.
I've updated the test to cover both `use_threads=False` and `use_threads=True`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50043: [C++][Python] Fix hash_any/hash_all on sliced boolean arrays [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on PR #50094:
URL: https://github.com/apache/arrow/pull/50094#issuecomment-4622898047
Thanks again @fenfeng9!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50043: [C++][Python] Fix hash_any/hash_all on sliced boolean arrays [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #50094:
URL: https://github.com/apache/arrow/pull/50094#issuecomment-4626034988
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 95cb5d084506f97a16df006053642e9690ab731d.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/79613381880) has more details. It also includes information about 25 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org