Support for non-u64 types for Window Bound #3916

mustafasrepo · 2022-10-21T07:15:32Z

Support for non-u64 types for Window Bound

Which issue does this PR close?

Closes #3571 .

Rationale for this change

With this change we are adding support for queries like below.

SELECT 
	COUNT(*) OVER(ORDER BY ts RANGE BETWEEN '1 DAY' PRECEDING AND '1 DAY' FOLLOWING) as cnt 
FROM t

What changes are included in this PR?

Closing this issue requires support for parsing expressions inside window frame queries. Hence we have updated sqlparser version.
assert_contains and assert_not_contains macros are used in tests at different places. These macros were duplicated where they were used, we have moved them under https://github.com/synnada-ai/arrow-datafusion/blob/feature/window_bound_scalar/datafusion/common/src/test_util.rs to prevent code duplication.
Util codes for datetime arithmetic are moved under https://github.com/synnada-ai/arrow-datafusion/blob/feature/window_bound_scalar/datafusion/common/src/scalar.rs. They are reached now with .add and .sub api of ScalarValue enum.
We removed type coercions between ScalarValue types. Arithmetic operations are done only between same types (for datetime types arithmetic operations are done only with INTERVAL types.)
Previously we were rejecting queries in the form

SELECT
      COUNT(c1) OVER (ORDER BY c2 RANGE BETWEEN 1 PRECEDING AND 2 PRECEDING)
      FROM aggregate_test_100

during window frame creation. Since we are supporting more reach variants such as

SELECT 
	COUNT(*) OVER(ORDER BY ts RANGE BETWEEN '1 DAY' PRECEDING AND '1 MONTH' PRECEDING) as cnt 
FROM t

it is not easy to do rejection during creation. We now do calculations for whether window frame is valid after physical schema information is available. Where we can do validation calculations with appropriate types. This doesn’t change anything for end users.

sqlparser had also changes in Expr type. We have incorporated those changes. However, possibly they are written with some new capabilities in mind. We just did the bare minimum to support the current test suit.

Are there any user-facing changes?

N.A

…ada-ai/arrow-datafusion into feature/window_bound_scalar # Conflicts: # datafusion/common/src/lib.rs

alamb

Thank you for the contribution @mustafasrepo -- these PRs are very impressive; I am very excited to see additional date/time functionality added to DataFusion and thus I try and prioritize reviewing such PRs.

One thing that would make it easier to review PRs such as this would be to break them up into multiple smaller PRs. The benefits of doing so are:

decreases the contiguous focus time (hard to find some days) needed to review them
increases the number of people who can review / merge them (we have many reviewers / committers, but only a few who feel comfortable reviewing a PR that touches so much of the codebase)

For example in this case multiple PRs could be:

Update sqlparser-rs
Update parser and logical planning (but error if anything other than a uint64 was passed)
move the location of test utils / code to common
physical planner / physical expr changes

The only question I have about this PR that I think needs to be addressed before merging is the type coercion (see comment inline)

cc @waitingkuo @avantgardnerio @ovr

datafusion/common/src/delta.rs

alamb · 2022-10-21T18:49:06Z

datafusion/common/src/scalar.rs

-                impl_distinct_cases_op!($LHS, $RHS, $OPERATION)
+            // Binary operations on arguments with different types:
+            (ScalarValue::Date32(Some(days)), _) => {
+                let value = date32_add(*days, $RHS, get_sign!($OPERATION))?;


since this is a generic macro I am surprised to see date32_add -- this would likely be surprising to someone if they tried to use this macro for mul or div I think

alamb · 2022-10-21T18:52:40Z

datafusion/common/src/scalar.rs

    };
 }

+#[inline]
+pub fn date32_add(days: i32, scalar: &ScalarValue, sign: i32) -> Result<i32> {


In general I would like to consolidate date / time arithmetic into a single location, and ideally that location is arrow-rs.

Thus I think we should be using the date math functions in arrow-rs -- specifically https://docs.rs/arrow/25.0.0/arrow/datatypes/struct.Date32Type.html

I see that this code is simply moved in this PR, but in general what do you think?

datafusion/common/src/scalar.rs

datafusion/common/src/test_util.rs

datafusion/expr/src/window_frame.rs

datafusion/physical-expr/src/window/aggregate.rs

alamb · 2022-10-21T19:11:00Z

datafusion/core/src/physical_plan/planner.rs

+/// if `column_type` is Timestamp the result is casted to Interval type. The reason is that
+/// Operation between Timestamps is not meaningful, However operation between Timestamp and
+/// Interval is valid. For basic types `column_type` is indeed the resulting type.
+fn convert_to_column_type(


This operation is typically called "coercion" and is handled in datafusion for other expression types here: https://github.com/apache/arrow-datafusion/blob/master/datafusion/expr/src/type_coercion.rs#L18-L32 (and submodule).

Did you consider doing this conversion as part of coercion?

One reason to do it as part of the normal coercion is that then the proper types will be present for operations such as constant folding / constant propagation. This might allow for expressions like

COUNT(*) OVER (ORDER BY ts RANGE BETWEEN INTERVAL '1' DAY + INTERVAL '1' DAY PRECEDING AND INTERVAL '3 DAY' FOLLOWING),

Eventually

ozankabak · 2022-10-22T14:43:29Z

Thank you Andrew for your careful review and comments (as always). We will go through them and update the PR soon.

alamb · 2022-10-24T21:06:01Z

What is your thinking regarding moving coercion of out physical planning and into the coercion pass? Is that something you plan to do?

ozankabak · 2022-10-25T02:01:36Z

@mustafasrepo is trying to solidify his understanding of your suggestion. He will probably circle back tomorrow to make sure he gets what you meant right -- and then follow through if you guys are on the same page. He may ask for some clarifying directions of you on how to do this.

mustafasrepo · 2022-10-25T11:13:31Z

What is your thinking regarding moving coercion of out physical planning and into the coercion pass? Is that something you plan to do?

Hi @alamb, I sent a commit moving coercion from physical planning to the optimizer type_coercion. Is this what you had in mind or I misunderstood what you meant? I can change the implementation according to your suggestions. Thanks for your feedback.

alamb · 2022-10-25T16:18:46Z

Hi @alamb, I sent a commit moving coercion from physical planning to the optimizer type_coercion. Is this what you had in mind or I misunderstood what you meant? I can change the implementation according to your suggestions. Thanks for your feedback.

This is exactly right -- thanks

alamb

I think this PR is ready to go -- thank you @mustafasrepo .

I will plan to merge it tomorrow unless anyone else would like to offer suggestions or needs more time to review it

alamb · 2022-10-26T12:23:01Z

Thanks again @mustafasrepo and @ozankabak -- this is epic work

ursabot · 2022-10-26T18:39:32Z

Benchmark runs are scheduled for baseline = 4e29835 and contender = 3940e36. 3940e36 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

* Support for non u64 values inside window frames * timestamp handling is added * get_rank removed * Tidy up datetime arithmetic, get rid of unwraps * new test for validity, during window frame creation is added * window_bound is changed to Expr (apache#6) * give fixed commit hash as dependency * remove locked flag * Simplify some frame bound conversion functions * Make things work the new sqlparser * Upgrade sqlparser version to 0.26 * Minor changes * type coercion for window frames moved to the optimizer. Co-authored-by: Mehmet Ozan Kabak <ozankabak@gmail.com>

mustafasrepo and others added 16 commits October 11, 2022 23:43

Support for non u64 values inside window frames

45ffb4a

timestamp handling is added

10204c8

get_rank removed

964f8c7

Tidy up datetime arithmetic, get rid of unwraps

2d8a52f

new test for validity, during window frame creation is added

d900968

window_bound is changed to Expr (#6)

19058b8

give fixed commit hash as dependency

51f8f74

remove locked flag

600d60b

Simplify some frame bound conversion functions

4f8a2ce

Merge branch 'master' into feature/window_bound_scalar

b1538ac

Make things work the new sqlparser

08af94e

Merge branch 'master' into feature/window_bound_scalar

09a01f2

Merge branch 'master' into feature/window_bound_scalar

5536597

Merge branch 'master' into feature/window_bound_scalar

85646de

Upgrade sqlparser version to 0.26

d8630e8

Merge branch 'feature/window_bound_scalar' of https://github.com/synn…

e4cf039

…ada-ai/arrow-datafusion into feature/window_bound_scalar # Conflicts: # datafusion/common/src/lib.rs

github-actions bot added core Core DataFusion crate logical-expr Logical plan and expressions physical-expr Physical Expressions sql SQL Planner labels Oct 21, 2022

mustafasrepo changed the title ~~Feature/window bound scalar~~ Support for non-u64 types for Window Bound Oct 21, 2022

alamb reviewed Oct 21, 2022

View reviewed changes

alamb mentioned this pull request Oct 21, 2022

Update sqlparser requirement from 0.25 to 0.26 #3921

Closed

Merge branch 'master' into feature/window_bound_scalar

f919ca3

Minor changes

082753c

type coercion for window frames moved to the optimizer.

b78eb16

github-actions bot added the optimizer Optimizer rules label Oct 25, 2022

alamb approved these changes Oct 25, 2022

View reviewed changes

alamb added the api change Changes the API exposed to users of the crate label Oct 25, 2022

alamb merged commit 3940e36 into apache:master Oct 26, 2022

andygrove mentioned this pull request Nov 1, 2022

Stack overflow planning complex query #4065

Closed

mustafasrepo deleted the feature/window_bound_scalar branch November 28, 2022 07:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for non-u64 types for Window Bound #3916

Support for non-u64 types for Window Bound #3916

mustafasrepo commented Oct 21, 2022 •

edited by alamb

Loading

alamb left a comment

alamb Oct 21, 2022

alamb Oct 21, 2022

alamb Oct 21, 2022

ozankabak commented Oct 22, 2022

alamb commented Oct 24, 2022

ozankabak commented Oct 25, 2022 •

edited

Loading

mustafasrepo commented Oct 25, 2022 •

edited

Loading

alamb commented Oct 25, 2022

alamb left a comment

alamb commented Oct 26, 2022

ursabot commented Oct 26, 2022

Support for non-u64 types for Window Bound #3916

Support for non-u64 types for Window Bound #3916

Conversation

mustafasrepo commented Oct 21, 2022 • edited by alamb Loading

Support for non-u64 types for Window Bound

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

alamb left a comment

Choose a reason for hiding this comment

alamb Oct 21, 2022

Choose a reason for hiding this comment

alamb Oct 21, 2022

Choose a reason for hiding this comment

alamb Oct 21, 2022

Choose a reason for hiding this comment

ozankabak commented Oct 22, 2022

alamb commented Oct 24, 2022

ozankabak commented Oct 25, 2022 • edited Loading

mustafasrepo commented Oct 25, 2022 • edited Loading

alamb commented Oct 25, 2022

alamb left a comment

Choose a reason for hiding this comment

alamb commented Oct 26, 2022

ursabot commented Oct 26, 2022

mustafasrepo commented Oct 21, 2022 •

edited by alamb

Loading

ozankabak commented Oct 25, 2022 •

edited

Loading

mustafasrepo commented Oct 25, 2022 •

edited

Loading