Fix non-sqlcompat validation in CalciteWindowQueryTest #15086

kgyrtkirk · 2023-10-04T12:16:28Z

enable back validation in useDefaultValueForNull=false
change validator to accept both null or the default_value in case the expectation is null
juggle around some stuff in CalciteWindowQueryTest
updated the resultsets to align with useDefaultValueForNull=true
change the no_grouping testcase ; as it was triggering some independent bug with the original query

This PR has:

been self-reviewed.

This reverts commit 5cf1e2c.

(cherry picked from commit ab346d4373ea888eb8ef6115e018e7fb0d27407f)

…ssue

sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java

soumyava · 2023-10-05T18:39:32Z

sql/src/test/resources/calcite/tests/window/no_grouping.sqlTest

@@ -3,7 +3,7 @@ type: "operatorValidation"
 sql: |
  SELECT
    m1,
-    COUNT(m1) OVER () cc


Can we add a separate test if we are changing from COUNT to SUM

...actually retaining the COUNT here was leading to a difference in the plans (and/or possibly to a bug) ; the goal in this patch was to enable the checks back - from the standpoint of this test the important thing is to run a query with at least 1 window function without any group by clauses

We can add that with a failingTest annotation instead of the regular operatorValidation. That way we'll have the failing test in there so that once that bug gets address we can update it

yep; that makes sense:

added the new test as no_grouping2.sqlTest

changed no_grouping.sqlTest to be failingTest

soumyava · 2023-10-05T18:40:40Z

sql/src/test/java/org/apache/druid/sql/calcite/QueryVerification.java

+            verifier.verifyResults(queryResults);
+          }
+          catch (Exception e) {
+            throw new RuntimeException("Exception during verification!", e);


Can use a druid exception here

I was looking around what else to use - but since this is part of the test framework and DruidException wants "Persona" and "Category" which is clearly unrelated as an Exception here means that the test most likely have crashed...

throw DruidException.forPersona(Persona.DEVELOPER) .ofCategory(Category.UNCATEGORIZED) .build(e, "Exception during verification!");

I could probably use Assert.fail here - but the junit4 Assert.fail does not accept an Exception so it would not retain the stacktrace... junit5 has a method like that - but we don't yet have that here (...and since the test most likely crashed; and not failed using that would kinda miss its usecase)

Letting out the Exception at this point kinda leads to a shotgun surgery (although I do thing that every test method should be allowed to just let checked Exception-s out)

I was looking around what other exceptions are out there...but didn't found a better match than retaining to wrap the checked Exception into a plain RuntimeException ; to let it out.

This reverts commit 8fa0664.

rohangarg

Thanks for the changes @kgyrtkirk - mostly looks good. Left a few comments!

sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java

sql/src/test/java/org/apache/druid/sql/calcite/CalciteWindowQueryTest.java

soumyava · 2023-10-10T18:34:00Z

There is a difference in the wee bits after the decimal in the tests. Can you please take a look at it. So far things looks good to me

soumyava · 2023-10-11T03:57:31Z

sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java

+              mismatchMessage(row, column),
+              (Float) expectedCell,
+              (Float) resultCell,
+              1e-5);


we should make this a static variable and set it to 1e-5. Also is there any rationale behind chosing this value ?

two tests were overriding the assertResultsEquals method - and they had this constant in them; to set the absolute error bound to 1e-5

I think they were making assertions on approximation results - which may be a little bit different based on the jvm or other external reasons.

I've extracted it into a constant

soumyava · 2023-10-11T03:57:41Z

sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java

+              mismatchMessage(row, column),
+              (Double) expectedCell,
+              (Double) resultCell,
+              1e-5);


same as above

sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java

+        if (colIndex > 0) {
+          sb.append(", ");
+        }
+        if (col == null) {


soumyava · 2023-10-11T15:54:49Z

Once the CI is all greens I'll merge this

rohangarg

LGTM

* fixes * check for latest rewrite place * Revert "check for latest rewrite place" This reverts commit 5cf1e2c. * some stuff (cherry picked from commit ab346d4373ea888eb8ef6115e018e7fb0d27407f) * update test output * updates to test ouptuts * some stuff * move validator * cleanup * fix * change test slightly * add apidoc cleanup warnings * cleanup/etc * instead of telling the story; add a fail with some reason whats the issue * lead-lag fix * add test * remove unnecessary throw * druidexception-trial * Revert "druidexception-trial" This reverts commit 8fa0664. * undo changes to no_grouping; add no_grouping2 * add missing assert on resultcount * rename method; update * introduce enum/etc * make resultmatchmode accessible from TestBuilder#expectedResults * fix dump results to use log * fix * handle null correctly * disable feature type based things for MSQ * fix varianssqlaggtest * use eps in other test * fix intellij error * add final * addrss review * update test/string/etc * write concat in 3 lines :D

Backport of #15086 to 28.0.0.

* fixes * check for latest rewrite place * Revert "check for latest rewrite place" This reverts commit 5cf1e2c. * some stuff (cherry picked from commit ab346d4373ea888eb8ef6115e018e7fb0d27407f) * update test output * updates to test ouptuts * some stuff * move validator * cleanup * fix * change test slightly * add apidoc cleanup warnings * cleanup/etc * instead of telling the story; add a fail with some reason whats the issue * lead-lag fix * add test * remove unnecessary throw * druidexception-trial * Revert "druidexception-trial" This reverts commit 8fa0664. * undo changes to no_grouping; add no_grouping2 * add missing assert on resultcount * rename method; update * introduce enum/etc * make resultmatchmode accessible from TestBuilder#expectedResults * fix dump results to use log * fix * handle null correctly * disable feature type based things for MSQ * fix varianssqlaggtest * use eps in other test * fix intellij error * add final * addrss review * update test/string/etc * write concat in 3 lines :D

kgyrtkirk added 16 commits October 4, 2023 09:49

fixes

bd55afd

check for latest rewrite place

5cf1e2c

Revert "check for latest rewrite place"

c4ff274

This reverts commit 5cf1e2c.

some stuff

89844e9

(cherry picked from commit ab346d4373ea888eb8ef6115e018e7fb0d27407f)

update test output

ed5d100

updates to test ouptuts

2a4a3ab

some stuff

02aac9d

move validator

a9877c4

cleanup

f104ce4

fix

145fe82

change test slightly

761cd68

add apidoc cleanup warnings

a10fe7a

cleanup/etc

6082df5

instead of telling the story; add a fail with some reason whats the i…

34a6aeb

…ssue

Merge remote-tracking branch 'apache/master' into windowing-fix-test-cmp

9b74ef5

lead-lag fix

e376ae9

github-actions bot added the Area - Querying label Oct 4, 2023

github-advanced-security bot found potential problems Oct 4, 2023

View reviewed changes

sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java Fixed Show fixed Hide fixed

kgyrtkirk added 2 commits October 5, 2023 08:25

add test

1acd53b

remove unnecessary throw

a708ef2

kgyrtkirk marked this pull request as ready for review October 5, 2023 12:36

soumyava reviewed Oct 5, 2023

View reviewed changes

kgyrtkirk added 4 commits October 5, 2023 21:18

druidexception-trial

8fa0664

Revert "druidexception-trial"

b484606

This reverts commit 8fa0664.

undo changes to no_grouping; add no_grouping2

2858ff6

Merge remote-tracking branch 'apache/master' into windowing-fix-test-cmp

2e91d7c

rohangarg reviewed Oct 6, 2023

View reviewed changes

kgyrtkirk added 3 commits October 6, 2023 16:59

add missing assert on resultcount

9a80c89

rename method; update

ee2b35d

introduce enum/etc

4e216b7

kgyrtkirk added 2 commits October 9, 2023 13:18

fix dump results to use log

0ddd3be

Merge remote-tracking branch 'apache/master' into windowing-fix-test-cmp

4073f5e

kgyrtkirk mentioned this pull request Oct 10, 2023

Enable resultset validation of Drill tests #15096

Merged

1 task

kgyrtkirk added 3 commits October 10, 2023 09:18

Merge remote-tracking branch 'apache/master' into windowing-fix-test-cmp

47e81d8

fix

edea152

handle null correctly

de099c6

kgyrtkirk mentioned this pull request Oct 10, 2023

DrillWindowQueryTest: use proper way to decide if the query is ordered #15118

Merged

abhishekagarwal87 added this to the 28.0 milestone Oct 10, 2023

disable feature type based things for MSQ

e018b2c

kgyrtkirk added 3 commits October 10, 2023 18:49

fix varianssqlaggtest

a74a9fd

use eps in other test

185b8e7

fix intellij error

ed1bb89

soumyava reviewed Oct 11, 2023

View reviewed changes

kgyrtkirk added 3 commits October 11, 2023 05:14

add final

91b1be9

addrss review

df73774

update test/string/etc

7714e2f

github-advanced-security bot found potential problems Oct 11, 2023

View reviewed changes

soumyava approved these changes Oct 11, 2023

View reviewed changes

write concat in 3 lines :D

78d1d31

rohangarg approved these changes Oct 11, 2023

View reviewed changes

soumyava merged commit ae88f2c into apache:master Oct 11, 2023
81 checks passed

pranavbhole mentioned this pull request Oct 12, 2023

Window Fn backports #15144

Closed

10 tasks

LakshSingla mentioned this pull request Oct 14, 2023

[Backport] Fix non-sqlcompat validation in CalciteWindowQueryTest #15161

Merged

cryptoe pushed a commit that referenced this pull request Oct 16, 2023

Fix non-sqlcompat validation in CalciteWindowQueryTest (#15086) (#15161)

da24e1a

Backport of #15086 to 28.0.0.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix non-sqlcompat validation in CalciteWindowQueryTest #15086

Fix non-sqlcompat validation in CalciteWindowQueryTest #15086

kgyrtkirk commented Oct 4, 2023

soumyava Oct 5, 2023

kgyrtkirk Oct 5, 2023

somu-imply Oct 5, 2023

kgyrtkirk Oct 5, 2023

soumyava Oct 5, 2023

kgyrtkirk Oct 5, 2023

rohangarg left a comment

soumyava commented Oct 10, 2023

soumyava Oct 11, 2023

kgyrtkirk Oct 11, 2023

soumyava Oct 11, 2023

soumyava commented Oct 11, 2023

rohangarg left a comment

Fix non-sqlcompat validation in CalciteWindowQueryTest #15086

Fix non-sqlcompat validation in CalciteWindowQueryTest #15086

Conversation

kgyrtkirk commented Oct 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohangarg left a comment

Choose a reason for hiding this comment

soumyava commented Oct 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soumyava commented Oct 11, 2023

rohangarg left a comment

Choose a reason for hiding this comment