Move date_part, date_trunc, date_bin functions to datafusion-functions #9435

Omega359 · 2024-03-03T02:07:48Z

Which issue does this PR close?

Closes #9421

Rationale for this change

Function migration.

What changes are included in this PR?

Code, tests moved to new location, references updated.

Are these changes tested?

Yes.

Are there any user-facing changes?

Functions are moved to new location, otherwise none.

# Conflicts: # datafusion/proto/src/logical_plan/from_proto.rs

datafusion/functions/src/datetime/mod.rs

datafusion/physical-expr/src/equivalence/projection.rs

Omega359 · 2024-03-03T02:55:34Z

I have no idea why the tpch test would be failing. When I run tpch locally it's q1 that fails first. Any suggestions are welcome

…lain text to reflect the change. The physical plan is unchanged.

# Conflicts: # datafusion/proto/src/logical_plan/from_proto.rs

Omega359 · 2024-03-04T14:42:00Z

blocked by #9450

# Conflicts: # datafusion/physical-expr/src/equivalence/projection.rs

…ysical-expr

alamb

Thank you so much for this @Omega359 -- it is looking very good. I am a little worried about some of the test changes, but I have some ideas on how to fix them.

Sorry for the delay in reviewing

alamb · 2024-03-05T22:38:35Z

datafusion/functions/src/datetime/date_bin.rs

@@ -0,0 +1,765 @@
+// Licensed to the Apache Software Foundation (ASF) under one


This is so nice that the date_bin function has been moved into its own module

datafusion/functions/src/datetime/mod.rs

datafusion/sqllogictest/test_files/tpch/q8.slt.part

alamb · 2024-03-05T22:46:51Z

datafusion/physical-expr/src/equivalence/projection.rs

@@ -761,10 +735,7 @@ mod tests {
                // expected
                vec![
                    // [a_new ASC, date_bin_res ASC]
-                    // Please note that result is not [a_new ASC, date_bin_res ASC, b_new ASC]


In general I think this is important test coverage (it ensures that date_bin is properly being treated as monotonic). Indeed perhaps that montonic marking was not brought over to the the UDF 🤔

Though on the other hand, I think this case is already covered by .slt test (a plan test, specifically). @mustafasrepo do you remember offhand? Otherwise I will dig around

The previous version still have the test in there .. but then I had dependencies on the function crate. I saw some things with date_bin and ordering in ordering.slt that I thought might cover it

I double checked whether this functionality is covered by the .slt tests. It seems that, .slt tests covers cases in the form where date_bin argument (such as ts) is leading ordering (e.g first ordering in lexicographical ordering expression). Then after date_bin is applied, date_bin result itself is ordered. In this sense, monotonicity behaviour is covered.

However, this unit test, tests when table is ordered as [a ASC, ts ASC],after projection a, date_bin(ts) result should have ordering: [a, date_bin(ts)]. Meaning, when date_bin argument (e.g ts) is not leading ordering, lexicographical ordering is still preserved. As long as previous expressions are present in the projection (in the example a).

I checked whether this behaviour is preserved in this PR. By running following test

# create an unbounded table that contains name, timestamp. # where table is ordered by name DESC, ts DESC statement ok CREATE UNBOUNDED EXTERNAL TABLE unbounded_csv_with_timestamps2 ( name VARCHAR, ts TIMESTAMP ) STORED AS CSV WITH ORDER (name DESC, ts DESC) LOCATION '../core/tests/data/timestamps.csv' query TT EXPLAIN SELECT name, date_bin('15 minutes', ts) as time_chunks FROM unbounded_csv_with_timestamps2 ORDER BY name DESC, time_chunks DESC LIMIT 5; ---- logical_plan Limit: skip=0, fetch=5 --Sort: unbounded_csv_with_timestamps2.name DESC NULLS FIRST, time_chunks DESC NULLS FIRST, fetch=5 ----Projection: unbounded_csv_with_timestamps2.name, date_bin(IntervalMonthDayNano("900000000000"), unbounded_csv_with_timestamps2.ts) AS time_chunks ------TableScan: unbounded_csv_with_timestamps2 projection=[name, ts] physical_plan GlobalLimitExec: skip=0, fetch=5 --SortPreservingMergeExec: [name@0 DESC,time_chunks@1 DESC], fetch=5 ----ProjectionExec: expr=[name@0 as name, date_bin(900000000000, ts@1) as time_chunks] ------RepartitionExec: partitioning=RoundRobinBatch(8), input_partitions=1 --------StreamingTableExec: partition_sizes=1, projection=[name, ts], infinite_source=true, output_ordering=[name@0 DESC, ts@1 DESC]

I think, resulting plan is correct. (e.g after projection ordering [name DESC, time_chunks DESC] is still satisfied). So, we have desired behaviour but not test coverage currently.

However, it would be nice to have unit test. Given that in case of failure, error context is much more visible. It would really speed up debugging. With end to end test coverage, considerable time goes to understanding the root cause of the error.

It looks like this test has been added by @Omega359 so I think we are good to go. Thank you @mustafasrepo

# Conflicts: # datafusion/physical-expr/src/equivalence/projection.rs

mustafasrepo · 2024-03-06T07:43:16Z

I think, plan change comes from optimize_projections logical plan rule. where ScalarFunctionDefinition enum changes from ScalarFunctionDefinition::Builtin variant to ScalarFunctionDefinition::Name variant (to be resolved in physical planning). I think somehow this triggers the plan change. But I am not sure about the mechanism. Writing this in case, it is helpful.

mustafasrepo · 2024-03-06T07:59:55Z

I think, plan change comes from optimize_projections logical plan rule. where ScalarFunctionDefinition enum changes from ScalarFunctionDefinition::Builtin variant to ScalarFunctionDefinition::Name variant (to be resolved in physical planning). I think somehow this triggers the plan change. But I am not sure about the mechanism. Writing this in case, it is helpful.

I have dubugged this further. It seemed like a small fix. Hence I sent a commit to fix this change. With this change, I also added the test in my previous comment to make sure we have desired coverage.

Omega359 · 2024-03-06T12:57:27Z

I have dubugged this further. It seemed like a small fix. Hence I sent a commit to fix this change. With this change, I also added the test in my previous comment to make sure we have desired coverage.

Thank you very much for your assistance in resolving this issue in this PR - it's very much appreciated

jayzhan211 · 2024-03-07T13:25:50Z

datafusion/expr/src/expr.rs

@@ -419,6 +419,11 @@ impl ScalarFunction {
            args,
        }
    }
+
+    /// Create a new ScalarFunction expression with a user-defined function (UDF)
+    pub fn new_func_def(func_def: ScalarFunctionDefinition, args: Vec<Expr>) -> Self {


Is it better to inline the arguments directly?

Could do that, sure.

alamb · 2024-03-08T20:50:02Z

Thanks @Omega359 -- I plan to review this one in the next day or two

alamb

Thank you for sticking with this one @Omega359 -- I think this PR looks great.

alamb · 2024-03-09T17:14:54Z

datafusion/physical-expr/src/equivalence/projection.rs

@@ -761,10 +735,7 @@ mod tests {
                // expected
                vec![
                    // [a_new ASC, date_bin_res ASC]
-                    // Please note that result is not [a_new ASC, date_bin_res ASC, b_new ASC]


It looks like this test has been added by @Omega359 so I think we are good to go. Thank you @mustafasrepo

alamb · 2024-03-09T17:32:18Z

I took the liberty of merging up from main to resolve conflicts (and then reapplied the changes from #9486 manually here)

alamb · 2024-03-10T09:34:53Z

And another merge to resolve conflicts

alamb · 2024-03-10T10:30:07Z

Thanks again @Omega359 and @mustafasrepo -- sorry this one took so long to get in.

Omega359 added 2 commits March 2, 2024 20:47

Move date_part, date_trunc, date_bin functions to datafusion-functions

bcd2bd1

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

761d873

# Conflicts: # datafusion/proto/src/logical_plan/from_proto.rs

github-actions bot added sql SQL Planner logical-expr Logical plan and expressions physical-expr Physical Expressions labels Mar 3, 2024

Omega359 commented Mar 3, 2024

View reviewed changes

datafusion/functions/src/datetime/mod.rs Outdated Show resolved Hide resolved

Omega359 commented Mar 3, 2024

View reviewed changes

datafusion/physical-expr/src/equivalence/projection.rs Outdated Show resolved Hide resolved

Omega359 added 2 commits March 3, 2024 12:22

I do not understand why the logical plan changed but updating the exp…

2a41960

…lain text to reflect the change. The physical plan is unchanged.

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

35f8e36

# Conflicts: # datafusion/proto/src/logical_plan/from_proto.rs

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Mar 3, 2024

Fix fmt

618cc40

Omega359 mentioned this pull request Mar 4, 2024

[Epic] Port BuiltInFunctons to datafusion-functions-* crates #9285

Closed

43 tasks

Omega359 added 2 commits March 4, 2024 19:44

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

7ea0527

# Conflicts: # datafusion/physical-expr/src/equivalence/projection.rs

Improvements to remove datafusion-functions dependency from sq and ph…

98d5ff7

…ysical-expr

Omega359 marked this pull request as ready for review March 5, 2024 02:38

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

42039da

alamb reviewed Mar 5, 2024

View reviewed changes

Omega359 added 2 commits March 5, 2024 19:50

Fix function arguments for date_bin, date_trunc and date_part.

9675dcd

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

444337c

# Conflicts: # datafusion/physical-expr/src/equivalence/projection.rs

Fix projection change. Add new test date_bin monotonicity

c0ce362

github-actions bot added the optimizer Optimizer rules label Mar 6, 2024

jayzhan211 reviewed Mar 7, 2024

View reviewed changes

alamb mentioned this pull request Mar 8, 2024

DataFusion weekly project plan (Andrew Lamb) - March 4, 2024 #9453

Closed

5 tasks

alamb approved these changes Mar 9, 2024

View reviewed changes

Merge remote-tracking branch 'apache/main' into feature/9421

bb29203

This was referenced Mar 9, 2024

Move from_unixtime, now, current_date, current_time to datafusion-functions #9512

Open

Move date_part, date_trunc, date_bin functions to datafusion-functions #9511

Closed

Merge remote-tracking branch 'apache/main' into feature/9421

173c8b3

alamb mentioned this pull request Mar 10, 2024

[task #9511] move date_part, date_trunc, date_bin functions to datafu… #9515

Closed

alamb merged commit acddecb into apache:main Mar 10, 2024
23 checks passed

Omega359 deleted the feature/9421 branch March 14, 2024 20:33

viirya mentioned this pull request Mar 31, 2024

Create physical scalar expression in functions modules from string (name) #9892

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move date_part, date_trunc, date_bin functions to datafusion-functions #9435

Move date_part, date_trunc, date_bin functions to datafusion-functions #9435

Omega359 commented Mar 3, 2024

Omega359 commented Mar 3, 2024

Omega359 commented Mar 4, 2024

alamb left a comment

alamb Mar 5, 2024

alamb Mar 5, 2024

Omega359 Mar 5, 2024

mustafasrepo Mar 6, 2024 •

edited

Loading

alamb Mar 9, 2024

mustafasrepo commented Mar 6, 2024

mustafasrepo commented Mar 6, 2024

Omega359 commented Mar 6, 2024

jayzhan211 Mar 7, 2024

Omega359 Mar 8, 2024

alamb commented Mar 8, 2024

alamb left a comment

alamb Mar 9, 2024

alamb commented Mar 9, 2024

alamb commented Mar 10, 2024

alamb commented Mar 10, 2024

		@@ -0,0 +1,765 @@
		// Licensed to the Apache Software Foundation (ASF) under one

Move date_part, date_trunc, date_bin functions to datafusion-functions #9435

Move date_part, date_trunc, date_bin functions to datafusion-functions #9435

Conversation

Omega359 commented Mar 3, 2024

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?

Omega359 commented Mar 3, 2024

Omega359 commented Mar 4, 2024

alamb left a comment

Choose a reason for hiding this comment

alamb Mar 5, 2024

Choose a reason for hiding this comment

alamb Mar 5, 2024

Choose a reason for hiding this comment

Omega359 Mar 5, 2024

Choose a reason for hiding this comment

mustafasrepo Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

alamb Mar 9, 2024

Choose a reason for hiding this comment

mustafasrepo commented Mar 6, 2024

mustafasrepo commented Mar 6, 2024

Omega359 commented Mar 6, 2024

jayzhan211 Mar 7, 2024

Choose a reason for hiding this comment

Omega359 Mar 8, 2024

Choose a reason for hiding this comment

alamb commented Mar 8, 2024

alamb left a comment

Choose a reason for hiding this comment

alamb Mar 9, 2024

Choose a reason for hiding this comment

alamb commented Mar 9, 2024

alamb commented Mar 10, 2024

alamb commented Mar 10, 2024

mustafasrepo Mar 6, 2024 •

edited

Loading