add time_zone into ConfigOptions by waitingkuo · Pull Request #3485 · apache/datafusion

waitingkuo · 2022-09-14T19:12:01Z

Which issue does this PR close?

Rationale for this change

some of the function requires session timezone to work
e.g. EXTRACT(HOUR FROM SOME_TIME) need this

as SHOW statement is done in #3455
we could do this now

❯ show datafusion.execution.time_zone;
datafusion.execution.time_zone
+--------------------------------+---------+
| name                           | setting |
+--------------------------------+---------+
| datafusion.execution.time_zone | UTC     |
+--------------------------------+---------+
1 row in set. Query took 0.004 seconds.

I also added the shortcut to align with postgrseql

❯ show timezone;
timezone
+--------------------------------+---------+
| name                           | setting |
+--------------------------------+---------+
| datafusion.execution.time_zone | UTC     |
+--------------------------------+---------+
1 row in set. Query took 0.004 seconds.

❯ show time zone;
time.zone
+--------------------------------+---------+
| name                           | setting |
+--------------------------------+---------+
| datafusion.execution.time_zone | UTC     |
+--------------------------------+---------+
1 row in set. Query took 0.004 seconds.

What changes are included in this PR?

Are there any user-facing changes?

waitingkuo · 2022-09-14T19:14:09Z

@avantgardnerio @liukun4515

waitingkuo · 2022-09-14T19:15:44Z

+    pub fn new_string(
+        key: impl Into<String>,
+        description: impl Into<String>,
+        default_value: String,
+    ) -> Self {
+        Self::new(
+            key,
+            description,
+            DataType::UInt64,
+            ScalarValue::Utf8(Some(default_value)),
+        )
+    }
 }


add a new_string method for adding String Type configuration

waitingkuo · 2022-09-14T19:19:09Z

+        } else if variable_lower == "timezone" || variable_lower == "time.zone" {
+            // we could introduce alias in OptionDefinition if this string matching thing grows
+            String::from("SELECT name, setting FROM information_schema.df_settings WHERE name = 'datafusion.execution.time_zone'")


if we have more and more alias like this, we could consider add aliases into ConfigDefinition

codecov-commenter · 2022-09-14T19:43:32Z

Codecov Report

Merging #3485 (327094c) into master (f3bb84f) will increase coverage by 0.00%.
The diff coverage is 82.60%.

@@           Coverage Diff           @@
##           master    #3485   +/-   ##
=======================================
  Coverage   85.75%   85.76%           
=======================================
  Files         299      299           
  Lines       55273    55311   +38     
=======================================
+ Hits        47402    47435   +33     
- Misses       7871     7876    +5

Impacted Files	Coverage Δ
datafusion/core/src/config.rs	`89.65% <42.85%> (-4.10%)`	⬇️
datafusion/core/tests/sql/information_schema.rs	`98.76% <100.00%> (+0.06%)`	⬆️
datafusion/sql/src/planner.rs	`80.97% <100.00%> (+0.02%)`	⬆️
...n/core/src/physical_plan/file_format/row_filter.rs	`87.30% <0.00%> (-0.31%)`	⬇️
datafusion/common/src/scalar.rs	`85.11% <0.00%> (-0.07%)`	⬇️
datafusion/proto/src/lib.rs	`94.14% <0.00%> (+<0.01%)`	⬆️
datafusion/proto/src/from_proto.rs	`39.93% <0.00%> (+0.06%)`	⬆️
datafusion/expr/src/logical_plan/plan.rs	`78.01% <0.00%> (+0.16%)`	⬆️
datafusion/core/src/physical_plan/metrics/value.rs	`87.43% <0.00%> (+0.50%)`	⬆️
...atafusion/physical-expr/src/expressions/in_list.rs	`85.73% <0.00%> (+0.53%)`	⬆️
... and 2 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

alamb

Other than the errant println! this looks good to me -- thanks @waitingkuo

alamb · 2022-09-14T19:53:26Z


-        let query = if variable.to_lowercase() == "all" {
+        let variable_lower = variable.to_lowercase();
+        println!("{}", variable_lower);


Perhaps is this a left over debugging println!?

my fault, let me push the fix

@alamb fixed

alamb · 2022-09-14T20:06:18Z

Thanks @waitingkuo

Co-authored-by: Kun Liu <liukun@apache.org>

waitingkuo · 2022-09-15T15:09:34Z

thank you @liukun4515

ursabot · 2022-09-15T17:13:02Z

Benchmark runs are scheduled for baseline = 6ffda68 and contender = 3319220. 3319220 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

@overload

add time_zone into ConfigOptions

becccea

github-actions Bot added core Core DataFusion crate sql SQL Planner labels Sep 14, 2022

waitingkuo commented Sep 14, 2022

View reviewed changes

alamb approved these changes Sep 14, 2022

View reviewed changes

fix debug leftover

8b24c3c

alamb approved these changes Sep 14, 2022

View reviewed changes

liukun4515 reviewed Sep 15, 2022

View reviewed changes

Comment thread datafusion/core/src/config.rs Outdated

Update datafusion/core/src/config.rs

327094c

Co-authored-by: Kun Liu <liukun@apache.org>

alamb merged commit 3319220 into apache:master Sep 15, 2022

Navigation Menu

Uh oh!

add time_zone into ConfigOptions - #3485

add time_zone into ConfigOptions#3485
alamb merged 3 commits into
apache:masterfrom
waitingkuo:show-timezone

waitingkuo commented Sep 14, 2022 •

edited

Loading

waitingkuo commented Sep 14, 2022

waitingkuo Sep 14, 2022

waitingkuo Sep 14, 2022

codecov-commenter commented Sep 14, 2022 •

edited

Loading

alamb left a comment

alamb Sep 14, 2022

waitingkuo Sep 14, 2022

waitingkuo Sep 14, 2022

alamb commented Sep 14, 2022

Uh oh!

waitingkuo commented Sep 15, 2022

ursabot commented Sep 15, 2022

Labels

5 participants

Uh oh!

Conversation

waitingkuo commented Sep 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

waitingkuo commented Sep 14, 2022

waitingkuo Sep 14, 2022

Choose a reason for hiding this comment

waitingkuo Sep 14, 2022

Choose a reason for hiding this comment

codecov-commenter commented Sep 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

alamb left a comment

Choose a reason for hiding this comment

alamb Sep 14, 2022

Choose a reason for hiding this comment

waitingkuo Sep 14, 2022

Choose a reason for hiding this comment

waitingkuo Sep 14, 2022

Choose a reason for hiding this comment

alamb commented Sep 14, 2022

Uh oh!

waitingkuo commented Sep 15, 2022

ursabot commented Sep 15, 2022

Labels

5 participants

waitingkuo commented Sep 14, 2022 •

edited

Loading

codecov-commenter commented Sep 14, 2022 •

edited

Loading