Skip to content

ARROW-9489: [C++] Add fill_null kernel implementation for (array[string], scalar[string]) - #8628

Closed
maartenbreddels wants to merge 11 commits into
apache:masterfrom
maartenbreddels:ARROW-9489
Closed

ARROW-9489: [C++] Add fill_null kernel implementation for (array[string], scalar[string])#8628
maartenbreddels wants to merge 11 commits into
apache:masterfrom
maartenbreddels:ARROW-9489

Conversation

@maartenbreddels

@maartenbreddelsmaartenbreddels commented Nov 10, 2020•
edited by pitrou
Loading

Copy link
Copy Markdown
Contributor

No description provided.

maartenbreddels
maartenbreddels commented Nov 10, 2020
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_fill_null.cc Outdated

@maartenbreddelsmaartenbreddelsNov 10, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I accidentally forgot the +1 here, and everything worked. Does CI include valgrind, and should it pick this up?

@pitroupitrouNov 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The +1 should not be necessary. This is the number of values appended, not the number of offsets.

@github-actions

github-actionsBot commented Nov 10, 2020

Copy link
Copy Markdown

https://issues.apache.org/jira/browse/ARROW-9489

pitrou
pitrou reviewed Nov 10, 2020
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_fill_null.cc Outdated
pitrou
pitrou reviewed Nov 10, 2020
View reviewed changes
Comment threadpython/pyarrow/tests/test_compute.py Outdated
@maartenbreddels

maartenbreddels commented Nov 10, 2020

Copy link
Copy Markdown
ContributorAuthor

Looking at the implementation of VisitArrayDataInline, which is quite similar to what is being done in FillNullFunctor<Type, enable_if_t<is_number_type<Type>::value>>, I don't see much room for improvement.

pitrou
pitrou reviewed Nov 12, 2020
View reviewed changes

@pitroupitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just a couple comments. Thank you for this PR!

Comment threadcpp/src/arrow/compute/kernels/scalar_fill_null.cc Outdated

@pitroupitrouNov 12, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Instead of calling MayHaveNulls, just compute the null count and check it here.

Comment threadcpp/src/arrow/compute/kernels/scalar_fill_null.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/codegen_internal.h Outdated
Comment threaddocs/source/cpp/compute.rst Outdated
Comment threadpython/pyarrow/tests/test_compute.py Outdated
@maartenbreddels

maartenbreddels commented Nov 13, 2020

Copy link
Copy Markdown
ContributorAuthor

@pitrou i think this is ready to go/review.

pitrou
pitrou approved these changes Nov 16, 2020
View reviewed changes

@pitroupitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1, will merge. Thank you @maartenbreddels !

@maartenbreddelsmaartenbreddels mentioned this pull request Jan 25, 2021
Merged
5 tasks
@asfimportasfimport mentioned this pull request Nov 16, 2020
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pitroupitroupitrou approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@maartenbreddels@pitrou