Repository: AlecStrong/sql-psi
Pull request #411 (2022-07-24 16:23:06)

add multiple column where fixture
Hey, 
I was trying to achieve [stable pagination](https://gist.github.com/sphrak/c0824be44151636a8bfbcd8dc04df40b) with sqlite and figured I'd try the same with sqldelight however its currently not too thrilled about it.

I attempted to add my own rule, something like this;
```bnf
multi_column_expr ::= '(' expr ( ',' expr ) * ')' (  '<' | '<=' | '>' | '>=' ) '(' expr ( ',' expr ) * ')' {
  pin = 1
}
```
.. but ill admit im just guessting here and then I read in contributing.md that `expr` is a special case that cannot be overridden? so im a tad bit confused on how I could proceed because it would be quite neat if this worked, but im not sure how to proceed.

https://github.com/AlecStrong/sql-psi/issues/273 seems kinda similar or at least a little bit related.

I attached a test fixture that fails with:
```sh
ava.lang.AssertionError: Test failed because the compile output unexpectedly has the errors <[
    Test.s line 10:9 - '(', ')', '.', <binary like operator real>, BETWEEN or IN expected, got ','
]>. 
Overall we expected to see no errors but got the errors <[
    Test.s line 10:9 - '(', ')', '.', <binary like operator real>, BETWEEN or IN expected, got ','
]>
	at com.alecstrong.sql.psi.test.fixtures.FixturesTest.execute(FixturesTest.kt:89)
	at jdk.internal.reflect.GeneratedMethodAccessor2.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63)
	at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
	at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
	at org.junit.runners.Suite.runChild(Suite.java:128)
	at org.junit.runners.Suite.runChild(Suite.java:27)
	at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
	at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
	at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
	at org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecutor.runTestClass(JUnitTestClassExecutor.java:110)
	at org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecutor.execute(JUnitTestClassExecutor.java:58)
	at org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecutor.execute(JUnitTestClassExecutor.java:38)
	at org.gradle.api.internal.tasks.testing.junit.AbstractJUnitTestClassProcessor.processTestClass(AbstractJUnitTestClassProcessor.java:62)
	at org.gradle.api.internal.tasks.testing.SuiteTestClassProcessor.processTestClass(SuiteTestClassProcessor.java:51)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:36)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
	at org.gradle.internal.dispatch.ContextClassLoaderDispatch.dispatch(ContextClassLoaderDispatch.java:33)
	at org.gradle.internal.dispatch.ProxyDispatchAdapter$DispatchingInvocationHandler.invoke(ProxyDispatchAdapter.java:94)
	at com.sun.proxy.$Proxy2.processTestClass(Unknown Source)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker$2.run(TestWorker.java:176)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.executeAndMaintainThreadName(TestWorker.java:129)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.execute(TestWorker.java:100)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.execute(TestWorker.java:60)
	at org.gradle.process.internal.worker.child.ActionExecutionWorker.execute(ActionExecutionWorker.java:56)
	at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:133)
	at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:71)
	at worker.org.gradle.process.internal.worker.GradleWorkerMain.run(GradleWorkerMain.java:69)
	at worker.org.gradle.process.internal.worker.GradleWorkerMain.main(GradleWorkerMain.java:74)```

hfhbd: Yeah you can't override expr. Does this syntax work in ANSI SQL or in sqlite only? If the first, you could just update the expr definition. Otherwise update the sqlite dialect in sqldelight by overwriting the extension_expr.
hfhbd: BTW the implementation is a bit tricky. The algorithm is greedy (that's why left side recursion is not allowed), so you have to do this:
```
expr ::= ( other_expr
         | raise_expr
         | case_expr
         | exists_expr
         | paren_expr
         | binary_or_expr
         | binary_and_expr
         | between_expr
         | binary_like_expr
         | is_expr
         | null_expr
         | collate_expr
         | cast_expr
         | function_expr
         | in_expr
         | binary_equality_expr
         | binary_boolean_expr
         | binary_bitwise_expr
         | binary_add_expr
         | binary_mult_expr
         | binary_pipe_expr
         | unary_expr
         | literal_expr
         | column_expr
         | bind_expr )

other_expr ::= extension_expr
extension_expr ::= NULL | multi_column_expr
multi_column_expr ::= values_expression [ ( LT | LTE | GT | GTE ) values_expression ] {
  pin = 1
}
```
sphrak: @hfhbd that was fast and indeed your code does make the test pass. 

It was surprisingly difficult to find a copy of anything that resembled some sort of specification but I found the SQL-92 spec [here](http://www.contrib.andrew.cmu.edu/~shadow/sql/sql1992.txt) and according to the linked SO post below they talk about that behavior on page 209.

there is a somewhat recent list of dbms and their support status for 'tuple comparison':

* https://www.sql-workbench.eu/dbms_comparison.html

The tldr is it seems to be supported for most dbms:

|                  | Oracle | Postgres | SQL Server | IBM DB2 | MySQL | MariaDB | Firebird | H2  | HSQLDB | SQLite |
|------------------|--------|----------|------------|---------|-------|---------|----------|-----|--------|--------|
| Tuple comparison | yes    | yes      | no         | yes     | yes   | yes     | no       | yes | yes    | yes    |

#### more links

* https://www.sql-workbench.eu/comparison/tuple_comparison.html
* https://stackoverflow.com/questions/5698801/sqlite-tuple-comparison
* https://www.sqlite.org/rowvalue.html
AlecStrong: I'm fine with it in the ANSI grammar then, that's pretty much as close to supported as ANSI as we ever see

Patch:
diff --git a/core/src/main/kotlin/com/alecstrong/sql/psi/core/sql.bnf b/core/src/main/kotlin/com/alecstrong/sql/psi/core/sql.bnf
index a38ead5..708359b 100644
--- a/core/src/main/kotlin/com/alecstrong/sql/psi/core/sql.bnf
+++ b/core/src/main/kotlin/com/alecstrong/sql/psi/core/sql.bnf
@@ -201,9 +201,11 @@ drop_view_stmt ::= DROP VIEW [ IF EXISTS ] [ database_name '.' ] view_name {
   stubClass = "com.alecstrong.sql.psi.core.psi.SchemaContributorStub"
   pin = 2
 }
-expr ::= ( raise_expr
+expr ::= ( other_expr
+         | raise_expr
          | case_expr
          | exists_expr
+         | multi_column_expr
          | paren_expr
          | binary_or_expr
          | binary_and_expr
@@ -223,13 +225,15 @@ expr ::= ( raise_expr
          | binary_pipe_expr
          | unary_expr
          | literal_expr
-         | other_expr
          | column_expr
          | bind_expr )
 
 other_expr ::= extension_expr
 extension_expr ::= NULL
 extension_stmt ::= NULL
+multi_column_expr ::= values_expression [ ( LT | LTE | GT | GTE ) values_expression ] {
+  pin = 1
+}
 literal_expr ::= literal_value
 bind_expr ::= bind_parameter
 column_expr ::= [ database_name '.' table_name '.' | table_name '.' ] column_name
diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml
index 15988f7..ff647c4 100644
--- a/gradle/libs.versions.toml
+++ b/gradle/libs.versions.toml
@@ -12,7 +12,7 @@ truth = "com.google.truth:truth:1.1.3"
 [plugins]
 kotlinJvm = { id = "org.jetbrains.kotlin.jvm", version.ref = "kotlin" }
 grammarKitComposer = { id = "com.alecstrong.grammar.kit.composer", version = "0.1.10" }
-spotless = { id = "com.diffplug.spotless", version = "6.8.0" }
+spotless = { id = "com.diffplug.spotless", version = "6.9.0" }
 mavenPublish = { id = "com.vanniktech.maven.publish", version = "0.21.0" }
 dokka = { id = "org.jetbrains.dokka", version = "1.7.10" }
 intellij = { id = "org.jetbrains.intellij", version = "1.7.0" }


Test patch:
diff --git a/test-fixtures/src/main/resources/fixtures/multiple-column-where/Test.s b/test-fixtures/src/main/resources/fixtures/multiple-column-where/Test.s
new file mode 100644
index 0000000..1c49aaa
--- /dev/null
+++ b/test-fixtures/src/main/resources/fixtures/multiple-column-where/Test.s
@@ -0,0 +1,17 @@
+CREATE TABLE posts (
+  id TEXT NOT NULL PRIMARY KEY,
+  text TEXT,
+  created_at INTEGER NOT NULL
+);
+
+-- works fine.
+SELECT *
+FROM posts
+WHERE (id, created_at) <= (?, ?)
+ORDER BY created_at DESC
+LIMIT 4;
+
+-- should fail.
+SELECT *
+FROM posts
+WHERE (id, ) <= (?, ?)
\ No newline at end of file
diff --git a/test-fixtures/src/main/resources/fixtures/multiple-column-where/failure.txt b/test-fixtures/src/main/resources/fixtures/multiple-column-where/failure.txt
new file mode 100644
index 0000000..f8f9465
--- /dev/null
+++ b/test-fixtures/src/main/resources/fixtures/multiple-column-where/failure.txt
@@ -0,0 +1,1 @@
+Test.s line 17:9 - '(', ')', ',', '.', <binary like operator real>, BETWEEN or IN expected, got ','
\ No newline at end of file