Omega359 commented on code in PR #18408:
URL: https://github.com/apache/datafusion/pull/18408#discussion_r2484915684
##########
datafusion/sqllogictest/test_files/set_variable.slt:
##########
@@ -244,3 +244,105 @@ SET TIME ZONE = 'Asia/Taipei2'
statement error Arrow error: Parser error: Invalid timezone "Asia/Taipei2":
failed to parse timezone
SELECT '2000-01-01T00:00:00'::TIMESTAMP::TIMESTAMPTZ
+
+# reset variable restores default
+statement ok
+set datafusion.catalog.information_schema = true
+
+statement ok
+SET datafusion.execution.batch_size = 1024
+
+query TT
+SHOW datafusion.execution.batch_size
+----
+datafusion.execution.batch_size 1024
+
+statement ok
+RESET datafusion.execution.batch_size
+
+query TT
+SHOW datafusion.execution.batch_size
+----
+datafusion.execution.batch_size 8192
+
+# reset variable with NULL default
+statement ok
+set datafusion.catalog.information_schema = true
+
+statement ok
+SET datafusion.execution.parquet.max_predicate_cache_size = '123'
+
+query TT
+SHOW datafusion.execution.parquet.max_predicate_cache_size
+----
+datafusion.execution.parquet.max_predicate_cache_size 123
+
+statement ok
+RESET datafusion.execution.parquet.max_predicate_cache_size
+
+query TT
+SHOW datafusion.execution.parquet.max_predicate_cache_size
+----
+datafusion.execution.parquet.max_predicate_cache_size NULL
+
+# reset time zone via aliases
+statement ok
+set datafusion.catalog.information_schema = true
+
+statement ok
+SET TIME ZONE = '+10:00'
+
+statement ok
+RESET TIME ZONE
+
+query TT
+SHOW TIME ZONE
+----
+datafusion.execution.time_zone +00:00
+
+statement ok
+SET TIMEZONE = '-03:00'
+
+statement ok
+RESET TIMEZONE
+
+query TT
+SHOW TIMEZONE
+----
+datafusion.execution.time_zone +00:00
+
+statement ok
+SET TIME ZONE = '+09:00'
+
+statement ok
+RESET timezone
+
+query TT
+SHOW TIME ZONE
+----
+datafusion.execution.time_zone +00:00
+
+# reset runtime variables
+statement ok
+SET datafusion.runtime.memory_limit = '1M'
+
+statement ok
+RESET datafusion.runtime.memory_limit
Review Comment:
where is the show to verify it was indeed reset?
##########
datafusion/common/src/config.rs:
##########
@@ -2523,7 +2647,7 @@ impl ConfigField for ConfigFileDecryptionProperties {
self.footer_signature_verification.set(rem, value.as_ref())
}
_ => _config_err!(
- "Config value \"{}\" not found on
ConfigFileEncryptionProperties",
+ "Config value \"{}\" not found on
ConfigFileDecryptionProperties",
Review Comment:
Nice catch
##########
datafusion/common/src/config.rs:
##########
@@ -1515,6 +1626,19 @@ macro_rules! config_field {
*self = $transform;
Ok(())
}
+
+ fn reset(&mut self, key: &str) -> $crate::error::Result<()> {
+ if key.is_empty() {
Review Comment:
This seems off. Why error if there is a key but don't if there isn't? Seems
backwards?
##########
datafusion/common/src/config.rs:
##########
@@ -1515,6 +1626,19 @@ macro_rules! config_field {
*self = $transform;
Ok(())
}
+
+ fn reset(&mut self, key: &str) -> $crate::error::Result<()> {
+ if key.is_empty() {
+ *self = <$t as Default>::default();
+ Ok(())
+ } else {
+ $crate::error::_config_err!(
+ "Config value \"{}\" not found on {}",
Review Comment:
This error message doesn't make sense in this context. We also should have a
test to cover this error
##########
datafusion/common/src/config.rs:
##########
@@ -1130,6 +1155,79 @@ impl ConfigField for ConfigOptions {
self.sql_parser.visit(v, "datafusion.sql_parser", "");
self.format.visit(v, "datafusion.format", "");
}
+
+ /// Reset a configuration option back to its default value
+ fn reset(&mut self, key: &str) -> Result<()> {
+ let Some((prefix, rest)) = key.split_once('.') else {
+ return _config_err!("could not find config namespace for key
\"{key}\"");
+ };
+
+ if prefix != "datafusion" {
+ return _config_err!("Could not find config namespace
\"{prefix}\"");
+ }
+
+ let (section, rem) = rest.split_once('.').unwrap_or((rest, ""));
+ match section {
+ "catalog" => {
+ if rem.is_empty() {
+ self.catalog = CatalogOptions::default();
Review Comment:
I am uncertain that this should be supported to be honest. I think that a
full configuration key should be required for reset.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
