Skip to content

Commit

Permalink
Fix SQL ArrayType / CustomType failure mode (#5445)
Browse files Browse the repository at this point in the history
* Fix SQL ArrayType / CustomType failure mode

The following code block now successfully runs. A couple of failure modes are indicated when a "bad" column type is used in an invalid way. In some cases Calcite will notice the issue, and other tim
es, it will pass the query to the engine where it might fail.

```python
from deephaven.experimental import sql
from deephaven import empty_table

a = {}
t1 = empty_table(4).view(["I=ii"])
t2 = t1.update("A = a")

# equivalent to t1_all = t1
t1_all = sql.evaluate('SELECT * FROM t1')

# equivalent to t2_all = t2
t2_all = sql.evaluate('SELECT * FROM t2')

# org.apache.calcite.sql.validate.SqlValidatorException: Cannot apply '+' to arguments of type '<JAVATYPE(CLASS IO.DEEPHAVEN.SQL.TYPEADAPTER$SQLTODOCUSTOMTYPE)> + <INTEGER>'. Supported form(s): '<NUMERIC> + <NUMERIC>'
# '<DATETIME_INTERVAL> + <DATETIME_INTERVAL>'
# '<DATETIME> + <DATETIME_INTERVAL>'
# '<DATETIME_INTERVAL> + <DATETIME>'
# 	at java.base/jdk.internal.reflect.DirectConstructorHandleAccessor.newInstance(DirectConstructorHandleAccessor.java:62)
# 	at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:502)
#	at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:486)
#	at org.apache.calcite.runtime.Resources$ExInstWithCause.ex(Resources.java:507)
#	at org.apache.calcite.runtime.Resources$ExInst.ex(Resources.java:601)
#	... 53 more
#t2_bad_op = sql.evaluate('SELECT A + 1 FROM t2')

# Requirement failed: required Comparable.class.isAssignableFrom(sortColumns[ii].getType()) || sortColumns[ii].getType().isPrimitive(), instead sortColumnNames[ii] == "A", sortColumns[ii].getType() == class org.jpy.PyDictWrapper.
#	at io.deephaven.base.verify.Require.fail(Require.java:108)
#	at io.deephaven.base.verify.Require.requirement(Require.java:169)
#	at io.deephaven.base.verify.Require.requirement(Require.java:175)
#	at io.deephaven.engine.table.impl.SortOperation.<init>(SortOperation.java:69)
#t2_engine_sort = sql.evaluate('SELECT A FROM t2 ORDER BY A')
```

Fixes #5443

* spotless

* Add tests
  • Loading branch information
devinrsmith authored and stanbrub committed May 3, 2024
1 parent ed7d643 commit 9928db0
Show file tree
Hide file tree
Showing 9 changed files with 109 additions and 2 deletions.
19 changes: 17 additions & 2 deletions sql/src/main/java/io/deephaven/sql/TypeAdapter.java
Original file line number Diff line number Diff line change
Expand Up @@ -117,13 +117,13 @@ public RelDataType visit(InstantType instantType) {
@Override
public RelDataType visit(ArrayType<?, ?> arrayType) {
// SQLTODO(array-type)
throw new UnsupportedOperationException("SQLTODO(array-type)");
return typeFactory.createJavaType(SqlTodoArrayType.class);
}

@Override
public RelDataType visit(CustomType<?> customType) {
// SQLTODO(custom-type)
throw new UnsupportedOperationException("SQLTODO(custom-type)");
return typeFactory.createJavaType(SqlTodoCustomType.class);
}

private RelDataType create(SqlTypeName typeName) {
Expand All @@ -133,4 +133,19 @@ private RelDataType create(SqlTypeName typeName) {
private RelDataType nullable(RelDataType relDataType) {
return typeFactory.createTypeWithNullability(relDataType, true);
}

// We currently need our type parsing to always succeed; right now, we implicitly inherit the user's scope, and if
// they happen to have a table with column of CustomType or ArrayType, we need to have that not immediately fail.
// Firstly, the user might not even be referencing that table / column. Secondly, the user might just be passing
// the column through unchanged, in which case Calcite is happy to create the plan.
// See https://github.com/deephaven/deephaven-core/issues/5443 for more context.

static class SqlTodoType {
}

static class SqlTodoArrayType extends SqlTodoType {
}

static class SqlTodoCustomType extends SqlTodoType {
}
}
47 changes: 47 additions & 0 deletions sql/src/test/java/io/deephaven/sql/SqlAdapterTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
import io.deephaven.qst.table.TableHeader;
import io.deephaven.qst.table.TableSpec;
import io.deephaven.qst.table.TicketTable;
import io.deephaven.qst.type.Type;
import org.apache.calcite.runtime.CalciteContextException;
import org.junit.jupiter.api.Test;

import java.io.IOException;
Expand All @@ -17,6 +19,7 @@
import java.util.List;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.failBecauseExceptionWasNotThrown;

public class SqlAdapterTest {

Expand Down Expand Up @@ -45,6 +48,14 @@ public class SqlAdapterTest {

private static final TableHeader TIME2 = TableHeader.of(ColumnHeader.ofInstant("Time2"));

interface MyCustomType {

}

private static final TableHeader CUSTOM = TableHeader.of(
ColumnHeader.of("Foo", Type.longType()),
ColumnHeader.of("Bar", Type.ofCustom(MyCustomType.class)));

@Test
void sql1() throws IOException, URISyntaxException {
final Scope scope = scope(
Expand Down Expand Up @@ -270,10 +281,46 @@ void sql35() throws IOException, URISyntaxException {
check(scope, 35);
}

@Test
void sql36() throws IOException, URISyntaxException {
final Scope scope = scope("custom", CUSTOM);
check(scope, 36);
}

@Test
void sql37() throws IOException, URISyntaxException {
final Scope scope = scope("custom", CUSTOM);
check(scope, 37);
}

@Test
void sql38() throws IOException, URISyntaxException {
final Scope scope = scope("custom", CUSTOM);
check(scope, 38);
}

@Test
void sql39() throws IOException, URISyntaxException {
final Scope scope = scope("custom", CUSTOM);
checkError(scope, 39, CalciteContextException.class,
"Cannot apply '+' to arguments of type '<JAVATYPE(CLASS IO.DEEPHAVEN.SQL.TYPEADAPTER$SQLTODOCUSTOMTYPE)> + <INTEGER>'");
}

private static void check(Scope scope, int index) throws IOException, URISyntaxException {
check(scope, String.format("query-%d.sql", index), String.format("qst-%d.dot", index));
}

private static void checkError(Scope scope, int index, Class<? extends Throwable> clazz, String messagePart)
throws IOException, URISyntaxException {
try {
SqlAdapter.parseSql(read(String.format("query-%d.sql", index)), scope);
failBecauseExceptionWasNotThrown(clazz);
} catch (Throwable t) {
assertThat(t).isInstanceOf(clazz);
assertThat(t).hasMessageContaining(messagePart);
}
}

private static void check(Scope scope, String queryResource, String expectedResource)
throws IOException, URISyntaxException {
checkSql(expectedResource, read(queryResource), scope);
Expand Down
7 changes: 7 additions & 0 deletions sql/src/test/resources/io/deephaven/sql/qst-36.dot
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
digraph {
"op_0" ["label"="ticketTable(scan/custom)"]
"op_1" ["label"="view(__p_0_0=Foo,__p_0_1=Bar)"]
"op_2" ["label"="view(Foo=__p_0_0,Bar=__p_0_1)"]
"op_1" -> "op_0"
"op_2" -> "op_1"
}
9 changes: 9 additions & 0 deletions sql/src/test/resources/io/deephaven/sql/qst-37.dot
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
digraph {
"op_0" ["label"="ticketTable(scan/custom)"]
"op_1" ["label"="view(__p_0_0=Foo,__p_0_1=Bar)"]
"op_2" ["label"="where(!isNull(__p_0_1))"]
"op_3" ["label"="view(Foo=__p_0_0,Bar=__p_0_1)"]
"op_1" -> "op_0"
"op_2" -> "op_1"
"op_3" -> "op_2"
}
9 changes: 9 additions & 0 deletions sql/src/test/resources/io/deephaven/sql/qst-38.dot
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
digraph {
"op_0" ["label"="ticketTable(scan/custom)"]
"op_1" ["label"="view(__p_1_0=Foo,__p_1_1=Bar)"]
"op_2" ["label"="view(Foo=__p_1_0,Bar=__p_1_1)"]
"op_3" ["label"="sort([ASCENDING(Bar)])"]
"op_1" -> "op_0"
"op_2" -> "op_1"
"op_3" -> "op_2"
}
4 changes: 4 additions & 0 deletions sql/src/test/resources/io/deephaven/sql/query-36.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
SELECT
*
FROM
custom
6 changes: 6 additions & 0 deletions sql/src/test/resources/io/deephaven/sql/query-37.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
SELECT
*
FROM
custom
WHERE
Bar is not NULL
6 changes: 6 additions & 0 deletions sql/src/test/resources/io/deephaven/sql/query-38.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
SELECT
*
FROM
custom
ORDER BY
Bar
4 changes: 4 additions & 0 deletions sql/src/test/resources/io/deephaven/sql/query-39.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
SELECT
Bar + 1
FROM
custom

0 comments on commit 9928db0

Please sign in to comment.