gianm commented on code in PR #17894:
URL: https://github.com/apache/druid/pull/17894#discussion_r2047759330
##########
docs/querying/sql.md:
##########
@@ -391,6 +391,26 @@ like `100` (denoting an integer), `100.0` (denoting a
floating point value), or
timestamps can be written like `TIMESTAMP '2000-01-01 00:00:00'`. Literal
intervals, used for time arithmetic, can be
written like `INTERVAL '1' HOUR`, `INTERVAL '1 02:03' DAY TO MINUTE`,
`INTERVAL '1-2' YEAR TO MONTH`, and so on.
+## SET statements
+
+Druid SQL over HTTP supports including 0 or more `SET` statements separated by
`;` preceding the statement to execute
+in order to assign [SQL query context parameter
values](../querying/sql-query-context.md).
+
+The syntax of a `SET` statement is
+
+```sql
+SET identifier = literal
Review Comment:
IMO, the example here should have a trailing `;` -- it's not technically
part of the statement, but it is in practice always required (since `SET` can
only appear before some other statement).
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java:
##########
@@ -283,6 +292,49 @@ public void close()
planner.close();
}
+ private SqlNode processStatementList(SqlNode root, String sql)
+ {
+ if (root instanceof SqlNodeList) {
+ final SqlNodeList nodeList = (SqlNodeList) root;
+ if (!allowSetStatementsToBuildContext && nodeList.size() > 1) {
+ throw InvalidSqlInput.exception("Multiple statements detected in SQL
string[%s], but only a single statement is supported",
Review Comment:
Echoing the entire string back in the error message probably isn't good,
since they can be quite long and all the bits after the statement ("but only a
single statement is supported") may be missed by the user. How about using a
message like: "SQL query string must contain a single statement only".
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java:
##########
@@ -283,6 +292,49 @@ public void close()
planner.close();
}
+ private SqlNode processStatementList(SqlNode root, String sql)
+ {
+ if (root instanceof SqlNodeList) {
+ final SqlNodeList nodeList = (SqlNodeList) root;
+ if (!allowSetStatementsToBuildContext && nodeList.size() > 1) {
+ throw InvalidSqlInput.exception("Multiple statements detected in SQL
string[%s], but only a single statement is supported",
+ sql
+ );
+ }
+ final Map<String, Object> contextMap = new LinkedHashMap<>();
+ boolean isMissingDruidStatementNode = true;
+ // convert 0 or more SET statements into a Map of stuff to add to the
query context
+ for (int i = 0; i < nodeList.size(); i++) {
+ SqlNode sqlNode = nodeList.get(i);
+ if (sqlNode instanceof SqlSetOption) {
+ final SqlSetOption sqlSetOption = (SqlSetOption) sqlNode;
+ if (!(sqlSetOption.getValue() instanceof SqlLiteral)) {
+ throw InvalidSqlInput.exception("Invalid sql SET statement[%s],
value must be a literal", sqlSetOption);
Review Comment:
What does the interpolated `sqlSetOption` end up looking like? I'm just
wondering if the error messages are going to be "nice".
SQL should be capitalized, btw.
I also wonder if it might be nicer to interpolate just the name of the
parameter.
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java:
##########
@@ -283,6 +292,49 @@ public void close()
planner.close();
}
+ private SqlNode processStatementList(SqlNode root, String sql)
+ {
+ if (root instanceof SqlNodeList) {
+ final SqlNodeList nodeList = (SqlNodeList) root;
+ if (!allowSetStatementsToBuildContext && nodeList.size() > 1) {
+ throw InvalidSqlInput.exception("Multiple statements detected in SQL
string[%s], but only a single statement is supported",
+ sql
+ );
+ }
+ final Map<String, Object> contextMap = new LinkedHashMap<>();
+ boolean isMissingDruidStatementNode = true;
+ // convert 0 or more SET statements into a Map of stuff to add to the
query context
+ for (int i = 0; i < nodeList.size(); i++) {
+ SqlNode sqlNode = nodeList.get(i);
+ if (sqlNode instanceof SqlSetOption) {
+ final SqlSetOption sqlSetOption = (SqlSetOption) sqlNode;
+ if (!(sqlSetOption.getValue() instanceof SqlLiteral)) {
+ throw InvalidSqlInput.exception("Invalid sql SET statement[%s],
value must be a literal", sqlSetOption);
+ }
+ final SqlLiteral value = (SqlLiteral) sqlSetOption.getValue();
+ contextMap.put(sqlSetOption.getName().getSimple(),
SqlResults.coerce(plannerContext.getJsonMapper(),
SqlResults.Context.fromPlannerContext(plannerContext), value.getValue(),
value.getTypeName(), "set"));
+ } else if (i < nodeList.size() - 1) {
+ // only SET statements can appear before the last statement
+ throw InvalidSqlInput.exception("Invalid sql statement list[%s] -
only SET statements are permitted before the final statement",
+ sql
+ );
+ } else {
+ // last SqlNode
+ root = sqlNode;
+ isMissingDruidStatementNode = false;
+ }
+ }
+ if (isMissingDruidStatementNode) {
+ throw InvalidSqlInput.exception(
+ "Invalid sql statement list[%s] - statement list must end with a
statement that is not a SET",
Review Comment:
Don't echo the entire `sql` in the error message for reasons discussed in
other comments. SQL should be capitalized.
##########
docs/querying/sql-query-context.md:
##########
@@ -60,19 +60,31 @@ For more information, see [Overriding default query context
values](../configura
## Set the query context
-You can configure query context parameters in the `context` object of the
[JSON API](../api-reference/sql-api.md) or as a [JDBC connection properties
object](../api-reference/sql-jdbc.md).
+You can configure query context parameters in the `context` object or as part
of the `query` string with [`SET`
Review Comment:
This has become kind of a run-on. Perhaps splitting it up into multiple
sentences or a bulleted list would be good.
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java:
##########
@@ -114,14 +120,16 @@ public AuthResult(
final FrameworkConfig frameworkConfig,
final PlannerContext plannerContext,
final SqlEngine engine,
- final PlannerHook hook
+ final PlannerHook hook,
+ boolean allowSetStatementsToBuildContext
Review Comment:
`final` please. It's good to be consistent and either use `final` on all
fields or none of them.
##########
sql/src/test/java/org/apache/druid/sql/avatica/DruidAvaticaHandlerTest.java:
##########
@@ -339,7 +339,7 @@ public void tearDown() throws Exception
public void testSelectCount() throws SQLException
{
try (Statement stmt = client.createStatement()) {
- final ResultSet resultSet = stmt.executeQuery("SELECT COUNT(*) AS cnt
FROM druid.foo");
+ final ResultSet resultSet = stmt.executeQuery("SELECT COUNT(*) AS cnt
FROM druid.foo;");
Review Comment:
nice
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java:
##########
@@ -283,6 +292,49 @@ public void close()
planner.close();
}
+ private SqlNode processStatementList(SqlNode root, String sql)
+ {
+ if (root instanceof SqlNodeList) {
+ final SqlNodeList nodeList = (SqlNodeList) root;
+ if (!allowSetStatementsToBuildContext && nodeList.size() > 1) {
+ throw InvalidSqlInput.exception("Multiple statements detected in SQL
string[%s], but only a single statement is supported",
+ sql
+ );
+ }
+ final Map<String, Object> contextMap = new LinkedHashMap<>();
+ boolean isMissingDruidStatementNode = true;
+ // convert 0 or more SET statements into a Map of stuff to add to the
query context
+ for (int i = 0; i < nodeList.size(); i++) {
+ SqlNode sqlNode = nodeList.get(i);
+ if (sqlNode instanceof SqlSetOption) {
+ final SqlSetOption sqlSetOption = (SqlSetOption) sqlNode;
+ if (!(sqlSetOption.getValue() instanceof SqlLiteral)) {
+ throw InvalidSqlInput.exception("Invalid sql SET statement[%s],
value must be a literal", sqlSetOption);
+ }
+ final SqlLiteral value = (SqlLiteral) sqlSetOption.getValue();
+ contextMap.put(sqlSetOption.getName().getSimple(),
SqlResults.coerce(plannerContext.getJsonMapper(),
SqlResults.Context.fromPlannerContext(plannerContext), value.getValue(),
value.getTypeName(), "set"));
+ } else if (i < nodeList.size() - 1) {
+ // only SET statements can appear before the last statement
+ throw InvalidSqlInput.exception("Invalid sql statement list[%s] -
only SET statements are permitted before the final statement",
Review Comment:
Don't echo the entire `sql` in the error message for reasons discussed in
other comments. SQL should be capitalized.
##########
docs/querying/sql.md:
##########
@@ -391,6 +391,26 @@ like `100` (denoting an integer), `100.0` (denoting a
floating point value), or
timestamps can be written like `TIMESTAMP '2000-01-01 00:00:00'`. Literal
intervals, used for time arithmetic, can be
written like `INTERVAL '1' HOUR`, `INTERVAL '1 02:03' DAY TO MINUTE`,
`INTERVAL '1-2' YEAR TO MONTH`, and so on.
+## SET statements
+
+Druid SQL over HTTP supports including 0 or more `SET` statements separated by
`;` preceding the statement to execute
+in order to assign [SQL query context parameter
values](../querying/sql-query-context.md).
+
+The syntax of a `SET` statement is
Review Comment:
should have trailing `:`
##########
sql/src/main/java/org/apache/druid/sql/DirectStatement.java:
##########
@@ -245,6 +240,17 @@ public ResultSet plan()
}
}
+ protected DruidPlanner getPlanner()
Review Comment:
nit: `createPlanner` is a better name; `getPlanner` suggests it's getting a
single thing (but this method creates a new planner each time).
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java:
##########
@@ -283,6 +292,49 @@ public void close()
planner.close();
}
+ private SqlNode processStatementList(SqlNode root, String sql)
Review Comment:
This method could use some javadoc about what is meant by "process" -- what
is it looking for and what does it return?
##########
docs/querying/sql.md:
##########
@@ -391,6 +391,26 @@ like `100` (denoting an integer), `100.0` (denoting a
floating point value), or
timestamps can be written like `TIMESTAMP '2000-01-01 00:00:00'`. Literal
intervals, used for time arithmetic, can be
written like `INTERVAL '1' HOUR`, `INTERVAL '1 02:03' DAY TO MINUTE`,
`INTERVAL '1-2' YEAR TO MONTH`, and so on.
+## SET statements
+
+Druid SQL over HTTP supports including 0 or more `SET` statements separated by
`;` preceding the statement to execute
+in order to assign [SQL query context parameter
values](../querying/sql-query-context.md).
Review Comment:
This should clarify that `SET` statements only affect the
immediately-following non-`SET` statement, and must be issued in the same HTTP
call as the non-`SET` statement. Otherwise people may assume that `SET` affects
all future statements they issue (kind of nonsensical since there are no
sessions in our HTTP API, but people may assume it anyway).
##########
extensions-contrib/spectator-histogram/src/main/java/org/apache/druid/spectator/histogram/NullableOffsetsHeader.java:
##########
@@ -308,7 +308,7 @@ int getOffsetIndex(int index)
offsetIndex += byteCardinality;
// After getting the cumulative cardinality upto the 64 bit boundary
immediately
- // preceeding the 64 bits that contains our index, we need to accumulate
the
+ // preceding the 64 bits that contains our index, we need to accumulate the
Review Comment:
ideally these comment fixes would be in their own, different commit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
