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++] Allow Substrait's default extension provider to be configured #33850

Closed
westonpace opened this issue Jan 24, 2023 · 8 comments · Fixed by #34042 or #34075
Closed

[C++] Allow Substrait's default extension provider to be configured #33850

westonpace opened this issue Jan 24, 2023 · 8 comments · Fixed by #34042 or #34075

Comments

@westonpace
Copy link
Member

Describe the enhancement requested

A custom extension provider can be specified when running a query. However, in some cases (e.g. when Acero is embedded in a larger program that provides its own extension handlers) it would be simpler to change the default instead of having to specify the custom provider with every query run.

Component(s)

C++

@rtpsw
Copy link
Contributor

rtpsw commented Feb 1, 2023

take

@rtpsw
Copy link
Contributor

rtpsw commented Feb 3, 2023

@westonpace, @icexelloss, this issue may be a no-op, because ExtensionProvider::kDefaultExtensionProvider is public and can already be directly set. We could have some kind of guarding around its setting, if desired.

@icexelloss
Copy link
Contributor

icexelloss commented Feb 3, 2023

IMO kDefaultExtensionProvider is not a public API so we should not change it directly, even if it works today there is no guarantee it will always work after some refactoring. The API can be as simple as changing that variable under the hood but we still need a nice public API for it.

@westonpace
Copy link
Member Author

I agree with @icexelloss . We should move kDefaultExtensionProvider into the .cc file and/or make it const.

@rtpsw
Copy link
Contributor

rtpsw commented Feb 3, 2023

We're on the same page on that. My question is whether we need anything more, like some kind of guard, in the implementation of the API for setting.

@westonpace
Copy link
Member Author

You mean like a mutex / lock_guard? Yes, that would be a good idea. Or am I misunderstanding?

@rtpsw
Copy link
Contributor

rtpsw commented Feb 3, 2023

Yes, and wanted to make sure there's nothing else needed that I might be missing.

@westonpace
Copy link
Member Author

I think that would be sufficient.

rtpsw added a commit to rtpsw/arrow that referenced this issue Feb 4, 2023
westonpace pushed a commit that referenced this issue Feb 6, 2023
…nfigured (#34042)

See #33850
* Closes: #33850

Authored-by: Yaron Gvili <rtpsw@hotmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
@westonpace westonpace added this to the 12.0.0 milestone Feb 6, 2023
rtpsw added a commit to rtpsw/arrow that referenced this issue Feb 8, 2023
westonpace pushed a commit that referenced this issue Feb 8, 2023
…nfigured (fix) (#34075)

See [this post](#34042 (comment)) for background.
* Closes: #33850

Authored-by: Yaron Gvili <rtpsw@hotmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
sjperkins pushed a commit to sjperkins/arrow that referenced this issue Feb 10, 2023
… be configured (apache#34042)

See apache#33850
* Closes: apache#33850

Authored-by: Yaron Gvili <rtpsw@hotmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
sjperkins pushed a commit to sjperkins/arrow that referenced this issue Feb 10, 2023
… be configured (fix) (apache#34075)

See [this post](apache#34042 (comment)) for background.
* Closes: apache#33850

Authored-by: Yaron Gvili <rtpsw@hotmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
gringasalpastor pushed a commit to gringasalpastor/arrow that referenced this issue Feb 17, 2023
… be configured (apache#34042)

See apache#33850
* Closes: apache#33850

Authored-by: Yaron Gvili <rtpsw@hotmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
gringasalpastor pushed a commit to gringasalpastor/arrow that referenced this issue Feb 17, 2023
… be configured (fix) (apache#34075)

See [this post](apache#34042 (comment)) for background.
* Closes: apache#33850

Authored-by: Yaron Gvili <rtpsw@hotmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
fatemehp pushed a commit to fatemehp/arrow that referenced this issue Feb 24, 2023
… be configured (apache#34042)

See apache#33850
* Closes: apache#33850

Authored-by: Yaron Gvili <rtpsw@hotmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
fatemehp pushed a commit to fatemehp/arrow that referenced this issue Feb 24, 2023
… be configured (fix) (apache#34075)

See [this post](apache#34042 (comment)) for background.
* Closes: apache#33850

Authored-by: Yaron Gvili <rtpsw@hotmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment