Add IGNORE NULLS clause to various Window functions by ptkool · Pull Request #10568 · prestodb/presto

ptkool · 2018-05-07T16:46:57Z

This PR adds the IGNORE/RESPECT NULLS clause to window functions LAG, LEAD, FIRST_VALUE, LAST_VALUE, and NTH_VALUE.

ptkool · 2018-05-07T17:03:21Z

@electrum @martint @maciejgrzybek I ended up having to recreate this pull request after inadvertently wiping out all of my changes in the original PR. Please have a look at this.

kbkrieb · 2018-06-07T02:22:49Z

Hey @ptkool - excited to see all the work you've done on this, I have a use case & was looking for it, so just stopping by to say thanks!

johnistan · 2018-06-15T02:46:14Z

I was just looking for the functionality. would give a +1 to the feature.

facebook-github-bot · 2018-07-16T16:19:45Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

facebook-github-bot · 2018-07-16T18:04:47Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

facebook-github-bot · 2018-07-16T19:34:59Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

mrbrahman · 2018-10-05T13:17:32Z

This will be a really nice to have feature! Will avoid all the workarounds currently needed. Hope this PR is very close to acceptance!

Bennyelg · 2018-12-04T11:42:45Z

Anything new about this?

Bennyelg · 2018-12-11T12:11:39Z

@electrum ? This is very important feature!!

jinma1978 · 2018-12-12T21:29:00Z

any updates here? this is a must-have feature!!

rongrong · 2018-12-13T00:54:50Z

@ptkool Could you please squash the commits into logical features? If there's only one you can just squash all into one commit. Thanks!

Bennyelg · 2018-12-13T08:42:05Z

workaround for now:
just force sort the partition by column (i.e: partition by v1 order by t1, case when t1 is null then 1 else 0 end)

stale · 2019-06-15T19:27:15Z

This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the task, make sure you've addressed reviewer comments, and rebase on the latest master. Thank you for your contributions!

ryan-benty · 2019-06-20T22:19:00Z

@ptkool bumping this PR - this would be a huge help!!

ptkool · 2019-09-23T20:11:30Z

@rongrong Can I get this reviewed again?

ben-gilbert · 2019-11-12T19:26:14Z

Also bumping, would love this added.

For those stuck I was able to use this solution on stackoverflow to come up with a stop-gap in Presto.

https://stackoverflow.com/a/19012333

rongrong

Thanks for working on this. Sorry about the late review. I somehow missed it earlier. A few high level comments:

We structure commits logically. So when we fix things, instead of creating a commit as "fix..." we just modify the commit directly. For this PR, I think you can merge all commits into 1. You can force push to the branch afterwards.
Since RESPECT NULLS and IGNORE NULLS is a common functionality to all ValueWindowFunction, you might want to restructure the code so this is handled in the parent class to avoid duplicate logic.

presto-parser/src/main/java/com/facebook/presto/sql/parser/AstBuilder.java

presto-parser/src/main/java/com/facebook/presto/sql/ExpressionFormatter.java

rongrong

Thanks for updating. Some more detailed review. Please also update the commit message to have a meaning full summary.

presto-docs/src/main/sphinx/functions/window.rst

rongrong · 2019-11-13T19:58:24Z

presto-main/src/main/java/com/facebook/presto/sql/planner/QueryPlanner.java

I don't think you need to add ignoreNulls to Aggregation. RESPECT NULLS | IGNORE NULLS is only relevant for ValueWindowFunction. It's probably better to add a semantic check in ExpressionAnalyzer to make sure this is only specified for related window functions, and throw an explicit SemanticException otherwise.

I have removed ignoreNulls from Aggregation.

As far as adding a semantic check in ExpressionAnalyzer, there doesn't appear to be a nice way of determining whether a function is a value window function (other than checking the function name against a list of names).

rongrong · 2019-11-13T20:15:21Z

...main/src/main/java/com/facebook/presto/operator/window/ReflectionWindowFunctionSupplier.java

I think ignoreNulls should be a variable in ValueWindowFunction. You can have a separate setIgnoreNulls in ValueWindowFunction, that way you don't need to mess with the constructors. ignoreNulls is not really part of the function inputs anyways. If you keep a separate currentNonNullPosition in ValueWindowFunction, I believe the null handling can be done in ValueWindowFunction.processRow.

I have made the suggested change with ignoreNulls.

I don't believe moving the null handling to ValueWindowFunction.processRow will simplify the code - the logic isn't duplicated in any of the window functions.

You probably still need special handling on each function, but how many non-null values there are since frameStart can be tracked. The current implementation is really inefficient as it tries to iterate from the beginning of the frame on every position to count whether it got enough non-null values. This is O(n^2) while you could have tracked this in O(n).

Add parser null treatment clause test Fix compilation error Clean up Remove ignoreNulls attribute from Aggregation class Fix documentation

facebook-github-bot added the CLA Signed label May 7, 2018

ptkool mentioned this pull request May 7, 2018

add support for null treatment in LAG/LEAD #4554

Closed

ptkool force-pushed the lag_lead_ignore_nulls branch from e5fcc25 to 5790346 Compare May 8, 2018 12:57

rongrong requested a review from electrum December 13, 2018 00:56

rongrong assigned electrum Dec 13, 2018

stale bot unassigned electrum Jun 15, 2019

stale bot added the stale label Jun 15, 2019

stale bot removed the stale label Jun 20, 2019

ptkool force-pushed the lag_lead_ignore_nulls branch 3 times, most recently from 83c8050 to 40338d3 Compare September 22, 2019 23:02

ptkool force-pushed the lag_lead_ignore_nulls branch 2 times, most recently from 57bcb64 to 1105314 Compare October 22, 2019 18:34

ptkool force-pushed the lag_lead_ignore_nulls branch from 199a254 to e0bf716 Compare November 7, 2019 17:20

rongrong requested changes Nov 13, 2019

View reviewed changes

presto-parser/src/main/java/com/facebook/presto/sql/parser/AstBuilder.java Outdated Show resolved Hide resolved

presto-parser/src/main/java/com/facebook/presto/sql/ExpressionFormatter.java Outdated Show resolved Hide resolved

ptkool force-pushed the lag_lead_ignore_nulls branch 2 times, most recently from b86cbcb to a94efcc Compare November 13, 2019 14:44

rongrong requested changes Nov 13, 2019

View reviewed changes

ptkool force-pushed the lag_lead_ignore_nulls branch 2 times, most recently from 19d5c09 to 94ee06f Compare November 14, 2019 10:39

Add support for null treatment clause to various window functions

4f1ee84

Add parser null treatment clause test Fix compilation error Clean up Remove ignoreNulls attribute from Aggregation class Fix documentation

ptkool force-pushed the lag_lead_ignore_nulls branch from 94ee06f to 4f1ee84 Compare November 14, 2019 14:16

rongrong approved these changes Dec 3, 2019

View reviewed changes

rongrong merged commit 7efeecc into prestodb:master Dec 3, 2019

ptkool deleted the lag_lead_ignore_nulls branch December 21, 2019 13:15

aweisberg mentioned this pull request Jan 17, 2020

Add release notes for 0.231 #13980

Closed

7 tasks

caithagoras mentioned this pull request Jan 22, 2020

Add release notes for 0.231 #13997

Merged

6 tasks

rongrong mentioned this pull request Sep 17, 2020

first_value and last_value do not support ignore nulls and respect nulls #5801

Closed

tdcmeehan mentioned this pull request Apr 30, 2024

IGNORE NULLs in Window aggregates doesn't seem to work #21304

Closed

Conversation

ptkool commented May 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ptkool commented May 7, 2018

Uh oh!

kbkrieb commented Jun 7, 2018

Uh oh!

johnistan commented Jun 15, 2018

Uh oh!

facebook-github-bot commented Jul 16, 2018

Uh oh!

facebook-github-bot commented Jul 16, 2018

Uh oh!

facebook-github-bot commented Jul 16, 2018

Uh oh!

mrbrahman commented Oct 5, 2018

Uh oh!

Bennyelg commented Dec 4, 2018

Uh oh!

Bennyelg commented Dec 11, 2018

Uh oh!

jinma1978 commented Dec 12, 2018

Uh oh!

rongrong commented Dec 13, 2018

Uh oh!

Bennyelg commented Dec 13, 2018

Uh oh!

stale bot commented Jun 15, 2019

Uh oh!

ryan-benty commented Jun 20, 2019

Uh oh!

ptkool commented Sep 23, 2019

Uh oh!

ben-gilbert commented Nov 12, 2019

Uh oh!

rongrong left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rongrong left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rongrong Nov 13, 2019

Choose a reason for hiding this comment

Uh oh!

ptkool Nov 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ptkool Nov 14, 2019

Choose a reason for hiding this comment

Uh oh!

rongrong Nov 13, 2019

Choose a reason for hiding this comment

Uh oh!

ptkool Nov 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ptkool Nov 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rongrong Nov 18, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

ptkool commented May 7, 2018 •

edited

Loading

rongrong left a comment •

edited

Loading

ptkool Nov 14, 2019 •

edited

Loading

ptkool Nov 14, 2019 •

edited

Loading

ptkool Nov 14, 2019 •

edited

Loading