You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "crepererum (via GitHub)" <gi...@apache.org> on 2023/03/01 14:09:00 UTC
[GitHub] [arrow-datafusion] crepererum opened a new pull request, #5442: feat: `extensions_options` macro
crepererum opened a new pull request, #5442:
URL: https://github.com/apache/arrow-datafusion/pull/5442
# 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] waynexia commented on pull request #5442: feat: `extensions_options` macro
Posted by "waynexia (via GitHub)" <gi...@apache.org>.
waynexia commented on PR #5442:
URL: https://github.com/apache/arrow-datafusion/pull/5442#issuecomment-1450473340
>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 :cry:
>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 :+1:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] ursabot commented on pull request #5442: feat: `extensions_options` macro
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #5442:
URL: https://github.com/apache/arrow-datafusion/pull/5442#issuecomment-1454155622
Benchmark runs are scheduled for baseline = ac618f034f566719f0a76c28608529b0a455a1c1 and contender = e9852074bacd8c891d84eba38b3417aa16a2d18c. e9852074bacd8c891d84eba38b3417aa16a2d18c is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/4900a46b6d0a48498313fa71c9c3335c...977b0d38725a4922b62045bc795c7081/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/fd73d158b96245d09a4797dd17ce01d6...36b853a1777e43279886f981d93e6c44/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/937465c8ba1e446eb400cdf0d9444a1f...c940739716a94489b331542dad6ae665/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/d80eba1756c8491580be3ebdbf0bda83...6c73023e726749888af2e086e803cec2/)
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb merged pull request #5442: feat: `extensions_options` macro
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #5442:
URL: https://github.com/apache/arrow-datafusion/pull/5442
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] crepererum commented on a diff in pull request #5442: feat: `extensions_options` macro
Posted by "crepererum (via GitHub)" <gi...@apache.org>.
crepererum commented on code in PR #5442:
URL: https://github.com/apache/arrow-datafusion/pull/5442#discussion_r1124222883
##########
datafusion/core/tests/config.rs:
##########
@@ -0,0 +1,30 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use datafusion::common::extensions_options;
+
+// The structure is not really used. We just check that the macro re-export works and everything referenced within the
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #5442: feat: `extensions_options` macro
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5442:
URL: https://github.com/apache/arrow-datafusion/pull/5442#discussion_r1123667727
##########
datafusion/core/tests/config.rs:
##########
@@ -0,0 +1,30 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use datafusion::common::extensions_options;
+
+// The structure is not really used. We just check that the macro re-export works and everything referenced within the
Review Comment:
Can you please move this code to https://github.com/apache/arrow-datafusion/tree/main/datafusion-examples/examples
That would also provide a good place maybe to provide some simple program showing how to use options created with this macro
Maybe we could add a small test showing that the configuration could be instantiated and used 🤔 As a way to try and document its use.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] crepererum commented on pull request #5442: feat: `extensions_options` macro
Posted by "crepererum (via GitHub)" <gi...@apache.org>.
crepererum commented on PR #5442:
URL: https://github.com/apache/arrow-datafusion/pull/5442#issuecomment-1450356790
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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #5442: feat: `extensions_options` macro
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5442:
URL: https://github.com/apache/arrow-datafusion/pull/5442#discussion_r1125029161
##########
datafusion-examples/examples/config_extension.rs:
##########
@@ -0,0 +1,52 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This example demonstrates how to extend the DataFusion configs with custom extensions.
+
+use datafusion::{
+ common::{config::ConfigExtension, extensions_options},
+ config::ConfigOptions,
+};
+
+extensions_options! {
+ /// My own config options.
+ pub struct MyConfig {
+ /// Should "foo" be replaced by "bar"?
+ pub foo_to_bar: bool, default = true
+
+ /// How many "baz" should be created?
+ pub baz_count: usize, default = 1337
+ }
+}
+
+impl ConfigExtension for MyConfig {
+ const PREFIX: &'static str = "my_config";
+}
+
+fn main() {
+ // set up config struct and register extension
+ let mut config = ConfigOptions::default();
+ config.extensions.insert(MyConfig::default());
Review Comment:
❤️
##########
datafusion/common/src/config.rs:
##########
@@ -634,3 +634,129 @@ trait Visit {
fn none(&mut self, key: &str, description: &'static str);
}
+
+/// Convenience macro to create [`ExtensionsOptions`].
Review Comment:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org