GH-43759: [C++] Acero: Minor code enhancement for Join #43760

mapleFU · 2024-08-19T19:58:42Z

Rationale for this change

Minor style enhancement for join

What changes are included in this PR?

Minor style enhancement for join

Are these changes tested?

Covered by existing

Are there any user-facing changes?

no

GitHub Issue: [C++] Acero: Minor code enhancement for Join #43759

github-actions · 2024-08-19T19:59:10Z

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

zanmato1984

LGTM

pitrou · 2024-08-20T08:46:32Z

cpp/src/arrow/compute/light_array_internal.cc

@@ -323,8 +322,7 @@ Status ResizableArrayData::ResizeFixedLengthBuffers(int num_rows_new) {
 }

 Status ResizableArrayData::ResizeVaryingLengthBuffer() {
-  KeyColumnMetadata column_metadata;
-  column_metadata = ColumnMetadataFromDataType(data_type_).ValueOrDie();
+  KeyColumnMetadata column_metadata = ColumnMetadataFromDataType(data_type_).ValueOrDie();


Why not use ARROW_ASSIGN_OR_RAISE? We are able to return a Status here.

Also, it seems ColumnMetadataFromDataType(data_type_) is being called from multiple methods. Why is the result not stored somewhere on the class?

Why not use ARROW_ASSIGN_OR_RAISE? We are able to return a Status here.

I'm new to this code, when I'm reading this code I found the Join will gurantees the type is safe from ColumnMetadataFromDataType.

Also, it seems ColumnMetadataFromDataType(data_type_) is being called from multiple methods. Why is the result not stored somewhere on the class?

I don't know :-( But this metadata is light weight, just a working around for data_type_, whether caching this is both ok for me

@pitrou Should I use column_metadata and vendor it? Or keep it here?

@zanmato1984 What do you think?

Why not use ARROW_ASSIGN_OR_RAISE? We are able to return a Status here.

Right, I would suggest the same. Also, there are other occurrences of unnecessary ValueOrDie() in this file. We'd better clean them all up.

Also, it seems ColumnMetadataFromDataType(data_type_) is being called from multiple methods. Why is the result not stored somewhere on the class?

Seems like ColumnMetadataFromDataType() is not so trivial - there are a bunch of dynamic casts. So yes, it's even better to store it in a private member of ResizableArrayData - this would save us a lot of s/ValueOrDie()/ARROW_ASSIGN_OR_RAISE work as well:

-void ResizableArrayData::Init(const std::shared_ptr<DataType>& data_type, +Status ResizableArrayData::Init(const std::shared_ptr<DataType>& data_type, MemoryPool* pool, int log_num_rows_min) { + ARROW_ASSIGN_OR_RAISE(auto metadata_after, ColumnMetadataFromDataType(data_type)); #ifndef NDEBUG if (num_rows_allocated_ > 0) { ARROW_DCHECK(data_type_ != NULLPTR); - KeyColumnMetadata metadata_before = - ColumnMetadataFromDataType(data_type_).ValueOrDie(); - ARROW_DCHECK(metadata_before.is_fixed_length == metadata_after.is_fixed_length && - metadata_before.fixed_length == metadata_after.fixed_length); + ARROW_DCHECK(metadata_.is_fixed_length == metadata_after.is_fixed_length && + metadata_.fixed_length == metadata_after.fixed_length); } #endif + metadata_ = metadata_after; Clear(/*release_buffers=*/false); log_num_rows_min_ = log_num_rows_min; data_type_ = data_type; pool_ = pool; + return Status::OK(); }

zanmato1984

Just show how we can avoid instantiating KeyColumnMetadata, and the subsequent ValueOrDie().

zanmato1984 · 2024-08-28T10:46:58Z

cpp/src/arrow/compute/light_array_internal.cc

@@ -343,20 +341,19 @@ Status ResizableArrayData::ResizeVaryingLengthBuffer() {
 }

 KeyColumnArray ResizableArrayData::column_array() const {
-  KeyColumnMetadata column_metadata;
-  column_metadata = ColumnMetadataFromDataType(data_type_).ValueOrDie();
+  KeyColumnMetadata column_metadata = ColumnMetadataFromDataType(data_type_).ValueOrDie();
  return KeyColumnArray(column_metadata, num_rows_,


Suggested change

return KeyColumnArray(column_metadata, num_rows_,

return KeyColumnArray(metadata_, num_rows_,

zanmato1984 · 2024-08-28T10:47:11Z

cpp/src/arrow/compute/light_array_internal.cc

@@ -343,20 +341,19 @@ Status ResizableArrayData::ResizeVaryingLengthBuffer() {
 }

 KeyColumnArray ResizableArrayData::column_array() const {
-  KeyColumnMetadata column_metadata;
-  column_metadata = ColumnMetadataFromDataType(data_type_).ValueOrDie();
+  KeyColumnMetadata column_metadata = ColumnMetadataFromDataType(data_type_).ValueOrDie();


Suggested change

KeyColumnMetadata column_metadata = ColumnMetadataFromDataType(data_type_).ValueOrDie();

mapleFU · 2024-08-28T11:11:42Z

I like the idea for that, but just mention that this hateful ValueOrDie is called widely in row-api, sigh

zanmato1984 · 2024-08-28T13:09:12Z

cpp/src/arrow/compute/light_array_internal.cc

@@ -235,6 +237,7 @@ void ResizableArrayData::Clear(bool release_buffers) {

 Status ResizableArrayData::ResizeFixedLengthBuffers(int num_rows_new) {
  ARROW_DCHECK(num_rows_new >= 0);
+  ARROW_DCHECK(data_type_ != nullptr);


Why do we need this (and several other similar) check? I think we can assume that these functions will always be called after an Init call, where data_type_ would be set to a non-null.

Rather we should add such a check in Init instead: the first use of passed data_type in Init is:
https://github.com/apache/arrow/pull/43760/files/990e9a15042a31fa8e9cfc07cb62da65cd11092e..87b5d061a8fe2c027b29335b07d2c66969688a23#diff-dca2a0b71d9a10c8634649df065d2631cadf93e3a05eefa5b83ae9d55348b63fR219
will give NPE.

Why do we need this (and several other similar) check? I think we can assume that these functions will always be called after an Init call, where data_type_ would be set to a non-null.

I just DCHECK(data_type_) to DCHECK it's initialized

It just feels weird to ensure having been initialized by checking a not-so-relative member data_type_. But OK I get it. If so, please also check the passed-in data_type of Init being non-null - you can see that we don't have any guard on its nullability, so we may end up with NPE.

It just feels weird to ensure having been initialized by checking a not-so-relative member data_type_.

Since we don't have initialized, lol

Added in b2223ff

I agree with @zanmato1984 that this doesn't seem terribly useful.

zanmato1984

Rest of changes LGTM though.

pitrou · 2024-08-29T10:44:48Z

cpp/src/arrow/compute/light_array_internal.cc

    ARROW_DCHECK(metadata_before.is_fixed_length == metadata_after.is_fixed_length &&
                 metadata_before.fixed_length == metadata_after.fixed_length);
  }
 #endif
+  ARROW_DCHECK(data_type != nullptr);


Can you avoid duplicating this line?

pitrou · 2024-08-29T10:46:53Z

cpp/src/arrow/compute/light_array_internal.cc

@@ -235,6 +237,7 @@ void ResizableArrayData::Clear(bool release_buffers) {

 Status ResizableArrayData::ResizeFixedLengthBuffers(int num_rows_new) {
  ARROW_DCHECK(num_rows_new >= 0);
+  ARROW_DCHECK(data_type_ != nullptr);


I agree with @zanmato1984 that this doesn't seem terribly useful.

conbench-apache-arrow · 2024-08-29T23:08:05Z

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

There were no benchmark performance regressions. 🎉

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

…43760) ### Rationale for this change Minor style enhancement for join ### What changes are included in this PR? Minor style enhancement for join ### Are these changes tested? Covered by existing ### Are there any user-facing changes? no * GitHub Issue: apache#43759 Authored-by: mwish <[email protected]> Signed-off-by: mwish <[email protected]>

apacheGH-43759: [C++] Acero: Minor code enhancement for Join

990e9a1

mapleFU requested a review from westonpace as a code owner August 19, 2024 19:58

github-actions bot added Component: C++ awaiting review Awaiting review labels Aug 19, 2024

mapleFU requested review from zanmato1984 and pitrou August 20, 2024 02:51

zanmato1984 approved these changes Aug 20, 2024

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Aug 20, 2024

pitrou reviewed Aug 20, 2024

View reviewed changes

zanmato1984 reviewed Aug 28, 2024

View reviewed changes

Update: return Status for some places

87b5d06

mapleFU requested a review from zanmato1984 August 28, 2024 11:31

zanmato1984 reviewed Aug 28, 2024

View reviewed changes

ci fix

c066b2b

zanmato1984 approved these changes Aug 29, 2024

View reviewed changes

Apply suggestions

b2223ff

mapleFU requested a review from pitrou August 29, 2024 04:48

pitrou reviewed Aug 29, 2024

View reviewed changes

remove data_type_ checking

428efdf

mapleFU requested a review from pitrou August 29, 2024 14:58

pitrou approved these changes Aug 29, 2024

View reviewed changes

mapleFU merged commit 4f91c8f into apache:main Aug 29, 2024
37 of 40 checks passed

mapleFU deleted the minor-join-stype-enhancement branch August 29, 2024 15:38

mapleFU removed the awaiting committer review Awaiting committer review label Aug 29, 2024

mapleFU mentioned this pull request Aug 29, 2024

[C++] Acero: Minor code enhancement for Join #43759

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-43759: [C++] Acero: Minor code enhancement for Join #43760

GH-43759: [C++] Acero: Minor code enhancement for Join #43760

mapleFU commented Aug 19, 2024 •

edited by github-actions bot

Loading

github-actions bot commented Aug 19, 2024

zanmato1984 left a comment

pitrou Aug 20, 2024

pitrou Aug 20, 2024

mapleFU Aug 20, 2024

mapleFU Aug 28, 2024

pitrou Aug 28, 2024

zanmato1984 Aug 28, 2024

zanmato1984 left a comment

zanmato1984 Aug 28, 2024

zanmato1984 Aug 28, 2024

mapleFU commented Aug 28, 2024

zanmato1984 Aug 28, 2024

mapleFU Aug 28, 2024

zanmato1984 Aug 29, 2024

mapleFU Aug 29, 2024

mapleFU Aug 29, 2024

pitrou Aug 29, 2024

zanmato1984 left a comment

pitrou Aug 29, 2024

mapleFU Aug 29, 2024

pitrou Aug 29, 2024

conbench-apache-arrow bot commented Aug 29, 2024

	return KeyColumnArray(column_metadata, num_rows_,
	return KeyColumnArray(metadata_, num_rows_,

GH-43759: [C++] Acero: Minor code enhancement for Join #43760

GH-43759: [C++] Acero: Minor code enhancement for Join #43760

Conversation

mapleFU commented Aug 19, 2024 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Aug 19, 2024

zanmato1984 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanmato1984 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mapleFU commented Aug 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanmato1984 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conbench-apache-arrow bot commented Aug 29, 2024

mapleFU commented Aug 19, 2024 •

edited by github-actions bot

Loading