[WIP] Fix incorrect FILTER clause evaluation order

[thomaschow]

• adding a unit test to reproduce the bug Incorrect FILTER clause evaluation order #7
• the bug seems to be in the h2database library that's used for testing, and not actually an issue in the presto core code itself. I also verified this with the hive connector through the presto-cli.
• bumping the underlying version of h2database to the latest release fixes this with h2database/h2database@34aaf06

example test output:

java.lang.AssertionError: Execution of 'expected' query failed: SELECT sum(1 / a) FILTER (WHERE a <> 0) FROM (VALUES (1), (0)) t(a)

	at org.testng.Assert.fail(Assert.java:83)
	at io.prestosql.tests.QueryAssertions.assertQuery(QueryAssertions.java:164)
	at io.prestosql.tests.QueryAssertions.assertQuery(QueryAssertions.java:106)
	at io.prestosql.tests.AbstractTestQueryFramework.assertQuery(AbstractTestQueryFramework.java:133)
	at io.prestosql.tests.AbstractTestQueryFramework.assertQuery(AbstractTestQueryFramework.java:128)
	at io.prestosql.tests.AbstractTestAggregations.testAggregationFilterEvaluationOrdering(AbstractTestAggregations.java:328)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:104)
	at org.testng.internal.Invoker.invokeMethod(Invoker.java:645)
	at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:851)
	at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1177)
	at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:129)
	at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:112)
	at org.testng.TestRunner.privateRun(TestRunner.java:756)
	at org.testng.TestRunner.run(TestRunner.java:610)
	at org.testng.SuiteRunner.runTest(SuiteRunner.java:387)
	at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:382)
	at org.testng.SuiteRunner.privateRun(SuiteRunner.java:340)
	at org.testng.SuiteRunner.run(SuiteRunner.java:289)
	at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:52)
	at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:86)
	at org.testng.TestNG.runSuitesSequentially(TestNG.java:1293)
	at org.testng.TestNG.runSuitesLocally(TestNG.java:1218)
	at org.testng.TestNG.runSuites(TestNG.java:1133)
	at org.testng.TestNG.run(TestNG.java:1104)
	at org.testng.IDEARemoteTestNG.run(IDEARemoteTestNG.java:72)
	at org.testng.RemoteTestNGStarter.main(RemoteTestNGStarter.java:124)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at com.intellij.rt.execution.application.AppMain.main(AppMain.java:147)
Caused by: org.jdbi.v3.core.statement.UnableToExecuteStatementException: org.h2.jdbc.JdbcSQLException: Division by zero: "1"; SQL statement:
SELECT sum(1 / a) FILTER (WHERE a <> 0) FROM (VALUES (1), (0)) t(a) [22012-197] [statement:"SELECT sum(1 / a) FILTER (WHERE a <> 0) FROM (VALUES (1), (0)) t(a)", rewritten:"SELECT sum(1 / a) FILTER (WHERE a <> 0) FROM (VALUES (1), (0)) t(a)", parsed:"ParsedSql{sql='SELECT sum(1 / a) FILTER (WHERE a <> 0) FROM (VALUES (1), (0)) t(a)', parameters=ParsedParameters{positional=false, parameterNames=[]}}", arguments:{positional:{}, named:{}, finder:[]}]
	

...

[cla-bot]

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 submit the signed CLA to cla@prestosql.io. For more information, see https://github.com/prestosql/cla.

[cla-bot]

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 submit the signed CLA to cla@prestosql.io. For more information, see https://github.com/prestosql/cla.

[ebyhr]

Hi, thank you for creating this PR.

Unfortunately(?), h2database 1.4.198 added INTERVAL data type and it breaks existing unit tests.
https://github.com/h2database/h2database/releases/tag/version-1.4.198

[ERROR] testNonReservedTimeWords(io.prestosql.tests.TestDistributedSpilledQueries)  Time elapsed: 0.107 s  <<< FAILURE!
java.lang.AssertionError: 
Execution of 'expected' query failed: SELECT TIME, TIMESTAMP, DATE, INTERVAL
FROM (SELECT 1 TIME, 2 TIMESTAMP, 3 DATE, 4 INTERVAL)
	at org.testng.Assert.fail(Assert.java:83)
...

[thomaschow]

@ebyhr do you think it'd be OK to remove that from the set of nonreserved time words?

[ebyhr]

I think it's not OK since they aren't reserved keyword as presto. Also, it seems below test class ensures filter and aggregation order.
https://github.com/prestosql/presto/blob/57051c9a9616eee4f8713f58b84c07a8041767cb/presto-main/src/test/java/io/prestosql/sql/query/TestFilteredAggregations.java#L50-L62

[martint]

the bug seems to be in the h2database library that's used for testing, and not actually an issue in the presto core code itself. I also verified this with the hive connector through the presto-cli.

It is actually a bug in Presto. It looks like some recent change made that specific shape of query succeed. I updated the example with a version that triggers the problem:

select 
    sum(1 / a) filter (where a <> 0), 
    sum(a) filter (where true)
from (values (1), (0)) t(a)

[sopel39]

Could it be related to: prestodb/presto#11425?

[martint]

This is not an issue with H2, but a bug in Presto, as described above. Closing, since it doesn't address the underlying problem. Please reopen once you've updated the implementation.