Add warning for IGNORE NULL with Window agggregates by auden-woolfson · Pull Request #23325 · prestodb/presto

auden-woolfson · 2024-07-29T22:26:49Z

Description

Added a warning when an IGNORE NULL clause is used on any non lag, lead, first or last value function. Presto will not throw an error in other cases, but it also will not ignore null values. See issue #21304

== RELEASE NOTES ==

General Changes

Add a warning when an IGNORE NULL clause is used on any non lag, lead, first, last, or nth value function. :pr:23325

linux-foundation-easycla · 2024-07-29T22:26:53Z

✅login: auden-woolfson / (64cb28b)

The committers listed above are authorized under a signed CLA.

auden-woolfson · 2024-07-29T22:34:44Z

Easy CLA should be resolved once I make/push more changes. Had to set local git config

tdcmeehan

Great work! Some suggestions.

tdcmeehan · 2024-07-30T13:59:06Z

presto-main/src/main/java/com/facebook/presto/sql/analyzer/ExpressionAnalyzer.java

            }

+            if (node.isIgnoreNulls()) {
+                ImmutableList<String> allowedIgnoreNullFunctionNames = ImmutableList.of("lead", "lag", "first_value", "last_value");


Let's not hardcode this. Instead, let's create a new method in FunctionResolution that identifies a window value function. I think it will be OK to hardcode it, but inside there, make it a constant. Also, I think you're missing one, Nth value. Double check this list by looking at all subclasses of ValueWindowFunction.

Thanks for the feedback. Made all the updates you suggested. I added the list of window value functions to FunctionResolution. Is this the right place for it to live?

Yes that's good. Next let's look for a place to put some tests. We should be able to construct some simple queries that show a warning is emitted for functions which are not window value functions, and no warning is emitted for window value functions.

tdcmeehan · 2024-07-30T13:59:20Z

presto-main/src/main/java/com/facebook/presto/sql/analyzer/ExpressionAnalyzer.java

+            if (node.isIgnoreNulls()) {
+                ImmutableList<String> allowedIgnoreNullFunctionNames = ImmutableList.of("lead", "lag", "first_value", "last_value");
+                if (!allowedIgnoreNullFunctionNames.contains(node.getName().toString())) {
+                    warningCollector.add(new PrestoWarning(SEMANTIC_WARNING, "IGNORE NULLS does not work for aggregate and ranking window functions"));


Also mention that it will start failing in the future.

tdcmeehan · 2024-07-30T18:49:32Z

presto-main/src/main/java/com/facebook/presto/sql/relational/FunctionResolution.java

        implements StandardFunctionResolution
 {
    private final FunctionAndTypeResolver functionAndTypeResolver;
+    private final ImmutableList<QualifiedObjectName> windowValueFunctions = ImmutableList.of(


Suggested change

private final ImmutableList<QualifiedObjectName> windowValueFunctions = ImmutableList.of(

private final List<QualifiedObjectName> windowValueFunctions = ImmutableList.of(

tdcmeehan · 2024-07-30T18:50:48Z

presto-main/src/main/java/com/facebook/presto/sql/analyzer/ExpressionAnalyzer.java

            }

+            if (node.isIgnoreNulls()) {
+                FunctionResolution functionResolution = new FunctionResolution(functionAndTypeResolver);


Let's make the resolution a member variable of the class

aditi-pandit · 2024-07-30T20:35:56Z

presto-main/src/test/java/com/facebook/presto/sql/analyzer/TestAnalyzer.java

+                    "(ORDER BY b)\n FROM (VALUES (1, 1, 3), (1, 2, null), (1, 4, 2)) AS t(a, b, c)"));
+        }
+
+        assertHasWarning(


Please add more tests with various aggregate functions and those in this list https://prestodb.io/docs/current/functions/window.html#ranking-functions

aditi-pandit · 2024-07-30T20:41:13Z

presto-main/src/main/java/com/facebook/presto/sql/analyzer/ExpressionAnalyzer.java

                }
            }

+            if (node.isIgnoreNulls()) {


Most of the window function semantic checks happen here https://github.com/prestodb/presto/blob/master/presto-main/src/main/java/com/facebook/presto/sql/analyzer/ExpressionAnalyzer.java#L981. Any particular reason we didn't add this check after the frame validation in that block ?

@tdcmeehan

There wasn't any reason before that I had placed it on that exact line, but it needs to be after the function variable has been initialized. I moved it back to line 1122.

aditi-pandit · 2024-07-30T20:41:44Z

@auden-woolfson : Thanks for working on this issue. Please can you merge all your commits into a single one.

tdcmeehan

LGTM % nits

tdcmeehan · 2024-07-31T14:32:14Z

presto-main/src/main/java/com/facebook/presto/sql/analyzer/ExpressionAnalyzer.java


+            if (node.isIgnoreNulls()) {
+                if (!functionResolution.isWindowValueFunction(function)) {
+                    String warningMessage = createWarningMessage(node, "IGNORE NULLS does not work for aggregate and ranking window functions, this will cause queries to fail in future versions");


Suggested change

String warningMessage = createWarningMessage(node, "IGNORE NULLS does not work for aggregate and ranking window functions, this will cause queries to fail in future versions");

String warningMessage = createWarningMessage(node, "IGNORE NULLS is not used for aggregate and ranking window functions. This will cause queries to fail in future versions.");

tdcmeehan · 2024-07-31T14:33:24Z

presto-main/src/test/java/com/facebook/presto/sql/analyzer/TestAnalyzer.java

    }

+    @Test
+    void testIgnoreNullWarning()


Suggested change

void testIgnoreNullWarning()

public void testIgnoreNullWarning()

tdcmeehan · 2024-07-31T18:41:54Z

Code looks good. Let's reword the commit, something like

Warn when a non-window value function is specified to ignore nulls

jaystarshot

Is this code path executed only once? Can there be multiple warnings for a single query

auden-woolfson · 2024-07-31T19:28:46Z

Is this code path executed only once? Can there be multiple warnings for a single query

I just tested this with the following query....

SELECT a, array_agg(c) IGNORE NULLS OVER ( PARTITION BY a ORDER BY b ROWS BETWEEN 1 PRECEDING AND CURRENT ROW ) AS agg_1, array_agg(c) IGNORE NULLS OVER ( PARTITION BY a ORDER BY b ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW ) AS agg_2 FROM (VALUES (1, 1, 3), (1, 2, NULL), (1, 4, 2)) AS t(a, b, c);

I did get two errors, it will error out for each individual "invalid" ignore null.

presto-main/src/main/java/com/facebook/presto/sql/analyzer/ExpressionAnalyzer.java

aditi-pandit

Thanks @auden-woolfson

tdcmeehan

Please add an issue to follow up and convert this warning into a failure after the next release has been cut.

tdcmeehan · 2024-08-05T14:07:03Z

@auden-woolfson please add a release note. See other PRs and our contributing guide for examples.

steveburnett

Reviewed text of warning, looks good to me!

steveburnett · 2024-08-05T16:17:16Z

For information on writing and formatting a release note entry, see the Release Notes Guidelines. As a starting point, maybe something like this following draft:

== RELEASE NOTES ==

General Changes
* Add a warning when an IGNORE NULL clause is used on any non lag, lead, first or last value function.  :pr:`23325`

auden-woolfson requested a review from tdcmeehan July 29, 2024 22:26

auden-woolfson self-assigned this Jul 29, 2024

auden-woolfson requested review from a team, feilong-liu, jaystarshot and vivek-bharathan as code owners July 29, 2024 22:26

auden-woolfson requested a review from presto-oss July 29, 2024 22:26

auden-woolfson linked an issue Jul 29, 2024 that may be closed by this pull request

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

Closed

tdcmeehan requested changes Jul 30, 2024

View reviewed changes

tdcmeehan reviewed Jul 30, 2024

View reviewed changes

aditi-pandit reviewed Jul 30, 2024

View reviewed changes

aditi-pandit changed the title ~~Window agg error~~ Add warning for IGNORE NULL with Window agggregates Jul 30, 2024

auden-woolfson force-pushed the window_agg_error branch from 5f2c078 to 9772262 Compare July 30, 2024 22:44

tdcmeehan reviewed Jul 31, 2024

View reviewed changes

auden-woolfson force-pushed the window_agg_error branch from 6a6e26b to ae8e08e Compare July 31, 2024 18:22

jaystarshot reviewed Jul 31, 2024

View reviewed changes

aditi-pandit reviewed Jul 31, 2024

View reviewed changes

presto-main/src/main/java/com/facebook/presto/sql/analyzer/ExpressionAnalyzer.java Outdated Show resolved Hide resolved

auden-woolfson force-pushed the window_agg_error branch from 0a3c63f to 98a5970 Compare August 2, 2024 20:27

auden-woolfson requested review from elharo and steveburnett as code owners August 2, 2024 20:27

tdcmeehan self-assigned this Aug 2, 2024

auden-woolfson force-pushed the window_agg_error branch 2 times, most recently from 796bf82 to a7f070a Compare August 2, 2024 20:43

aditi-pandit approved these changes Aug 5, 2024

View reviewed changes

tdcmeehan approved these changes Aug 5, 2024

View reviewed changes

steveburnett approved these changes Aug 5, 2024

View reviewed changes

auden-woolfson force-pushed the window_agg_error branch from a7f070a to 0906b5f Compare August 8, 2024 22:30

Warn when a non-window value function is specified to ignore nulls

64cb28b

auden-woolfson force-pushed the window_agg_error branch from 0906b5f to 64cb28b Compare August 8, 2024 22:37

auden-woolfson merged commit 23477ad into prestodb:master Aug 9, 2024

tdcmeehan mentioned this pull request Aug 23, 2024

Add release notes for 0.289 #23513

Merged

34 tasks

	private final ImmutableList<QualifiedObjectName> windowValueFunctions = ImmutableList.of(
	private final List<QualifiedObjectName> windowValueFunctions = ImmutableList.of(

	String warningMessage = createWarningMessage(node, "IGNORE NULLS does not work for aggregate and ranking window functions, this will cause queries to fail in future versions");
	String warningMessage = createWarningMessage(node, "IGNORE NULLS is not used for aggregate and ranking window functions. This will cause queries to fail in future versions.");

	void testIgnoreNullWarning()
	public void testIgnoreNullWarning()

Conversation

auden-woolfson commented Jul 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

linux-foundation-easycla bot commented Jul 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

auden-woolfson commented Jul 29, 2024

Uh oh!

tdcmeehan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aditi-pandit commented Jul 30, 2024

Uh oh!

tdcmeehan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tdcmeehan commented Jul 31, 2024

Uh oh!

jaystarshot left a comment

Choose a reason for hiding this comment

Uh oh!

auden-woolfson commented Jul 31, 2024

Uh oh!

Uh oh!

aditi-pandit left a comment

Choose a reason for hiding this comment

Uh oh!

tdcmeehan left a comment

Choose a reason for hiding this comment

Uh oh!

tdcmeehan commented Aug 5, 2024

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

steveburnett commented Aug 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

auden-woolfson commented Jul 29, 2024 •

edited

Loading

linux-foundation-easycla bot commented Jul 29, 2024 •

edited

Loading