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

GH-44541: [C++] NumericArray<T> should not use ctor from parent directly #44542

Merged
merged 4 commits into from
Nov 11, 2024
Merged

GH-44541: [C++] NumericArray<T> should not use ctor from parent directly #44542

merged 4 commits into from
Nov 11, 2024

Conversation

mapleFU
Copy link
Member

@mapleFU mapleFU commented Oct 28, 2024 •
edited by github-actions bot
Loading

Rationale for this change

See #44541

What changes are included in this PR?

NumericArray<T> not using child's ctor.

Are these changes tested?

Yes

Are there any user-facing changes?

Bugfix

@mapleFU mapleFU requested review from pitrou and bkietz October 28, 2024 11:16
@mapleFU
Copy link
Member Author

mapleFU commented Oct 28, 2024

@pitrou @bkietz @felipecrv
cc @wgtmac

@github-actions GitHub Actions
Copy link

⚠️ GitHub issue #44541 has been automatically assigned in GitHub to PR creator.

@mapleFU mapleFU force-pushed the fix-numeric-array-ctor branch from a8169c4 to 1896d63 Compare October 28, 2024 11:20
wgtmac
wgtmac reviewed Oct 28, 2024
View reviewed changes
cpp/src/arrow/array/array_primitive.h
@@ -119,8 +129,6 @@ class NumericArray : public PrimitiveArray {
IteratorType end() const { return IteratorType(*this, length()); }

protected:
using PrimitiveArray::PrimitiveArray;
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to add NumericArray() if this line is deleted?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch 🤔

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Oct 28, 2024
@wgtmac wgtmac changed the title GH-44541: [C++] NumericArray<T> should not use child's ctor directly GH-44541: [C++] NumericArray<T> should not use ctor from parent directly Oct 28, 2024
wgtmac
wgtmac reviewed Oct 28, 2024
View reviewed changes
cpp/src/arrow/array/array_test.cc
@@ -585,6 +585,18 @@ TEST_F(TestArray, TestValidateNullCount) {
}
}

TEST_F(TestArray, TestValidValues) {
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a test case for TimestampArray specifically?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

wgtmac
wgtmac approved these changes Oct 28, 2024
View reviewed changes
Copy link
Member

@wgtmac wgtmac left a comment

Choose a reason for hiding this comment

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

+1. Thanks!

bkietz
bkietz reviewed Oct 28, 2024
View reviewed changes
cpp/src/arrow/array/array_primitive.h
@@ -119,7 +129,7 @@ class NumericArray : public PrimitiveArray {
IteratorType end() const { return IteratorType(*this, length()); }

protected:
Copy link
Member

Choose a reason for hiding this comment

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

The use of protected is odd here; nothing inherits from NumericArray inside arrow. Is it intended to support user subclassing of NumericArray? Or could we just mark NumericArray final? @pitrou

Copy link
Member Author

Choose a reason for hiding this comment

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

This might keep same style from PrimitiveArray, which initializes raw_values_ in ctor. Don't know should we change here

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Oct 28, 2024
@mapleFU
Copy link
Member Author

mapleFU commented Nov 3, 2024

Gentle ping @pitrou

@mapleFU
Copy link
Member Author

mapleFU commented Nov 6, 2024

@pitrou @bkietz should we move forward first since this is a bugfix..?

@wgtmac
Copy link
Member

wgtmac commented Nov 6, 2024

cc @kou @emkornfield

kou
kou approved these changes Nov 6, 2024
View reviewed changes
Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

+1

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Nov 6, 2024
@mapleFU
Copy link
Member Author

mapleFU commented Nov 7, 2024 •
edited
Loading

Would merge this in next monday if no negative comments, hope it would be in 18.0.1 @pitrou

@mapleFU mapleFU merged commit 95b7181 into apache:main Nov 11, 2024
39 checks passed
@mapleFU mapleFU removed the awaiting merge Awaiting merge label Nov 11, 2024
@mapleFU mapleFU mentioned this pull request Nov 11, 2024
Closed
@mapleFU mapleFU deleted the fix-numeric-array-ctor branch November 11, 2024 06:07
@mapleFU
Copy link
Member Author

mapleFU commented Nov 11, 2024

I merge this firstly, negative comment can by addressed later

@conbench-apache-arrow conbench-apache-arrow
Copy link

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 95b7181.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them.

@mapleFU mapleFU mentioned this pull request Nov 12, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bkietz bkietz bkietz left review comments

@wgtmac wgtmac wgtmac approved these changes

@kou kou kou approved these changes

@pitrou pitrou Awaiting requested review from pitrou

Assignees
No one assigned
Labels
Component: C++
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mapleFU @wgtmac @kou @bkietz