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

Add SessionConfig reference to ScalarFunctionArgs #13519

Open
alamb opened this issue Nov 21, 2024 · 9 comments · May be fixed by #13527
Open

Add SessionConfig reference to ScalarFunctionArgs #13519

alamb opened this issue Nov 21, 2024 · 9 comments · May be fixed by #13527
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@alamb
Copy link
Contributor

alamb commented Nov 21, 2024

Is your feature request related to a problem or challenge?

@Omega359 noted that by adding SessionConfig to the ScalarFunctionArgs unblock several tasks such as

Describe the solution you'd like

Add &SessionConfig to ScalarFunctionArgs, and ideally add a test that shows the config gets through.

Describe alternatives you've considered

No response

Additional context

We should try and get this done before DataFusion 44 is released so it isn't a breaking change

@Omega359
Copy link
Contributor

Well, small issue. ScalarFunctionArgs is in datafusion-expr which can't use SessionConfig directly since it's in datafusion-execution which depends on datafusion-expr which would cause a circular dependency. We can use ConfigOptions there which would give us access to all config except the opaque extensions in SessionConfig which I think is acceptable.

@niebayes
Copy link
Contributor

Great work!

@Omega359
Copy link
Contributor

Started delving into this trying to find a good way to impl. Trying to add config_options: ConfigOptions to ScalarFunctionExpr Having lots of fun with eq and hash with SessionConfig and f64 atm.

@alamb
Copy link
Contributor Author

alamb commented Nov 21, 2024

I wonder if we can use this trait: https://docs.rs/datafusion/latest/datafusion/catalog/trait.Session.html

@Omega359
Copy link
Contributor

We could but it wouldn't solve the issue. The issue is PhysicalExpr requires implementations to impl Eq and Hash or to have implementations for DynEq and DynHash. That is fine until something like SessionConfig which doesn't is introduced. I am looking at updating the either the 'config_namespace' macro or add explicit implementations for 'ScalarFunctionExpr' to try and impl either of the above and use f64.to_bits() and f64::from_bits(..) to handle the problematic f64's in the config. I think it's possible

@jayzhan211
Copy link
Contributor

jayzhan211 commented Nov 22, 2024

It makes sense to me to move SessionConfig to common or common-runtime crate

@Omega359
Copy link
Contributor

take

@alamb
Copy link
Contributor Author

alamb commented Nov 22, 2024

It makes sense to me to move SessionConfig to common or common-runtime crate

I agree

@Omega359
Copy link
Contributor

Omega359 commented Nov 22, 2024

I mistyped above - I meant we couldn't use SessionContext, not SessionConfig. Sorry about the confusion. ... or not. I need more caffeine today. PR uses ConfigOptions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants