[GitHub] [druid] clintropolis commented on pull request #12418: Handling planning with alias for time for group by and order by

[GitBox <gi...@apache.org>]

clintropolis commented on PR #12418:
URL: https://github.com/apache/druid/pull/12418#issuecomment-1094560432

   I think changing `DruidRules` maybe isn't the best fix here, because what is going on is a bit of a mismatch of expectations on a method to check if a query is valid. [This code in `DruidRel`](https://github.com/apache/druid/blob/master/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidRel.java#L62) assumes that if a query can be explained without exploding, that it is a 'valid' druid query. [However, `DruidOuterQueryRel`](https://github.com/apache/druid/blob/master/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidOuterQueryRel.java#L117) doesn't actually explain the subquery, so cannot actually be treated as if the query is valid because full validation is deferred until the subquery is actually transformed (which is where the failure occurs). But because the query is "valid", the transforms happen and then explodes ,which is not caught by any of the rules which call this method. We would have to handle every call to `isValidDruidQuery` similarly with a try catch (wh
 ich already has it's own) if we don't fix the contract of the method itself I think.
   
   The change in this PR just makes `DruidOuterQueryRel` eagerly 'validate' the subquery by explaining it, so that the exception happens early and the 'is valid' check catches and returns false, which allows the planner to proceed to other plans. An alternative fix would be to change the valid check to transform to an actual druid query instead of using the explain method to validate, which is perhaps better if we think there are other rels that cannot be considered valid by just explaining, at least with their current explain implementations.


-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org