SQL Calcite #3630

devinrsmith · 2023-03-30T15:03:57Z

This is a large, self-contained PR that adds server-side SQL parsing and execution support.

The SQL implementation logic is largely self-contained in the sql package, and the public interface it exposes is very small. Its job is to translate SQL strings into TableSpec, but not to execute it. This keeps dependencies at a minimum, and allows for the usage of sql by java clients if they so desire.

The server-side SQL execution is the responsibility of the engine-sql package. The package is also wrapped in the deephaven.experimental.sql python module:

from deephaven import time_table
from deephaven.experimental.sql import execute_sql

my_table = time_table("00:00:01")

sql = """
SELECT 
  count(*) as my_count 
FROM 
  my_table
"""

out = execute_sql(sql)

This should be considered an experimental / minimial-viable-SQL offering - there is a lot of opportunity for improvements and optimizations in the future. Some of these have been captured with "SQLTODO"s with descriptions inline, as well as listed in sql/DEVELOPMENT.md.

niloc132

There are many TODOs that need to either be fixed, made into SQLTODOs, or reference bugs. Also, System.out.printlns.

Many classes could use javadoc, if only to briefly describe what they are doing to deal with their calcite counterparts - I think the VC summary you gave me would probably do the job for most of these.

I'm not sure that either project specifically belongs where it is - would these make more sense in extensions or java-client?

sql/src/main/java/io/deephaven/sql/LogicalProjectAdapter.java

niloc132 · 2023-03-30T17:22:25Z

sql/build.gradle

+ useJUnitPlatform()
+}
+
+// todo: should this be a java 8 project?


if so, is this intended to be part of java-client, rather than a new top-level project?

No; it's not meant to be part of the java-client. Ultimately, we are targetting server side SQL. That said, the conversion from String -> TableSpec is completely devoid of server side code, so it is a natural fit if the java client wants to make use out of it.

I'm going to make a better note.

This note was more about "Why would this be java 8, if it isnt meant to be designed for clients that can't update"

niloc132 · 2023-03-30T17:30:15Z

engine/sql/src/main/java/io/deephaven/engine/sql/Sql.java

+ @Override
+ public Table of(TicketTable ticketTable) {
+ final String scanTicket = new String(ticketTable.ticket(), StandardCharsets.UTF_8);
+ if (!scanTicket.startsWith("scan/")) {


here an TableScanAdapter should find a way to have this be a constant - for my money, i'm not sure this class belongs in engine itself.

I've updated the interface to give clear ownership to the calling side; the SqlAdapter layer won't be creating any TicketTables that the caller needs to know the format of.

engine/sql/src/main/java/io/deephaven/engine/sql/Sql.java

sql/src/main/java/io/deephaven/sql/SqlFunctions.java

engine/sql/src/main/java/io/deephaven/engine/sql/Sql.java

qst/src/main/java/io/deephaven/qst/table/TableLabelVisitor.java

server/jetty-app/src/main/resources/logback-minimal.xml

server/jetty-app/src/main/resources/logback.xml

sql/IMPLEMENTATION.md

rcaudy · 2023-05-23T16:10:55Z

sql/src/test/java/io/deephaven/sql/SqlAdapterTest.java

+import java.nio.file.Path;
+import java.util.List;
+
+import static org.assertj.core.api.Assertions.assertThat;


Moving from numbers to descriptive names would make this better and more maintainable.

or maybe named constants in the test class

sql/src/test/resources/io/deephaven/sql/query-12.sql

sql/src/main/java/io/deephaven/sql/SqlAdapter.java

sql/src/main/java/io/deephaven/sql/SqlRootContext.java

sql/src/main/java/io/deephaven/sql/IndexRef.java

sql/src/main/java/io/deephaven/sql/LogicalSortAdapter.java

…s case-sensitivity

rcaudy

Finished re-review of files I looked at on Tuesday

qst/src/main/java/io/deephaven/qst/table/TableLabelVisitor.java

Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java

py/server/deephaven/table.py

sql/src/main/java/io/deephaven/sql/ExtractAnds.java

sql/src/main/java/io/deephaven/sql/LiteralAdapter.java

sql/src/main/java/io/deephaven/sql/RexNodeExpressionAdapterImpl.java

sql/src/main/java/io/deephaven/sql/RelNodeVisitorAdapter.java

rcaudy · 2023-05-25T20:25:31Z

I think it's very important that we write a comprehensive, user-facing document explaining what we support, what we want to, what we never will, etc.

sql/src/main/java/io/deephaven/sql/LiteralAdapter.java

sql/src/main/java/io/deephaven/sql/TypeAdapter.java

rcaudy · 2023-05-26T16:01:32Z

sql/src/main/java/io/deephaven/sql/TypeAdapter.java

+
+ @Override
+ public void visit(FloatType floatType) {
+ out = create(SqlTypeName.FLOAT);


Is this correct?

Good call. Should be REAL.

…ble-headers-out)

py/server/deephaven/experimental/sql.py

chipkent · 2023-05-30T20:03:33Z

py/server/tests/test_sql.py

+ result_table = sql.eval("SELECT CURRENT_TIMESTAMP")
+ self.assertEqual(result_table.size, 1)
+
+


We tested a few other scoping scenarios for the table methods. Consider testing some of those here as well. See for example the stuff around this line in the test file.
https://github.com/deephaven/deephaven-core/blob/main/py/server/tests/test_table.py#L730

The scoping for SQL is a bit more limited, as it's limited to only referencing table names. Custom functions are on the roadmap, and we'd be able to get more tests of this kind then.

The test cases I'm referring to as examples make sure that query scope is properly obtained from the correct Python scope.

I've added a new test, but it's not really testing anything new - is there another specific SQL test you want to see?

py/server/deephaven/experimental/sql.py

chipkent · 2023-05-30T20:17:17Z

py/server/deephaven/experimental/sql.py

+ j_py_script_session.popScope()
+
+
+def eval(


eval is a built-in python function, so we are better off renaming this to sql_eval or something else.

The naming was intentional, as I think the built-in eval is very similar in spirit to evaluating SQL (thus, also why I exposed globals/locals as params). I'm hoping we always reference it qualified by the sql module:

from deephaven.experimental import sql sql.eval(...)

Or, I could get behind the more explicit name evaluate(...) if we prefer it to be "unique".

The user can always do that themselves:
from deephaven.experimental.sql import eval as sql_eval

Overwriting a built-in function is generally considered to be bad practice.

I've changed the function name to "evaluate"

py/server/deephaven/experimental/sql.py

rcaudy

I'm content to merge this, once the Python crowd is on board.

jmao-denver

LGTM

deephaven-internal · 2023-06-02T16:16:18Z

Labels indicate documentation is required. Issues for documentation have been opened:

How-to: https://github.com/deephaven/deephaven.io/issues/2707
Conceptual: https://github.com/deephaven/deephaven.io/issues/2706
Reference: https://github.com/deephaven/deephaven.io/issues/2705
Blog: https://github.com/deephaven/deephaven.io/issues/2708

devinrsmith requested a review from niloc132 March 30, 2023 15:03

devinrsmith self-assigned this Mar 30, 2023

niloc132 reviewed Mar 30, 2023

View reviewed changes

devinrsmith marked this pull request as ready for review May 19, 2023 19:23

devinrsmith force-pushed the calcite branch from 741a57b to 458b9ba Compare May 19, 2023 19:23

devinrsmith requested review from chipkent, jmao-denver, rcaudy, kosak and jcferretti as code owners May 19, 2023 19:23

devinrsmith added DocumentationNeeded ReleaseNotesNeeded Release notes are needed labels May 19, 2023

devinrsmith added this to the May 2023 milestone May 19, 2023

devinrsmith changed the base branch from devin-tmp/filter-condition-expression to main May 19, 2023 19:24

devinrsmith removed request for kosak and jcferretti May 19, 2023 19:25

Add experimental server-side SQL parsing and execution support

d87e228

devinrsmith force-pushed the calcite branch from 458b9ba to d87e228 Compare May 23, 2023 15:21

rcaudy reviewed May 23, 2023

View reviewed changes

devinrsmith added 4 commits May 23, 2023 09:36

Graphviz test fixes

cf0fb9e

Fix TabelLabelVisitor

e4aed53

Add dry run for SQL

76bdf64

Quick IMPLEMENTATION.md fix

3ce6764

rcaudy reviewed May 23, 2023

View reviewed changes

devinrsmith added 6 commits May 23, 2023 14:50

Remove extraneous String.format calls

e9a0a89

Guard debug logging

567031c

Use ViewExpanders.simpleContext instead of new ViewExpander impl

27f5243

Updated error message

9f25598

Use Lex.JAVA, which preserves unquoted identifier casing, and enforce…

50d280e

…s case-sensitivity

Change lexing back to Quoting.DOUBLE_QUOTE

ac1520b

Merge remote-tracking branch 'upstream/main' into calcite

04e828d

rcaudy reviewed May 25, 2023

View reviewed changes

qst/src/main/java/io/deephaven/qst/table/TableLabelVisitor.java Outdated Show resolved Hide resolved

Sql python testing

01d8a7d

rcaudy reviewed May 25, 2023

View reviewed changes

review stuff

38beee7

devinrsmith requested a review from rcaudy May 26, 2023 00:07

fix test

805e885

rcaudy reviewed May 26, 2023

View reviewed changes

sql/src/main/java/io/deephaven/sql/LiteralAdapter.java Show resolved Hide resolved

rcaudy reviewed May 26, 2023

View reviewed changes

Fix TypeAdapter. Add note about improving return type with SQLTODO(ta…

1c7a353

…ble-headers-out)

devinrsmith requested a review from rcaudy May 30, 2023 16:31

jmao-denver reviewed May 30, 2023

View reviewed changes

py/server/deephaven/experimental/sql.py Outdated Show resolved Hide resolved

devinrsmith requested a review from jmao-denver May 30, 2023 17:19

chipkent requested changes May 30, 2023

View reviewed changes

Remove explicit scopes from sql.eval method

a6b4675

devinrsmith requested a review from chipkent May 30, 2023 23:19

rcaudy previously approved these changes May 31, 2023

View reviewed changes

jmao-denver previously approved these changes Jun 1, 2023

View reviewed changes

Review responses

8800b35

devinrsmith dismissed stale reviews from jmao-denver and rcaudy via 8800b35 June 1, 2023 22:31

Merge remote-tracking branch 'upstream/main' into calcite

e894af0

rcaudy approved these changes Jun 2, 2023

View reviewed changes

chipkent approved these changes Jun 2, 2023

View reviewed changes

devinrsmith merged commit 38b2e1c into deephaven:main Jun 2, 2023

devinrsmith deleted the calcite branch June 2, 2023 16:16

github-actions bot locked and limited conversation to collaborators Jun 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL Calcite #3630

SQL Calcite #3630

devinrsmith commented Mar 30, 2023 •

edited

Loading

niloc132 left a comment

niloc132 Mar 30, 2023

devinrsmith May 18, 2023

niloc132 May 25, 2023

niloc132 Mar 30, 2023

devinrsmith May 19, 2023

rcaudy May 23, 2023

niloc132 May 25, 2023

rcaudy left a comment

rcaudy commented May 25, 2023

rcaudy May 26, 2023

devinrsmith May 26, 2023

chipkent May 30, 2023

devinrsmith May 30, 2023

chipkent Jun 1, 2023

devinrsmith Jun 1, 2023

chipkent May 30, 2023

devinrsmith May 30, 2023 •

edited

Loading

rcaudy May 31, 2023

chipkent Jun 1, 2023

devinrsmith Jun 1, 2023

rcaudy left a comment

jmao-denver left a comment

deephaven-internal commented Jun 2, 2023

		result_table = sql.eval("SELECT CURRENT_TIMESTAMP")
		self.assertEqual(result_table.size, 1)

SQL Calcite #3630

SQL Calcite #3630

Conversation

devinrsmith commented Mar 30, 2023 • edited Loading

niloc132 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcaudy left a comment

Choose a reason for hiding this comment

rcaudy commented May 25, 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

Choose a reason for hiding this comment

devinrsmith May 30, 2023 • edited Loading

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

rcaudy left a comment

Choose a reason for hiding this comment

jmao-denver left a comment

Choose a reason for hiding this comment

deephaven-internal commented Jun 2, 2023

devinrsmith commented Mar 30, 2023 •

edited

Loading

devinrsmith May 30, 2023 •

edited

Loading