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

feat: extensions_options macro #5442

Merged
merged 2 commits into from
Mar 3, 2023

Conversation

crepererum
Copy link
Contributor

Which issue does this PR close?

-

Rationale for this change

I was implementing ExtensionsOptions in InfluxDB IOx and it was quite cumbersome, since you need to be very careful to keep all the set/entries methods in sync with the structure definition. So I wrote a macro that does a similar magic than the DF builtin configs. I would like to upstream this since I think it actually makes config extensions usable.

What changes are included in this PR?

A new macro.

Are these changes tested?

Small "does it expand" test.

Are there any user-facing changes?

New macro.

@github-actions github-actions bot added the core Core DataFusion crate label Mar 1, 2023
@crepererum crepererum force-pushed the crepererum/extensions_options_macro branch from a175acf to 3ba4d39 Compare March 1, 2023 14:17
Copy link
Member

@waynexia waynexia left a comment

Choose a reason for hiding this comment

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

Looks good to me!

The default value might be better with an attribute macro, but it would be much more complicated. This one looks pretty enough 👍

@crepererum
Copy link
Contributor Author

The issue with prog macros (which I think is what you mean with "attribute macros") is: they are slower, more complicated to write & test, and you ideally have a trait just for the prog macro because they are compiled just to check/document the downstream and you wanna avoid that for costly crates.

If you meant "can you just use a macro syntax for defaults"? Ahh, maybe. If there's strong desire to do that, I can try (although I'm not sure if traditional macros can match on macro-like attributes).

@waynexia
Copy link
Member

waynexia commented Mar 1, 2023

The issue with prog macros (which I think is what you mean with "attribute macros") is: they are slower, more complicated to write & test, and you ideally have a trait just for the prog macro because they are compiled just to check/document the downstream and you wanna avoid that for costly crates.

Agree, I tried one recently and it does take me lots of time to debug 😢

If you meant "can you just use a macro syntax for defaults"? Ahh, maybe. If there's strong desire to do that, I can try (although I'm not sure if traditional macros can match on macro-like attributes).

As long as the current implementation works fine and does not have ambiguous grammar, we can keep using it. And to me it's straitforward 👍

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Other than the location of the test I think this one is ready to go -- thank you @crepererum

For anyone following along, the config was added to IOx in https://github.com/influxdata/influxdb_iox/pull/7020

datafusion/core/tests/config.rs Outdated Show resolved Hide resolved
@crepererum crepererum force-pushed the crepererum/extensions_options_macro branch from 3ba4d39 to e6ae53e Compare March 3, 2023 09:36
@github-actions github-actions bot removed the core Core DataFusion crate label Mar 3, 2023
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @crepererum -- this is really nice. Thank you @waynexia for the review

fn main() {
// set up config struct and register extension
let mut config = ConfigOptions::default();
config.extensions.insert(MyConfig::default());
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

@@ -634,3 +634,129 @@ trait Visit {

fn none(&mut self, key: &str, description: &'static str);
}

/// Convenience macro to create [`ExtensionsOptions`].
Copy link
Contributor

Choose a reason for hiding this comment

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

giphy

@alamb alamb merged commit e985207 into apache:main Mar 3, 2023
@ursabot
Copy link

ursabot commented Mar 3, 2023

Benchmark runs are scheduled for baseline = ac618f0 and contender = e985207. e985207 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
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

@andygrove andygrove added the enhancement New feature or request label Mar 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants