Skip to content

Add range support for window function#18953

Merged
feilong-liu merged 1 commit intoprestodb:masterfrom
feilong-liu:window_range_support
Feb 17, 2023
Merged

Add range support for window function#18953
feilong-liu merged 1 commit intoprestodb:masterfrom
feilong-liu:window_range_support

Conversation

@feilong-liu
Copy link
Contributor

@feilong-liu feilong-liu commented Jan 20, 2023 •
edited
Loading

supersede #16932

Cherry-pick of trinodb/trino#5639

Co-authored-by: Jinlin Zhang z_jinlin@yahoo.com
Co-authored-by: kasiafi 30203062+kasiafi@users.noreply.github.com

Test plan - (Please fill in how you tested your changes)

Unit tests.

== RELEASE NOTES ==

General Changes
* Add range with offset expression support to window function

@feilong-liu feilong-liu requested a review from a team as a code owner January 20, 2023 17:57
@feilong-liu feilong-liu marked this pull request as draft January 20, 2023 17:57
@feilong-liu feilong-liu force-pushed the window_range_support branch 4 times, most recently from 89dff64 to 0eb2a26 Compare January 25, 2023 20:29
@feilong-liu feilong-liu mentioned this pull request Jan 25, 2023
Closed
@feilong-liu feilong-liu force-pushed the window_range_support branch 3 times, most recently from c068697 to 49179dc Compare January 26, 2023 01:24
@feilong-liu feilong-liu marked this pull request as ready for review January 26, 2023 18:08
@feilong-liu feilong-liu force-pushed the window_range_support branch 2 times, most recently from b1c489e to ce2b2a1 Compare January 26, 2023 18:28
feilong-liu
feilong-liu commented Feb 10, 2023
View reviewed changes
Copy link
Contributor Author

@feilong-liu feilong-liu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comments on the mapping between this PR and the ported PR to help review.

presto-analyzer/src/main/java/com/facebook/presto/sql/analyzer/Analysis.java Outdated
Copy link
Contributor Author

@feilong-liu feilong-liu Feb 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change in this file match the change in the ported PR https://github.com/trinodb/trino/pull/5639/files#diff-fbcf95c2a430eb781899577e1a8f949debae117552e3449f6c91acbb02e21efa

presto-analyzer/src/main/java/com/facebook/presto/sql/analyzer/SemanticErrorCode.java Outdated
Comment on lines +53 to +54
Copy link
Contributor Author

@feilong-liu feilong-liu Feb 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corresponding to changes here https://github.com/trinodb/trino/pull/5639/files#diff-01bc66a3163352eb4ad49f6100ca0652ca7f53e6a32d9795deed8e9f6f187b02

INVALID_ORDER_BY is added in the ported PR, MISSING_ORDER_BY was added before this ported PR and not available in our codebase, hence add here.

presto-common/src/main/java/com/facebook/presto/common/type/ShortDecimalType.java Outdated
Copy link
Contributor Author

@feilong-liu feilong-liu Feb 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not from ported PR.
Need this change, as it's accessed in test TestWindowFrameRange

presto-main/src/main/java/com/facebook/presto/operator/PagesIndex.java Outdated
Copy link
Contributor Author

@feilong-liu feilong-liu Feb 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ported from https://github.com/trinodb/trino/pull/5639/files#diff-65aef49d12f7f73ff3e216dbd14814478babcd87548ea92fd7c47902da75aa88

presto-main/src/main/java/com/facebook/presto/operator/SimpleChannelComparator.java Outdated
Copy link
Contributor Author

@feilong-liu feilong-liu Feb 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ported from https://github.com/trinodb/trino/pull/5639/files#diff-5e0c59beecd68b5003a8b1e38f114aeeaf754d7d92f145a8c95ede66076cc4fb.

The difference is that, in the ported PR, it's using BlockPositionComparison which compares two block positions in ascending order, which is not available in our codebase. Here use the compareBlockValue function of sort order.

...rc/test/java/com/facebook/presto/sql/planner/optimizations/TestPruneUnreferencedOutputs.java Outdated
Copy link
Contributor Author

@feilong-liu feilong-liu Feb 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not from ported PR. Change as we added additional fields.

presto-main/src/test/java/com/facebook/presto/sql/planner/optimizations/TestReorderWindows.java Outdated
Comment on lines +342 to 349
Copy link
Contributor Author

@feilong-liu feilong-liu Feb 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in ported PR.

Change this, because the planner will add one more project to cast the sortkey to the same type as the offset expression. While in this test, it only contains a minimum number of rules, which will fail to remove this additional project node.

I've checked queries end to end with the full plan optimizer list, this additional project node will be removed, hence not a problem

presto-main/src/test/java/com/facebook/presto/sql/planner/plan/TestWindowNode.java Outdated
Copy link
Contributor Author

@feilong-liu feilong-liu Feb 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ported from https://github.com/trinodb/trino/pull/5639/files#diff-03762781190ed00a6b33be2a6d059a81024faccf4f6a8e8dad18a6173605061e

...ain/src/test/java/com/facebook/presto/sql/planner/sanity/TestVerifyNoOriginalExpression.java Outdated
Copy link
Contributor Author

@feilong-liu feilong-liu Feb 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not from ported PR.

presto-main/src/test/java/com/facebook/presto/sql/query/TestWindowFrameRange.java Outdated
Copy link
Contributor Author

@feilong-liu feilong-liu Feb 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ported from https://github.com/trinodb/trino/pull/5639/files#diff-42acdfe2669ee17584a5c176d10c42fedfea6f2ca56c5454889da40d0dd8f75e

rschlussel
rschlussel reviewed Feb 13, 2023
View reviewed changes
presto-main/src/test/java/com/facebook/presto/sql/query/TestWindowFrameRange.java Outdated
Copy link
Contributor

@rschlussel rschlussel Feb 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will run with LocalQueryRunner. I think it would also be worth running with a distributed query runner to be confident the execution side is correct in a distributed environment. Maybe instead add these tests to AbstractTestWindowQueries. That way we'll also get some spill-to-disk tests for free (via TestSpilledWindowQueries, which extends that class), as we want to make sure that spilling also works for this.

@feilong-liu feilong-liu force-pushed the window_range_support branch 2 times, most recently from ec4bcf2 to 64b06c9 Compare February 14, 2023 01:22
@v-jizhang
Copy link
Contributor

v-jizhang commented Feb 14, 2023

@prestobot kick off tests

pranjalssh
pranjalssh reviewed Feb 15, 2023
View reviewed changes
presto-tests/src/main/java/com/facebook/presto/tests/H2QueryRunner.java Outdated
Copy link
Contributor

@pranjalssh pranjalssh Feb 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change related?

Copy link
Contributor Author

@feilong-liu feilong-liu Feb 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. This is not ported, it's added for this PR.
This is because the tests added include Date type. In our test code, we convert the returned result for both Presto and H2 query runner to the same type for comparison. This code is to handle the case for DateType.

@feilong-liu @v-jizhang @kasiafi
Add range support for window function
a3c6d08 
Cherry-pick of trinodb/trino#5639

Co-authored-by: Jinlin Zhang <z_jinlin@yahoo.com>
Co-authored-by: kasiafi <30203062+kasiafi@users.noreply.github.com>
@feilong-liu feilong-liu merged commit 0fd0d6e into prestodb:master Feb 17, 2023
@wanglinsong wanglinsong mentioned this pull request Feb 25, 2023
Closed
12 tasks
@feilong-liu feilong-liu mentioned this pull request Jun 1, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pranjalssh pranjalssh pranjalssh approved these changes

@presto-oss presto-oss Awaiting requested review from presto-oss presto-oss is a code owner automatically assigned from prestodb/committers

@rschlussel rschlussel Awaiting requested review from rschlussel

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@feilong-liu @v-jizhang @pranjalssh @rschlussel