Skip to content

Conversation

@cyb70289
Copy link
Contributor

No description provided.

@github-actions
Copy link

@cyb70289
Copy link
Contributor Author

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

@cyb70289
Copy link
Contributor Author

Appveyor CI failure looks a spurious network issue:
fatal: unable to access 'https://github.com/frerich/clcache.git/': Could not resolve host: github.com

@cyb70289
Copy link
Contributor Author

cyb70289 commented Apr 28, 2021

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.

Shall we include all fixes in this single PR? @kszucs @pitrou

EDIT: fixes included in this pr.

@pitrou
Copy link
Member

pitrou commented Apr 28, 2021

@cyb70289 That would be nice, yes!

@cyb70289 cyb70289 changed the title ARROW-12568: [C++][Compute] Segfault when casting sliced ListArray ARROW-12568: [C++][Compute] Fix nullptr deference when array contains no nulls Apr 28, 2021
@cyb70289 cyb70289 requested review from bkietz and pitrou April 28, 2021 11:27
Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

Sorry, some more comments.

@@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

How about instead:

out->null_count = 0;
out->buffers[0] = nullptr;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.
Looks it's better to tell the scalar kernel framework not to pre-allocate the valid buffer.

@pitrou pitrou self-requested a review May 3, 2021 14:51
Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

+1, thank you @cyb70289

@pitrou pitrou closed this in ce28617 May 3, 2021
@cyb70289 cyb70289 deleted the 12568-cast-crash branch May 4, 2021 00:20
kszucs pushed a commit to kszucs/arrow that referenced this pull request May 17, 2021
… no nulls

Closes apache#10184 from cyb70289/12568-cast-crash

Lead-authored-by: Yibo Cai <yibo.cai@arm.com>
Co-authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants