Skip to content

ARROW-12669:[C++][Python] Implement a new scalar function: list_element - #11159

Closed
aucahuasi wants to merge 1 commit into
apache:masterfrom
aucahuasi:return-array-for-each-list-in-array
Closed

ARROW-12669:[C++][Python] Implement a new scalar function: list_element#11159
aucahuasi wants to merge 1 commit into
apache:masterfrom
aucahuasi:return-array-for-each-list-in-array

Conversation

@aucahuasi

Copy link
Copy Markdown
Contributor

@github-actions

Copy link
Copy Markdown

@aucahuasi

Copy link
Copy Markdown
ContributorAuthor

Tomorrow I'll work on the python and R bindings for this new function
cc @nealrichardson@edponce@ianmcook

@aucahuasiaucahuasi changed the title ARROW-12669:[C++][Python][R] Implement a new vector function: list_elementARROW-12669:[C++][R] Implement a new vector function: list_elementSep 14, 2021
@aucahuasi
aucahuasiforce-pushed the return-array-for-each-list-in-array branch from 91f1638 to b38ee7eCompareSeptember 15, 2021 01:14
@aucahuasi
aucahuasi marked this pull request as ready for review September 15, 2021 01:19
@aucahuasiaucahuasi changed the title ARROW-12669:[C++][R] Implement a new vector function: list_elementARROW-12669:[C++][Python] Implement a new vector function: list_elementSep 15, 2021
@lidavidm

Copy link
Copy Markdown
Member

Just a quick thought, why is this a vector kernel instead of a scalar one? (Though with one input it's maybe not a big difference.)

@lidavidm

Copy link
Copy Markdown
Member

Oh wait nevermind, the index here is a parameter and not an options.

Comment threadcpp/src/arrow/compute/kernels/vector_nested.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/vector_nested.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/vector_nested.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/vector_nested.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/vector_nested.cc Outdated
@nealrichardson

Copy link
Copy Markdown
Member

why is this a vector kernel instead of a scalar one?

I think it has to be a scalar kernel if we want to be able to call it in a projection node, which is why we're doing this now--right?

@lidavidm

Copy link
Copy Markdown
Member

why is this a vector kernel instead of a scalar one?

I think it has to be a scalar kernel if we want to be able to call it in a projection node, which is why we're doing this now--right?

Ah, right - plus expressions aren't evaluatable unless they're scalar:

if (!expr.IsScalarExpression()) {
returnStatus::Invalid(
"ExecuteScalarExpression cannot Execute non-scalar expression ", expr.ToString());
}

I think most kernels like this pass the second argument via options, not as an argument. I was also debating whether to comment about this, but might as well: there's no need to generate kernels for different index types, especially if we just make the index an option instead of a parameter. (It'll also limit the number of special cases to account for in type coverage testing.)

@jorisvandenbossche

Copy link
Copy Markdown
Member

I think most kernels like this pass the second argument via options, not as an argument.

Why can't this be a scalar kernel while still having index as a second argument?
Having the index as argument keeps the flexibility to be able to get a different index of each row (I don't know if this would be an important functionality though, but there are probably use cases)

Comment threaddocs/source/cpp/compute.rst Outdated
@lidavidm

Copy link
Copy Markdown
Member

I think most kernels like this pass the second argument via options, not as an argument.

Why can't this be a scalar kernel while still having index as a second argument?
Having the index as argument keeps the flexibility to be able to get a different index of each row (I don't know if this would be an important functionality though, but there are probably use cases)

Ah, you're right. So this should just be a scalar kernel, then.

@aucahuasi
aucahuasiforce-pushed the return-array-for-each-list-in-array branch 2 times, most recently from ea560fa to 954ca3eCompareSeptember 16, 2021 01:55
Comment threadcpp/src/arrow/compute/kernels/test_util.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_nested_test.cc Outdated
Comment threaddocs/source/cpp/compute.rst Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_nested.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_nested.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_nested.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_nested.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_nested.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_nested.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/codegen_internal.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_nested.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_nested.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_nested_test.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_nested_test.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_nested_test.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_nested_test.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/test_util.cc Outdated
Comment threaddocs/source/cpp/compute.rst Outdated
@aucahuasi
aucahuasiforce-pushed the return-array-for-each-list-in-array branch from 954ca3e to 6201ebfCompareSeptember 18, 2021 00:05

@lidavidmlidavidm 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! This looks good to me, I just left one note - the scalar kernel doesn't need to be templated on the list type, so you can save yourself some generated code there.

Comment threaddocs/source/cpp/compute.rst Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_nested_test.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_nested.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_nested.cc Outdated
@aucahuasi
aucahuasiforce-pushed the return-array-for-each-list-in-array branch from 6201ebf to 2a207d5CompareSeptember 18, 2021 14:57
Comment threaddocs/source/python/api/compute.rst Outdated
Comment threaddocs/source/cpp/compute.rst Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_nested.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_nested_test.cc Outdated
@ianmcookianmcook changed the title ARROW-12669:[C++][Python] Implement a new vector function: list_elementARROW-12669:[C++][Python] Implement a new scalar function: list_elementSep 18, 2021
@aucahuasi
aucahuasiforce-pushed the return-array-for-each-list-in-array branch from 2a207d5 to 5b899d6CompareSeptember 20, 2021 17:01
Comment threadcpp/src/arrow/compute/kernels/scalar_nested.cc Outdated

@lidavidmlidavidm 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.

LGTM, thanks!

Comment threadcpp/src/arrow/compute/kernels/scalar_nested.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_nested.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_nested.cc Outdated
add python test and C++ docs for the new function: list_element
convert list_element into a scalar function
minor changes
format
support scalar inputs for list_element, improve tests
minor changes
less generated code thanks to some template tricks
less generated code, again using template tricks
@aucahuasi
aucahuasiforce-pushed the return-array-for-each-list-in-array branch from 5b899d6 to 2369457CompareSeptember 20, 2021 18:10
Comment threadcpp/src/arrow/compute/kernels/scalar_nested.cc

@edponceedponce left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for resolving this issue!

ViniciusSouzaRoque pushed a commit to s1mbi0se/arrow that referenced this pull request Oct 20, 2021
https://issues.apache.org/jira/browse/ARROW-12669Closesapache#11159 from aucahuasi/return-array-for-each-list-in-array
Authored-by: Percy Camilo Triveño Aucahuasi <percy.camilo.ta@gmail.com>
Signed-off-by: David Li <li.davidm96@gmail.com>
Sign up for freeto 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.

6 participants

@aucahuasi@lidavidm@nealrichardson@jorisvandenbossche@ianmcook@edponce