alamb commented on code in PR #2754: URL: https://github.com/apache/arrow-datafusion/pull/2754#discussion_r903766051
########## datafusion/core/src/config.rs: ########## @@ -0,0 +1,187 @@ +// 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. + +//! DataFusion Configuration Options + +use datafusion_common::ScalarValue; +use sqlparser::ast::DataType; +use std::collections::HashMap; + +/// Configuration option "datafusion.optimizer.filterNullJoinKeys" +pub const OPT_FILTER_NULL_JOIN_KEYS: &str = "datafusion.optimizer.filterNullJoinKeys"; + +/// Definition of a configuration option +pub struct ConfigDefinition { + /// key used to identifier this configuration option + key: String, + /// Description to be used in generated documentation + description: String, + /// Data type of this option + data_type: DataType, + /// Default value + default_value: ScalarValue, +} + +impl ConfigDefinition { + /// Create a configuration option definition + pub fn new( + name: &str, + description: &str, Review Comment: ```suggestion name: impl Into<String>, description: impl Into<String>, ``` And then you can change `name.to_string()` to `name.into()` and `description.to_string()` into `description.into()` It is a minor thing, but that way the caller can pass in a `String` if they have it and avoid the extra copy 🤷 ########## datafusion/core/src/config.rs: ########## @@ -0,0 +1,187 @@ +// Licensed to the Apache Software Foundation (ASF) under one Review Comment: If this module is in `datafusion-core` it will not be able to be used in other modules directly. That seems ok for now 🤔 but I haven't fully thought about it ########## datafusion/core/src/config.rs: ########## @@ -0,0 +1,187 @@ +// 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. + +//! DataFusion Configuration Options + +use datafusion_common::ScalarValue; +use sqlparser::ast::DataType; +use std::collections::HashMap; + +/// Configuration option "datafusion.optimizer.filterNullJoinKeys" Review Comment: using `.` for namespacing the configuration makes sense to me ########## datafusion/core/src/config.rs: ########## @@ -0,0 +1,187 @@ +// 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. + +//! DataFusion Configuration Options + +use datafusion_common::ScalarValue; +use sqlparser::ast::DataType; Review Comment: I think using `arrow::data_types::DataType` for the type would be easier to understand (as that is the DataType used by `ScalarValue`). -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
