Allow DESC ordering in window expressions#15195
Conversation
kgyrtkirk
commented
Oct 18, 2023
kgyrtkirk
commented
- allow DESC ordering in window expressions
- allow and accept NULLS FIRST / NULLS LAST options if they align with the actual ordering mode ; and report if they are unsupported
- been self-reviewed.
- added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
* allow DESC ordering in window expressions * allow and accept NULLS FIRST / NULLS LAST options if they align with the actual ordering mode ; and report if they are unsupported
imply-cheddar
left a comment
imply-cheddar
left a comment
There was a problem hiding this comment.
Aside from the comment about ensuring there's enough context to make sense of the error message, it generally looks good. So, going to go ahead and approve it, but do please make sure that there's context in the error message (fwiw, if you go the ValidationException route, you can validate that the locations exist as they will be on the context of the DruidException and you can validate using expectContext on the DruidExceptionMatcher.
| 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!"); |
There was a problem hiding this 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?
There was a problem hiding this comment.
initially I've used use validationError....but then I've seen a lot of place where InvalidSqlInput was used in similar situations like here in the DruidSqlParserUtils; and I was just "following the current practices"
There was a problem hiding this comment.
I think it would probably make more sense to leave it like this for now - and instead of just fixing this one instance - replace all possible InvalidSqlInput -s with an alternate (which could pinpoint the sql location) and use that everywhere.
There was a problem hiding this comment.
I think there are some places where InvalidSqlInput can't really point to a sql location. I agree with the fact that we can take that up as a whole, but I agree with Eric that we should provide some context info in the error message. If we are to go with the InvalidSqlInput way, we would add context to it like https://github.com/apache/druid/blob/master/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java#L421
What do you think?
There was a problem hiding this comment.
yeah; I guessed there will be some which can't be changed - however at a lot of callsites we do have an SqlNode ; I had in mind to target those
| 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)"); |
There was a problem hiding this 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.
There was a problem hiding this comment.
Could be extended - but I don't really see the reason to do that right now: as so far the exception message matching was enough - it at some point it will need to optionally access some other things ; this can be extended/changed/etc
note that I don't want to give better access to these things from the annotation @NotYetSupported either ; so that these enum entries are more-or-less mandatory - so that they could also act as natural magnets for similar issues :)
|
Thanks a lot for the changes @kgyrtkirk 👍 |
(cherry picked from commit fbbb9c7)
|
Thank you @imply-cheddar and @rohangarg for reviewing and merging the changes! |
4 participants
