imply-cheddar commented on code in PR #15195:
URL: https://github.com/apache/druid/pull/15195#discussion_r1364780202
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlValidator.java:
##########
@@ -39,4 +44,23 @@ protected DruidSqlValidator(
{
super(opTab, catalogReader, typeFactory, validatorConfig);
}
+
+ @Override
+ public void validateCall(SqlCall call, SqlValidatorScope scope)
+ {
+ if (call.getKind() == SqlKind.NULLS_FIRST) {
+ SqlNode op0 = call.getOperandList().get(0);
+ if (op0.getKind() == SqlKind.DESCENDING) {
+ throw InvalidSqlInput.exception("DESCENDING ordering with NULLS FIRST
is not supported!");
Review Comment:
I've had experiences with really large SQL queries where a message like this
tells me what's wrong, but doesn't always inform me of where I made the
mistake. When we convert a `ValidationException` from Calcite into an
`InvalidSqlInput` exception, it comes with the location of the SQL issue, which
is helpful context. I wonder if either
a) We can add more context here to the error message to help identify which
part of a potentially huge, sprawling SQL statement has an issue, OR
b) Maybe in the `validateCall()` method it's correct to throw a
ValidationException to get the Calcite system to add the context of where it
happened and then rely on the normal conversion of that to the
`InvalidSqlInput` exception on the way out?
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java:
##########
@@ -14247,4 +14250,24 @@ public void
testLatestByOnStringColumnWithoutMaxBytesSpecified()
new Object[] {"abc", defaultString, "def", defaultString, "def",
defaultString}
));
}
+
+ @Test
+ public void testUnSupportedNullsFirst()
+ {
+ DruidException e = assertThrows(DruidException.class, () -> testBuilder()
+ .queryContext(ImmutableMap.of(PlannerContext.CTX_ENABLE_WINDOW_FNS,
true))
+ .sql("SELECT dim1,ROW_NUMBER() OVER (ORDER BY dim1 DESC NULLS FIRST)
from druid.foo")
+ .run());
+ assertEquals("DESCENDING ordering with NULLS FIRST is not supported!",
e.getMessage());
Review Comment:
There is a `DruidExceptionMatcher` which you can use to build matchers that
test things like the persona and other parts of the exception which are also
important. On this test class, there are static helper methods
`invalidSqlIs()` and `invalidSqlContains()` which will build a matcher checking
the message. Please use those instead.
##########
sql/src/test/java/org/apache/druid/sql/calcite/NotYetSupported.java:
##########
@@ -88,7 +88,8 @@ enum Modes
INCORRECT_SYNTAX(DruidException.class, "Incorrect syntax near the
keyword"),
// at least c7 is represented oddly in the parquet file
T_ALLTYPES_ISSUES(AssertionError.class,
"(t_alltype|allTypsUniq|fewRowsAllData).parquet.*Verifier.verify"),
- RESULT_MISMATCH(AssertionError.class, "assertResultsEquals");
+ RESULT_MISMATCH(AssertionError.class, "assertResultsEquals"),
+ UNSUPPORTED_NULL_ORDERING(DruidException.class, "(A|DE)SCENDING ordering
with NULLS (LAST|FIRST)");
Review Comment:
Stream of consciousness: Looking at these definitions and thinking about the
comment above about the `DruidExceptionMatcher`. I wonder if this enum
shouldn't take a `Matcher<? extends Throwable>` instead of the class and
message regex.
--
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]
