You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/04/28 06:46:11 UTC
[GitHub] [arrow] cyb70289 opened a new pull request #10184: ARROW-12568: [C++][Compute] Segfault when casting sliced ListArray
cyb70289 opened a new pull request #10184:
URL: https://github.com/apache/arrow/pull/10184
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cyb70289 commented on a change in pull request #10184: ARROW-12568: [C++][Compute] Fix nullptr deference when array contains no nulls
Posted by GitBox <gi...@apache.org>.
cyb70289 commented on a change in pull request #10184:
URL: https://github.com/apache/arrow/pull/10184#discussion_r623048881
##########
File path: cpp/src/arrow/compute/kernels/scalar_boolean_test.cc
##########
@@ -110,6 +110,12 @@ TEST(TestBooleanKernel, KleeneAnd) {
expected = ArrayFromJSON(boolean(), "[true, false, false, null, false]");
CheckScalarBinary("and_kleene", left, right, expected);
CheckBooleanScalarArrayBinary("and_kleene", left);
+
+ left = ArrayFromJSON(boolean(), " [true, true, false, true]");
+ right = ArrayFromJSON(boolean(), " [true, false, false, false]");
+ expected = ArrayFromJSON(boolean(), "[true, false, false, false]");
+ CheckScalarBinary("and_kleene", left, right, expected);
+ CheckBooleanScalarArrayBinary("and_kleene", left);
Review comment:
Yes. These tests segfault without the change. So is the added hash aggregagte test.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cyb70289 commented on pull request #10184: ARROW-12568: [C++][Compute] Segfault when casting sliced ListArray
Posted by GitBox <gi...@apache.org>.
cyb70289 commented on pull request #10184:
URL: https://github.com/apache/arrow/pull/10184#issuecomment-828201417
Appveyor CI failure looks a spurious network issue:
`fatal: unable to access 'https://github.com/frerich/clcache.git/': Could not resolve host: github.com`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cyb70289 commented on pull request #10184: ARROW-12568: [C++][Compute] Segfault when casting sliced ListArray
Posted by GitBox <gi...@apache.org>.
cyb70289 commented on pull request #10184:
URL: https://github.com/apache/arrow/pull/10184#issuecomment-828200338
Please not this patch cannot be directly cherry picked to 4.0 release branch, as it's based on #10098 just merged, which is a pretty big change. Maybe apply manually a trivial patch to 4.0 release only? @kszucs
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cyb70289 edited a comment on pull request #10184: ARROW-12568: [C++][Compute] Segfault when casting sliced ListArray
Posted by GitBox <gi...@apache.org>.
cyb70289 edited a comment on pull request #10184:
URL: https://github.com/apache/arrow/pull/10184#issuecomment-828213344
Turns out there are more "buffers[0]->data()" codes where "buffers[0]" is not checked for null.
Unit test doesn't cover the case that input contains no nulls.
- Three in https://github.com/apache/arrow/blob/master/cpp/src/arrow/compute/kernels/scalar_boolean.cc
- One in https://github.com/apache/arrow/blob/master/cpp/src/arrow/compute/kernels/hash_aggregate.cc
Shall we include all fixes in this single PR? @kszucs @pitrou
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cyb70289 commented on a change in pull request #10184: ARROW-12568: [C++][Compute] Segfault when casting sliced ListArray
Posted by GitBox <gi...@apache.org>.
cyb70289 commented on a change in pull request #10184:
URL: https://github.com/apache/arrow/pull/10184#discussion_r622046810
##########
File path: cpp/src/arrow/compute/kernels/scalar_boolean.cc
##########
@@ -175,15 +175,24 @@ struct KleeneAnd : Commutative<KleeneAnd> {
}
if (right_true) {
- GetBitmap(*out, 0).CopyFrom(GetBitmap(left, 0));
+ if (left.null_count == 0) {
Review comment:
Should use `GetNullCount()`?
I see `null_count` is used extensively in this file.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cyb70289 commented on a change in pull request #10184: ARROW-12568: [C++][Compute] Fix nullptr deference when array contains no nulls
Posted by GitBox <gi...@apache.org>.
cyb70289 commented on a change in pull request #10184:
URL: https://github.com/apache/arrow/pull/10184#discussion_r625114213
##########
File path: cpp/src/arrow/compute/kernels/scalar_boolean.cc
##########
@@ -175,15 +175,24 @@ struct KleeneAnd : Commutative<KleeneAnd> {
}
if (right_true) {
- GetBitmap(*out, 0).CopyFrom(GetBitmap(left, 0));
+ if (left.GetNullCount() == 0) {
+ GetBitmap(*out, 0).SetBitsTo(true);
Review comment:
Done.
Looks it's better to tell the scalar kernel framework not to pre-allocate the valid buffer.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #10184: ARROW-12568: [C++][Compute] Fix nullptr deference when array contains no nulls
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #10184:
URL: https://github.com/apache/arrow/pull/10184#discussion_r622893252
##########
File path: cpp/src/arrow/compute/kernels/scalar_boolean.cc
##########
@@ -175,15 +175,24 @@ struct KleeneAnd : Commutative<KleeneAnd> {
}
if (right_true) {
- GetBitmap(*out, 0).CopyFrom(GetBitmap(left, 0));
+ if (left.null_count == 0) {
Review comment:
Yes, `GetNullCount()` will cache the null count value so that it's only computed once.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10184: ARROW-12568: [C++][Compute] Segfault when casting sliced ListArray
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10184:
URL: https://github.com/apache/arrow/pull/10184#issuecomment-828191810
https://issues.apache.org/jira/browse/ARROW-12568
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cyb70289 edited a comment on pull request #10184: ARROW-12568: [C++][Compute] Segfault when casting sliced ListArray
Posted by GitBox <gi...@apache.org>.
cyb70289 edited a comment on pull request #10184:
URL: https://github.com/apache/arrow/pull/10184#issuecomment-828213344
Turns out there are more "buffers[0]->data()" codes where "buffers[0]" is not checked for null.
Unit test doesn't cover the case that input contains no nulls.
- Three in https://github.com/apache/arrow/blob/master/cpp/src/arrow/compute/kernels/scalar_boolean.cc
- One in https://github.com/apache/arrow/blob/master/cpp/src/arrow/compute/kernels/hash_aggregate.cc
Shall we include all fixes in this single PR? @kszucs @pitrou
EDIT: fixes included in this pr.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #10184: ARROW-12568: [C++][Compute] Fix nullptr deference when array contains no nulls
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #10184:
URL: https://github.com/apache/arrow/pull/10184#discussion_r624976026
##########
File path: cpp/src/arrow/compute/kernels/scalar_boolean.cc
##########
@@ -379,15 +397,24 @@ struct KleeneAndNot {
}
if (left_true) {
- GetBitmap(*out, 0).CopyFrom(GetBitmap(right, 0));
+ if (right.GetNullCount() == 0) {
+ GetBitmap(*out, 0).SetBitsTo(true);
Review comment:
And here.
##########
File path: cpp/src/arrow/compute/kernels/scalar_boolean.cc
##########
@@ -175,15 +175,24 @@ struct KleeneAnd : Commutative<KleeneAnd> {
}
if (right_true) {
- GetBitmap(*out, 0).CopyFrom(GetBitmap(left, 0));
+ if (left.GetNullCount() == 0) {
+ GetBitmap(*out, 0).SetBitsTo(true);
Review comment:
How about instead:
```c++
out->null_count = 0;
out->buffers[0] = nullptr;
```
##########
File path: cpp/src/arrow/compute/kernels/scalar_boolean.cc
##########
@@ -266,15 +275,24 @@ struct KleeneOr : Commutative<KleeneOr> {
}
if (right_false) {
- GetBitmap(*out, 0).CopyFrom(GetBitmap(left, 0));
+ if (left.GetNullCount() == 0) {
+ GetBitmap(*out, 0).SetBitsTo(true);
Review comment:
Same here.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #10184: ARROW-12568: [C++][Compute] Segfault when casting sliced ListArray
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #10184:
URL: https://github.com/apache/arrow/pull/10184#issuecomment-828254745
@cyb70289 That would be nice, yes!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou closed pull request #10184: ARROW-12568: [C++][Compute] Fix nullptr deference when array contains no nulls
Posted by GitBox <gi...@apache.org>.
pitrou closed pull request #10184:
URL: https://github.com/apache/arrow/pull/10184
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cyb70289 commented on pull request #10184: ARROW-12568: [C++][Compute] Segfault when casting sliced ListArray
Posted by GitBox <gi...@apache.org>.
cyb70289 commented on pull request #10184:
URL: https://github.com/apache/arrow/pull/10184#issuecomment-828213344
Turns out there are more "buffers[0]->data()" codes where "buffers[0]" is not checked for null.
- Three in https://github.com/apache/arrow/blob/master/cpp/src/arrow/compute/kernels/scalar_boolean.cc
- One in https://github.com/apache/arrow/blob/master/cpp/src/arrow/compute/kernels/hash_aggregate.cc
Unit test doesn't cover the case that input contains no nulls.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cyb70289 edited a comment on pull request #10184: ARROW-12568: [C++][Compute] Segfault when casting sliced ListArray
Posted by GitBox <gi...@apache.org>.
cyb70289 edited a comment on pull request #10184:
URL: https://github.com/apache/arrow/pull/10184#issuecomment-828213344
Turns out there are more "buffers[0]->data()" codes where "buffers[0]" is not checked for null.
Unit test doesn't cover the case that input contains no nulls.
- Three in https://github.com/apache/arrow/blob/master/cpp/src/arrow/compute/kernels/scalar_boolean.cc
- One in https://github.com/apache/arrow/blob/master/cpp/src/arrow/compute/kernels/hash_aggregate.cc
Shall we include all fixes in this single PR? @kszucs @pitrou
**EDIT: fixes included in this pr.**
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #10184: ARROW-12568: [C++][Compute] Fix nullptr deference when array contains no nulls
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #10184:
URL: https://github.com/apache/arrow/pull/10184#discussion_r622894175
##########
File path: cpp/src/arrow/compute/kernels/scalar_boolean_test.cc
##########
@@ -110,6 +110,12 @@ TEST(TestBooleanKernel, KleeneAnd) {
expected = ArrayFromJSON(boolean(), "[true, false, false, null, false]");
CheckScalarBinary("and_kleene", left, right, expected);
CheckBooleanScalarArrayBinary("and_kleene", left);
+
+ left = ArrayFromJSON(boolean(), " [true, true, false, true]");
+ right = ArrayFromJSON(boolean(), " [true, false, false, false]");
+ expected = ArrayFromJSON(boolean(), "[true, false, false, false]");
+ CheckScalarBinary("and_kleene", left, right, expected);
+ CheckBooleanScalarArrayBinary("and_kleene", left);
Review comment:
Did you validate those tests fail without the changes in `scalar_boolean.cc`?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org