kgyrtkirk commented on code in PR #14510:
URL: https://github.com/apache/druid/pull/14510#discussion_r1289776535
##########
sql/pom.xml:
##########
@@ -258,6 +257,11 @@
<artifactId>jdbi</artifactId>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.immutables</groupId>
+ <artifactId>value-annotations</artifactId>
+ <version>2.8.8</version>
Review Comment:
I think this dep should be handled via `dependencyManagement`:
* add this whole section with the version to the root `pom.xml` in:
`project/dependencyManagement/dependencies`
* remove the version from this dependency declaration
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidTypeSystem.java:
##########
@@ -120,7 +124,13 @@ public RelDataType deriveAvgAggType(
final RelDataType argumentType
)
{
- return RelDataTypeSystem.DEFAULT.deriveAvgAggType(typeFactory,
argumentType);
+ // Widen all averages to 64-bits regardless of the size of the inputs.
+
+ if (SqlTypeName.INT_TYPES.contains(argumentType.getSqlTypeName())) {
+ return Calcites.createSqlTypeWithNullability(typeFactory,
SqlTypeName.BIGINT, argumentType.isNullable());
+ } else {
+ return Calcites.createSqlTypeWithNullability(typeFactory,
SqlTypeName.DOUBLE, argumentType.isNullable());
+ }
Review Comment:
it seems to me that right now `DOUBLE` will be returned for everything which
is not in `INT_TYPES` - which could potentially lure it into some problematic
case.
It might be probably better to handle `APPROX_TYPES` as returning `DOUBLE` -
and decide what to do with other case:
* dispatch it to `RelDataTypeSystem.DEFAULT.deriveAvgAggType(typeFactory,
argumentType);`
* ...or throw an exception to raise attention if other cases should be
unlikely to happen
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidTypeSystem.java:
##########
@@ -108,9 +112,9 @@ public RelDataType deriveSumType(final RelDataTypeFactory
typeFactory, final Rel
// Widen all sums to 64-bits regardless of the size of the inputs.
if (SqlTypeName.INT_TYPES.contains(argumentType.getSqlTypeName())) {
- return Calcites.createSqlType(typeFactory, SqlTypeName.BIGINT);
+ return Calcites.createSqlTypeWithNullability(typeFactory,
SqlTypeName.BIGINT, argumentType.isNullable());
} else {
- return Calcites.createSqlType(typeFactory, SqlTypeName.DOUBLE);
+ return Calcites.createSqlTypeWithNullability(typeFactory,
SqlTypeName.DOUBLE, argumentType.isNullable());
Review Comment:
same note as on `deriveAvgAggType`
##########
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQReplaceTest.java:
##########
@@ -329,8 +329,7 @@ public void testReplaceIncorrectSyntax()
.setExpectedDataSource("foo1")
.setQueryContext(context)
.setExpectedValidationErrorMatcher(invalidSqlContains(
- "Missing time chunk information in OVERWRITE clause for REPLACE. "
Review Comment:
this test seemed to be designed to check that in case there are missing
pieces after the `OVERWRITE` keyword - a good [helpfull
error](https://github.com/apache/druid/blob/4b9846b90fd4ca977c9073e80b993a9259f70240/sql/src/main/java/org/apache/druid/sql/calcite/planner/IngestHandler.java#L326-L329)
is being emitted...
##########
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQReplaceTest.java:
##########
@@ -329,8 +329,7 @@ public void testReplaceIncorrectSyntax()
.setExpectedDataSource("foo1")
.setQueryContext(context)
.setExpectedValidationErrorMatcher(invalidSqlContains(
- "Missing time chunk information in OVERWRITE clause for REPLACE. "
Review Comment:
looking at the history it seems like: `OVERWRITE` was added as a `keyword`
and `nonReservedKeyword` as well to `config.fmpp` in this
[commit](https://github.com/apache/druid/commit/39b3487aa9829d1d5b19733ef76319d8133dcb38)
I've spent some time fixing this...and its half-as-fun as it seems like :D
<details>
<summary>this patch could restore the old error message</summary>
<detail>
```
diff --git
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQReplaceTest.java
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQReplaceTest.java
index f951d24f6c..80d6719f12 100644
---
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQReplaceTest.java
+++
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQReplaceTest.java
@@ -329,7 +329,8 @@ public class MSQReplaceTest extends MSQTestBase
.setExpectedDataSource("foo1")
.setQueryContext(context)
.setExpectedValidationErrorMatcher(invalidSqlContains(
- "Incorrect syntax near the keyword 'OVERWRITE'"
+ "Missing time chunk information in OVERWRITE clause for
REPLACE. "
+ + "Use OVERWRITE WHERE <__time based condition> or OVERWRITE
ALL to overwrite the entire table."
))
.verifyPlanningErrors();
}
diff --git sql/src/main/codegen/config.fmpp sql/src/main/codegen/config.fmpp
index 0dfdef46b2..87195131b5 100644
--- sql/src/main/codegen/config.fmpp
+++ sql/src/main/codegen/config.fmpp
@@ -67,6 +67,10 @@ data: {
"PARTITIONED"
]
+ nonReservedKeywordsToAdd: [
+ "OVERWRITE"
+ ]
+
# List of methods for parsing custom SQL statements.
# Return type of method implementation should be 'SqlNode'.
# Example: SqlShowDatabases(), SqlShowTables().
diff --git sql/src/main/codegen/includes/replace.ftl
sql/src/main/codegen/includes/replace.ftl
index 1af45cb769..33d8070467 100644
--- sql/src/main/codegen/includes/replace.ftl
+++ sql/src/main/codegen/includes/replace.ftl
@@ -42,8 +42,9 @@ SqlNode DruidSqlReplaceEof() :
}
}
]
+ <OVERWRITE>
[
- <OVERWRITE> replaceTimeQuery = ReplaceTimeQuery()
+ replaceTimeQuery = ReplaceTimeQuery()
]
source = OrderedQueryOrExpr(ExprContext.ACCEPT_QUERY)
// PARTITIONED BY is necessary, but is kept optional in the grammar. It
is asserted that it is not missing in the
```
<detail>
<details>
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidTypeSystem.java:
##########
@@ -120,7 +124,13 @@ public RelDataType deriveAvgAggType(
final RelDataType argumentType
)
{
- return RelDataTypeSystem.DEFAULT.deriveAvgAggType(typeFactory,
argumentType);
+ // Widen all averages to 64-bits regardless of the size of the inputs.
+
+ if (SqlTypeName.INT_TYPES.contains(argumentType.getSqlTypeName())) {
+ return Calcites.createSqlTypeWithNullability(typeFactory,
SqlTypeName.BIGINT, argumentType.isNullable());
+ } else {
+ return Calcites.createSqlTypeWithNullability(typeFactory,
SqlTypeName.DOUBLE, argumentType.isNullable());
+ }
Review Comment:
I don't know what are the benefits of widening the return type of `AVG`
aggregates - I wonder if this change is strictly necessary for the upgrade - or
it could be done separately?
##########
sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java:
##########
@@ -682,7 +682,7 @@ private static RowSignature computeOutputRowSignature(
}
}
- private VirtualColumns getVirtualColumns(final boolean includeDimensions)
Review Comment:
I was curious why this is neccessary - but the removal of `private` is not
needed
--
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]
