Skip to content

ARROW-13358: [C++] Improve type support in if_else - #11218

Closed
lidavidm wants to merge 5 commits into
apache:masterfrom
lidavidm:arrow-13358
Closed

ARROW-13358: [C++] Improve type support in if_else#11218
lidavidm wants to merge 5 commits into
apache:masterfrom
lidavidm:arrow-13358

Conversation

@lidavidm

@lidavidmlidavidm commented Sep 23, 2021

Copy link
Copy Markdown
Member

No description provided.

@github-actions

github-actionsBot commented Sep 23, 2021

Copy link
Copy Markdown

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

lidavidm
lidavidm commented Sep 23, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/codegen_internal.cc Outdated

@lidavidmlidavidmSep 23, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Much of this will need to be reconciled with ARROW-13390/#11080.

@pitrou

pitrou commented Sep 29, 2021

Copy link
Copy Markdown
Member

This needs rebasing now.

@lidavidm
lidavidmforce-pushed the arrow-13358 branch 2 times, most recently from bca16d4 to 870c1a6CompareSeptember 29, 2021 18:00
@lidavidm

lidavidm commented Sep 29, 2021

Copy link
Copy Markdown
MemberAuthor

Contains some changes from #11272 so if_else/case_when/coalesce have mostly similar implicit cast rules (though I will leave it to ARROW-14105 to fully unify these). I may also revisit the dictionary case here as it seems slow from test runtime; instead of unifying the dictionaries on-the-fly we should unify them up front and construct a transposition map. (If we copy the RHS to the output, we can use the existing function for transposing indices, and assuming unification preserves the first dictionary's indices exactly, we can then copy the LHS to the output without having to transpose again.)

@lidavidm

lidavidm commented Sep 29, 2021

Copy link
Copy Markdown
MemberAuthor

Ah, the described optimization would require being able to unify dictionaries with nulls. Filed ARROW-14177 for now.

pitrou
pitrou reviewed Sep 30, 2021
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.

Very nice!

Comment threadcpp/src/arrow/compute/kernels/scalar_if_else.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_if_else.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_if_else.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_if_else.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_if_else_test.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_if_else_test.cc Outdated
pitrou
pitrou approved these changes Oct 4, 2021
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.

Thanks for the updates. +1 from me.

@pitroupitrou closed this in dfb109cOct 4, 2021
ViniciusSouzaRoque pushed a commit to s1mbi0se/arrow that referenced this pull request Oct 20, 2021
@lidavidm@pitrou@ViniciusSouzaRoque
ARROW-13358: [C++] Improve type support in if_else
01584a8
Closesapache#11218 from lidavidm/arrow-13358
Lead-authored-by: David Li <li.davidm96@gmail.com>
Co-authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@asfimportasfimport mentioned this pull request Nov 29, 2021
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

Component: C++

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@lidavidm@pitrou