Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ARROW-6276: [C++] Add operator[] for some arrow classes #11694

Closed
wants to merge 5 commits into from
Closed

ARROW-6276: [C++] Add operator[] for some arrow classes #11694

wants to merge 5 commits into from

Conversation

9prady9
Copy link
Contributor

@9prady9 9prady9 commented Nov 12, 2021 •
edited
Loading

The following classes have now an additional operator
overload that fetches ith element.

  • BooleanArray
  • NumericArray
  • DayTimeIntervalArray
  • MonthDayNanoIntervalArray
  • BaseBinaryArray
  • FixedSizeBinaryArray

@github-actions
Copy link

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

@pitrou
Copy link
Member

pitrou commented Nov 16, 2021

I think this should be consistent with our iterators, and they return a util::optional. So this cannot simply defer to GetView.
https://github.com/apache/arrow/blob/master/cpp/src/arrow/stl_iterator.h#L46

@9prady9 9prady9 force-pushed the ARROW-6276-Add-operator-to-some-concrete-Array-impl branch from 659eb03 to 32f8388 Compare November 17, 2021 11:03
@9prady9
Copy link
Contributor Author

9prady9 commented Nov 17, 2021 •
edited
Loading

@pitrou Please have a look now. I have tried to use the aliases from the classes but that doesn't work as of now.

@9prady9 9prady9 marked this pull request as ready for review November 17, 2021 11:04
pitrou
pitrou requested changes Nov 22, 2021
View reviewed changes
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.

This looks better now but can you add some tests?
The related test files are array_test.cc, arrow_binary_test.cc, etc. inside arrow/array.

cpp/src/arrow/array/array_primitive.h Outdated Show resolved Hide resolved
@9prady9 9prady9 force-pushed the ARROW-6276-Add-operator-to-some-concrete-Array-impl branch from 32f8388 to b4040a9 Compare November 23, 2021 10:43
pitrou
pitrou requested changes Nov 24, 2021
View reviewed changes
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.

Thanks for the update! Here are some comments, and an answer to your question.

cpp/src/arrow/array/array_binary_test.cc Outdated Show resolved Hide resolved
cpp/src/arrow/array/array_binary_test.cc Outdated Show resolved Hide resolved
cpp/src/arrow/array/array_test.cc Outdated Show resolved Hide resolved
cpp/src/arrow/array/array_test.cc Outdated Show resolved Hide resolved
@9prady9 9prady9 force-pushed the ARROW-6276-Add-operator-to-some-concrete-Array-impl branch from 90cf838 to 75faa3e Compare November 24, 2021 18:21
@9prady9 9prady9 requested a review from pitrou November 25, 2021 06:14
pitrou
pitrou requested changes Nov 25, 2021
View reviewed changes
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.

Thanks for the update. Some more comments.

cpp/src/arrow/array/array_test.cc Outdated Show resolved Hide resolved
cpp/src/arrow/array/array_test.cc Outdated Show resolved Hide resolved
cpp/src/arrow/array/array_test.cc Outdated Show resolved Hide resolved
cpp/src/arrow/array/array_primitive.h Show resolved Hide resolved
@9prady9
Copy link
Contributor Author

9prady9 commented Nov 25, 2021

#11694 (comment) hehe, yes, sorry.

#11694 (comment) noted

@9prady9
ARROW-6276: [C++] Add operator[] for Array classes
4b61d41 
The following classes have now an additional operator[](int64_t)
overload that fetches ith element.

- BooleanArray
- NumericArray
- DayTimeIntervalArray
- MonthDayNanoIntervalArray
- BaseBinaryArray
- FixedSizeBinaryArray

Authored-by: Pradeep Garigipati <pradeep.garigipati@gmail.com>
Signed-off-by: Pradeep Garigipati <pradeep.garigipati@gmail.com>
@9prady9
Split IntervalArray tests into separate unit tests
8341628 
Signed-off-by: Pradeep Garigipati <pradeep.garigipati@gmail.com>
@9prady9 9prady9 force-pushed the ARROW-6276-Add-operator-to-some-concrete-Array-impl branch from 75faa3e to 8341628 Compare November 25, 2021 15:18
@9prady9 9prady9 requested a review from pitrou November 25, 2021 15:19
pitrou
pitrou reviewed Nov 25, 2021
View reviewed changes
cpp/src/arrow/array/array_test.cc Outdated

typedef ::testing::Types<PBoolean, PUInt8, PUInt16, PUInt32, PUInt64, PInt8, PInt16,
PInt32, PInt64, PFloat, PDouble>
NumericPrimitives;
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to also make it work for MonthIntervalType / MonthIntervalArray? See arrow/type_fwd.h for their definitions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I was referring to that itself in the first sentence of my earlier comment when I said two kinds of vectors are needed: 1) Array of scalars for all numeric types 2) Array of tuples is needed for IntervalArray types.

I tried and failed with one approach earlier. But I think current one(about to push) works fine. Please have a look.

@9prady9
Modified Test Fixture to handle all types
fb652e3 
Signed-off-by: Pradeep Garigipati <pradeep.garigipati@gmail.com>
@9prady9 9prady9 requested a review from pitrou November 25, 2021 16:31
9prady9 and others added 2 commits November 25, 2021 22:20
@9prady9
Add missing asserts to builder calls
7dbf008 
Signed-off-by: Pradeep Garigipati <pradeep.garigipati@gmail.com>
@pitrou
Test against expected validity bits
b14896d
pitrou
pitrou approved these changes Nov 30, 2021
View reviewed changes
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.

I've pushed a small change and will merge. Thanks @9prady9 !

@pitrou pitrou closed this in 0962957 Nov 30, 2021
@ursabot
Copy link

ursabot commented Nov 30, 2021 •
edited
Loading

Benchmark runs are scheduled for baseline = 0ee3449 and contender = 0962957. 0962957 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️25.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️1.79% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.13% ⬆️0.09%] ursa-thinkcentre-m75q
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True

@9prady9 9prady9 deleted the ARROW-6276-Add-operator-to-some-concrete-Array-impl branch November 30, 2021 13:35
kou pushed a commit to kou/arrow that referenced this pull request Dec 1, 2021
@9prady9 @pitrou @kou
ARROW-6276: [C++] Add operator[] for some arrow classes
d6805ba 
The following classes have now an additional operator[](int64_t)
overload that fetches ith element.

- BooleanArray
- NumericArray
- DayTimeIntervalArray
- MonthDayNanoIntervalArray
- BaseBinaryArray
- FixedSizeBinaryArray

Closes apache#11694 from 9prady9/ARROW-6276-Add-operator-to-some-concrete-Array-impl

Lead-authored-by: Pradeep Garigipati <pradeep.garigipati@gmail.com>
Co-authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@asfimport asfimport mentioned this pull request Dec 1, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pitrou pitrou pitrou 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.
3 participants
@9prady9 @pitrou @ursabot