Skip to content

GH-33607: [C++] Support optional additional arguments for inline visit functions - #33608

Merged
pitrou merged 4 commits into
apache:masterfrom
js8544:jinshang/additional_args_for_visit_functions
Jan 16, 2023
Merged

GH-33607: [C++] Support optional additional arguments for inline visit functions#33608
pitrou merged 4 commits into
apache:masterfrom
js8544:jinshang/additional_args_for_visit_functions

Conversation

@js8544

@js8544 js8544 commented Jan 11, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes #33607

Rationale for this change

Sometimes we need extra arguments for the Visit function, the most common of which is output parameters to save the result. For now, we need to have a member variable and a member function to get the result:

class ExampleVisitor {
 public:
  template <typename T>
  Status Visit(const T& arr) {
    /// Do stuff and save result to output_;
    return Status::OK();
  }

  std::shared_ptr<Array> GetOutput() { return output_; }

 private:
  std::shared_ptr<Array> output_;
};

ExampleVisitor visitor;
RETURN_NOT_OK(VisitArrayInline(*arr, &visitor));
*output = visitor.GetOutput();

It will be more convenient to write a Visitor if the VisitArrayInline function supports additional args for the Visit method:

class ExampleVisitorWithArg {
 public:
  template <typename T>
  Status Visit(const T& arr, std::shared_ptr<Array>* output) {
    /// Do stuff and save result to output directly;
    return Status::OK();
  }
};

ExampleVisitorWithArg visitor;
RETURN_NOT_OK(VisitArrayInline(*arr, &visitor, &output));

Are these changes tested?

Not sure if we need to explicitly test this feature, since no runtime behavior is changed. The existing tests all pass.

Are there any user-facing changes?

There is a small chance, like in diff.cc:402, VisitTypeInline is declared as a friend function. The declaration needs to be changed. But I am not sure if any outside user would do this.

@github-actions

github-actions Bot commented Jan 11, 2023

Copy link
Copy Markdown

@github-actions

github-actions Bot commented Jan 11, 2023

Copy link
Copy Markdown

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

@js8544 js8544 changed the title GH-33607: [C++] Support optional additional arguments for inline visi… GH-33607: [C++] Support optional additional arguments for inline visit functions Jan 11, 2023
pitrou
pitrou reviewed Jan 12, 2023
View reviewed changes
Comment thread cpp/src/arrow/visit_array_inline.h Outdated
internal::checked_cast<const typename TypeTraits<TYPE_CLASS##Type>::ArrayType&>( \
array));
array), \
args...);

@pitrou pitrou Jan 12, 2023

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.

Wouldn't it be better to use something like std::forward<decltype(args)>?

@js8544 js8544 Jan 12, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Definitely! Changed to std::forward

@pitrou

pitrou commented Jan 12, 2023

Copy link
Copy Markdown
Member

Ok, is it possible to exploit this new possibility in some select places? It will validate that it works and also show the benefits.

(you can of course grep through the codebase to find candidates)

pitrou
pitrou reviewed Jan 12, 2023
View reviewed changes
Comment thread cpp/src/arrow/visit_type_inline.h Outdated
break;
}
return std::forward<VISITOR>(visitor)(type);
return std::forward<VISITOR>(visitor)(type, args...);

@pitrou pitrou Jan 12, 2023

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.

Should forward here and below as well.

@js8544 js8544 Jan 13, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

@js8544

js8544 commented Jan 13, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Ok, is it possible to exploit this new possibility in some select places? It will validate that it works and also show the benefits.

(you can of course grep through the codebase to find candidates)

Good point. I've applied the new VisitArrayInlinein column_writer.cc, which saves a copy construction of std::shared_ptr<Buffer>. And VisitTypeInline in builder.h, which eliminates the need for a wrapper for the visitor. Unfortunately I couldn't find a suitable place to apply VisitScalarInline as it is only occasionally used and none of them are suitable for this new feature.

@js8544
js8544 requested a review from pitrou January 13, 2023 02:11
pitrou
pitrou approved these changes Jan 16, 2023
View reviewed changes

@pitrou pitrou 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, thank you @js8544

@pitrou
pitrou merged commit 4b48ecc into apache:master Jan 16, 2023
@pitrou pitrou mentioned this pull request Jan 16, 2023
Closed
@ursabot

ursabot commented Jan 16, 2023

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = 4d9228f and contender = 4b48ecc. 4b48ecc is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.18% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.28% ⬆️0.09%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 4b48eccc ec2-t3-xlarge-us-east-2
[Failed] 4b48eccc test-mac-arm
[Finished] 4b48eccc ursa-i9-9960x
[Finished] 4b48eccc ursa-thinkcentre-m75q
[Finished] 4d9228fd ec2-t3-xlarge-us-east-2
[Finished] 4d9228fd test-mac-arm
[Finished] 4d9228fd ursa-i9-9960x
[Finished] 4d9228fd ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@sighingnow sighingnow mentioned this pull request May 8, 2023
Merged
sighingnow added a commit to v6d-io/v6d that referenced this pull request May 8, 2023
@sighingnow
Adapt to arrow-11/12 (VisitTypeInline & Run-End encoded data) (#1362)
eef604f 
Related issue number
--------------------

See also: 
- Homebrew/homebrew-core#129859
- apache/arrow#33608
- apache/arrow#33641

Signed-off-by: Tao He <[email protected]>
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++ Component: Parquet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[C++] Support optional additional arguments for inline visit functions

3 participants

@js8544 @pitrou @ursabot