Move the to_timestamp* functions to datafusion-functions #9388

Omega359 · 2024-02-28T17:53:42Z

Which issue does this PR close?

Closes #9291

Rationale for this change

#9285

What changes are included in this PR?

code, tests

Are these changes tested?

Yes

Are there any user-facing changes?

Functions moved to new crate but are enabled by default.

# Conflicts: # datafusion/physical-expr/src/functions.rs # datafusion/proto/proto/datafusion.proto

Omega359 · 2024-02-28T17:55:55Z

datafusion/core/tests/simplification.rs

@@ -79,6 +86,43 @@ fn schema() -> DFSchema {
    .unwrap()
 }

+fn test_table_scan() -> LogicalPlan {


These tests were migrated from the optimizer/simplify_expressions file

Omega359 · 2024-02-28T17:57:09Z

datafusion/functions/benches/to_timestamp.rs

+        ];
+        b.iter(|| {
+            for i in inputs.iter() {
+                black_box(to_timestamp(vec![i.clone()]));


The benchmarks had to change to test the public api vs the previous version that tested the internal api.

I think testing the public API is actually a more real-world comparison (so this seems like an improvement to me)

Omega359 · 2024-02-28T17:58:40Z

datafusion/functions/src/datetime/mod.rs

+mod to_timestamp;
+
+// create UDFs
+make_udf_function!(to_timestamp::ToTimestampFunc, TO_TIMESTAMP, to_timestamp);


Because to_timestamp functions are nary functions the export_functions! macro could not be used. Instead this is the manual equivalent of what it expands out to.

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

# Conflicts: # datafusion/functions/Cargo.toml # datafusion/functions/src/lib.rs

alamb

Thanks @Omega359 -- this is looking pretty sweet

alamb · 2024-02-28T20:51:12Z

datafusion-examples/examples/expr_api.rs

-        "to_timestamp",
-        vec![lit("2020-09-08T12:00:00+00:00")],
-    )?);
+    let expr = col("ts").eq(to_timestamp(vec![lit("2020-09-08T12:00:00+00:00")]));


datafusion/core/tests/parquet/page_pruning.rs

datafusion/core/tests/parquet/row_group_pruning.rs

datafusion/core/tests/simplification.rs

datafusion/core/tests/sql/explain_analyze.rs

alamb · 2024-02-28T20:57:06Z

datafusion/functions/src/datetime/to_timestamp.rs

+const ERR_NANOSECONDS_NOT_SUPPORTED: &str = "The dates that can be represented as nanoseconds have to be between 1677-09-21T00:12:44.0 and 2262-04-11T23:47:16.854775804";
+
+#[derive(Debug)]
+pub(super) struct ToTimestampFunc {


These are all so similar, I wonder if we could make a common class to avoid the duplication something like

struct ToTimestampImpl { /// target datatype data_type: DataType, }

🤔

The ScalarUDFImpl is different for each 'version' of that struct. It would be possible to have a single struct but then the impl would have to match on the data_type (really in this case I think it might be timeunit). I'll do a test of that tonight to see if it's cleaner overall.

I don't think it is required, just a suggestion I thought of while reviewing this code

Ok. I just looked into it and it's actually more than the impl methods because it would affect the make_udf_function macro as well (or a clone) to allow args to be passed into the ::new() call. Possible for a future improvement

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

datafusion/optimizer/src/simplify_expressions/simplify_exprs.rs

alamb · 2024-02-28T21:01:09Z

datafusion/physical-expr/src/datetime_expressions.rs

 use datafusion_expr::ColumnarValue;

 use crate::expressions::cast_column;

-/// Error message if nanosecond conversion request beyond supported interval


this is great to extract all this code out of datafusion/physical-expr

alamb · 2024-02-28T21:03:35Z

datafusion/sqllogictest/test_files/functions.slt

@@ -483,7 +483,7 @@ statement error Did you mean 'arrow_typeof'?
 SELECT arrowtypeof(v1) from test;

 # Scalar function
-statement error Did you mean 'to_timestamp_seconds'?


🤔 this looks like there may be special case handling for function hinting that doesn't work for user defined functions.

I'll file a ticket to add that feature to user defined functions too

Filed #9392

Fixed in #9407

Co-authored-by: Andrew Lamb <[email protected]>

# Conflicts: # datafusion/functions/src/lib.rs

…timizer to core/tests.

Omega359 · 2024-02-29T03:28:04Z

datafusion/core/tests/simplification.rs

+    test_evaluate(
+        (lit("foo").not_eq(lit("foo"))).or(col("c").eq(lit(1))),
+        // lit(false).or(col("c").eq(lit(1))),
+        col("c").eq(lit(1)),


The comment was the version that was in expr_simplifier but that no longer passed the test once the test_evaluate_with_start_time function was changed to use ExprSimplifier from ConstEvaluator. I am unsure if this is in fact correct or not @alamb

This change looks better to me (false OR ...) is always (...).

The reason that the test changed is that previously it was only testing the constant propagation, but now it is also testing the simplifier code as well. I think this looks good

alamb

Thank you @Omega359 -- this looks just about perfect. There are a few tests I want to revert but I will handle doing so and push changes to this PR

alamb · 2024-02-29T21:20:29Z

datafusion/core/tests/simplification.rs

+    test_evaluate(
+        (lit("foo").not_eq(lit("foo"))).or(col("c").eq(lit(1))),
+        // lit(false).or(col("c").eq(lit(1))),
+        col("c").eq(lit(1)),


This change looks better to me (false OR ...) is always (...).

The reason that the test changed is that previously it was only testing the constant propagation, but now it is also testing the simplifier code as well. I think this looks good

datafusion/core/tests/simplification.rs

datafusion/core/tests/parquet/row_group_pruning.rs

alamb · 2024-02-29T21:23:53Z

datafusion/functions/benches/to_timestamp.rs

+        ];
+        b.iter(|| {
+            for i in inputs.iter() {
+                black_box(to_timestamp(vec![i.clone()]));


I think testing the public API is actually a more real-world comparison (so this seems like an improvement to me)

datafusion/functions/Cargo.toml

alamb

I am merging up from main to resolve conflicts in this PR now

alamb · 2024-02-29T21:41:49Z

I am merging up from main to resolve conflicts in this PR now

I tried with this for a bit but ran out of time. @Omega359 I think the issue is that the to_date function got added and needs to be ported as well as its implementation depends on the to_timestamp code.

Is this something you have time to do? If not, I will find time to do it myself.

Omega359 · 2024-02-29T21:53:11Z

I tried with this for a bit but ran out of time. @Omega359 I think the issue is that the to_date function got added and needs to be ported as well as its implementation depends on the to_timestamp code.

Is this something you have time to do? If not, I will find time to do it myself.

I can handle it, thanks for all your help. Likely tomorrow sometime.

alamb · 2024-02-29T21:58:33Z

BTW here is a proposed PR (targeting this PR) to avoid the new dependency: Omega359#1

Move `cast_column` to `ColumnarValue::cast_to`

# Conflicts: # datafusion/expr/src/expr_fn.rs # datafusion/functions/Cargo.toml # datafusion/physical-expr/src/datetime_expressions.rs

alamb

Thank you so much @Omega359 -- this is epic work

alamb · 2024-03-01T12:09:06Z

datafusion/functions/Cargo.toml

 log = "0.4.20"
+rand = { workspace = true }


I think several of these dependencies are only used in the tests (so they could be dev depenencies). ~~I'll make a follow on PR clean it up as this one is already really large.~~

Update I pushed a commit to move then to dev dependencies

alamb · 2024-03-01T12:12:28Z

I think the only thing left here is to resolve the merge conflicts (which is due to to_date). Otherwise this PR is ready to go. Thanks again @Omega359

…side to_timestamp.

# Conflicts: # datafusion/functions/Cargo.toml

Omega359 · 2024-03-01T17:15:39Z

datafusion/functions/src/datetime/common.rs

@@ -0,0 +1,389 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file


Moved functions common to to_timestamp and to_date to here

Omega359 · 2024-03-01T17:18:53Z

datafusion/functions/src/datetime/mod.rs

+pub mod expr_fn {
+    use datafusion_expr::Expr;
+
+    /// ```ignore


It's unfortunately but this rustdoc won't work as is because of the SessionContext::new() call is into a non-accessible module. I think we either need to pull examples out of rustdoc or find some other solution that doesn't involve circular dependencies. This is one of the more limiting aspects with rust documentation I've encountered.

I agree we probably need to pull them out of the rust doc directly (maybe we can put them in the examples 🤔 ) and maybe link to them from here

alamb

Awesome work @Omega359 -- thanks again. Let's merge and iterate in follow on PRs

Omega359 added 6 commits February 21, 2024 09:13

WIP

6ffb364

Merge remote-tracking branch 'upstream/main' into feature/9291

aa56090

WIP

b1868ef

Merge remote-tracking branch 'upstream/main' into feature/9291

aa1c5a6

# Conflicts: # datafusion/physical-expr/src/functions.rs # datafusion/proto/proto/datafusion.proto

cargo fmt updates.

d8e6016

Migrate to_timestamp* functions to new functions crate.

78baa96

github-actions bot added documentation Improvements or additions to documentation logical-expr Logical plan and expressions physical-expr Physical Expressions optimizer Optimizer rules core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Feb 28, 2024

Omega359 commented Feb 28, 2024

View reviewed changes

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs Outdated Show resolved Hide resolved

Omega359 added 4 commits February 28, 2024 13:04

Merge remote-tracking branch 'upstream/main' into feature/9291

45bea0f

# Conflicts: # datafusion/functions/Cargo.toml # datafusion/functions/src/lib.rs

update to allow wasm run to complete.

a02ecae

Fix expr_api example

87a9797

cargo fmt.

54d5ee6

Omega359 marked this pull request as ready for review February 28, 2024 20:25

alamb mentioned this pull request Feb 28, 2024

DataFusion weekly project plan (Andrew Lamb) - Feb 26, 2024 #9345

Closed

9 tasks

alamb reviewed Feb 28, 2024

View reviewed changes

alamb mentioned this pull request Feb 28, 2024

Add ScalarUDFs in missing function hints / suggested errors #9392

Closed

Omega359 and others added 5 commits February 28, 2024 16:26

Update datafusion/core/tests/simplification.rs

00fa3cb

Co-authored-by: Andrew Lamb <[email protected]>

Revert changes to sql in tests to restore to_timestamp(..) calls.

320f867

Merge remote-tracking branch 'upstream/main' into feature/9291

152fc83

# Conflicts: # datafusion/functions/src/lib.rs

Rust fmt apparently can't make up it's mind.

4fd9ec9

Updates for merge and moving some expression simplifier tests from op…

d83cef5

…timizer to core/tests.

Omega359 commented Feb 29, 2024

View reviewed changes

alamb reviewed Feb 29, 2024

View reviewed changes

alamb added 2 commits February 29, 2024 16:28

Revert test changes

dd9baa0

Update datafusion/core/tests/simplification.rs

10b0a8a

alamb reviewed Feb 29, 2024

View reviewed changes

datafusion/functions/Cargo.toml Outdated Show resolved Hide resolved

alamb reviewed Feb 29, 2024

View reviewed changes

Move cast_column to ColumnarValue::cast_to

c172c4c

Remove datafusion-physical-expr dependency

d5f9615

alamb mentioned this pull request Feb 29, 2024

Move cast_column to ColumnarValue::cast_to Omega359/arrow-datafusion#1

Merged

Omega359 added 2 commits February 29, 2024 19:24

Merge pull request #1 from alamb/alamb/move_cast

e29e62d

Move `cast_column` to `ColumnarValue::cast_to`

Merge remote-tracking branch 'upstream/main' into feature/9291

0767a07

# Conflicts: # datafusion/expr/src/expr_fn.rs # datafusion/functions/Cargo.toml # datafusion/physical-expr/src/datetime_expressions.rs

alamb approved these changes Mar 1, 2024

View reviewed changes

Move dependencies to dev

f59254e

Omega359 added 4 commits March 1, 2024 11:09

Fix merge conflict by migrating to_date to new functions module along…

7a1cc02

…side to_timestamp.

Merge remote-tracking branch 'origin/feature/9291' into feature/9291

c3a8d0e

# Conflicts: # datafusion/functions/Cargo.toml

Cargo lock update from merge.

b1fd2bf

Add missing licenses.

200e2f8

Omega359 commented Mar 1, 2024

View reviewed changes

alamb approved these changes Mar 1, 2024

View reviewed changes

alamb merged commit 9e39afd into apache:main Mar 1, 2024
24 checks passed

Omega359 deleted the feature/9291 branch March 1, 2024 18:49

waynexia mentioned this pull request Mar 7, 2024

chore: simplify the return type of validate_data_types() #9491

Merged

alamb mentioned this pull request Mar 10, 2024

feat: function name hints for UDFs #9407

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move the to_timestamp* functions to datafusion-functions #9388

Move the to_timestamp* functions to datafusion-functions #9388

Omega359 commented Feb 28, 2024 •

edited

Loading

Omega359 Feb 28, 2024

Omega359 Feb 28, 2024

alamb Feb 29, 2024

Omega359 Feb 28, 2024

alamb left a comment

alamb Feb 28, 2024

alamb Feb 28, 2024

Omega359 Feb 28, 2024

alamb Feb 28, 2024

Omega359 Feb 28, 2024

alamb Feb 28, 2024

alamb Feb 28, 2024

alamb Feb 28, 2024

alamb Mar 10, 2024

Omega359 Feb 29, 2024

alamb Feb 29, 2024

alamb left a comment

alamb Feb 29, 2024

alamb Feb 29, 2024

alamb left a comment

alamb commented Feb 29, 2024

Omega359 commented Feb 29, 2024

alamb commented Feb 29, 2024

alamb left a comment

alamb Mar 1, 2024 •

edited

Loading

alamb commented Mar 1, 2024

Omega359 Mar 1, 2024

Omega359 Mar 1, 2024

alamb Mar 1, 2024 •

edited

Loading

alamb left a comment

		@@ -0,0 +1,389 @@
		// Licensed to the Apache Software Foundation (ASF) under one
		// or more contributor license agreements. See the NOTICE file

Move the to_timestamp* functions to datafusion-functions #9388

Move the to_timestamp* functions to datafusion-functions #9388

Conversation

Omega359 commented Feb 28, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb commented Feb 29, 2024

Omega359 commented Feb 29, 2024

alamb commented Feb 29, 2024

alamb left a comment

Choose a reason for hiding this comment

alamb Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

alamb commented Mar 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Omega359 commented Feb 28, 2024 •

edited

Loading

alamb Mar 1, 2024 •

edited

Loading

alamb Mar 1, 2024 •

edited

Loading