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.

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

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

Closed
js8544 opened this issue Jan 11, 2023 · 1 comment · Fixed by #33608
Closed

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

js8544 opened this issue Jan 11, 2023 · 1 comment · Fixed by #33608

Comments

@js8544
Copy link
Collaborator

js8544 commented Jan 11, 2023

Describe the enhancement requested

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));

Component(s)

C++

js8544 added a commit to js8544/arrow that referenced this issue Jan 11, 2023
pitrou pushed a commit that referenced this issue Jan 16, 2023
…t functions (#33608)

# 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:
```cpp
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:
```cpp
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](https://github.com/apache/arrow/pull/33608/files#diff-ef9d394f251ee719f9896b864246bd97a81165c08195d4846d5d99e589e5c1c0L402), `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.

Authored-by: Jin Shang <shangjin1997@gmail.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@pitrou pitrou added this to the 11.0.0 milestone Jan 16, 2023
@pitrou
Copy link
Member

pitrou commented Jan 16, 2023

Issue resolved by pull request 33608
#33608

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants