abhishekagarwal87 commented on pull request #11911:
URL: https://github.com/apache/druid/pull/11911#issuecomment-978121310
Surfacing the error in post-parsing validation can be done in some places
e.g. in MaxSQLAggregator, druid can tell calcite that string operands are not
supported. This can be done by returning a custom operator with supported types
in `calciteFunction()` instead of returning a standard operator from the
calcite library. I do not know how would we do that at other places such as an
unsupported join condition or union between incompatible data sources.
It is good to see these ideas coming in for how to surface these errors in a
very useful. There are two major areas of improvement I can see based on the
discussion so far
- one is the new error framework itself that is added in this PR. Here the
debate is about showcasing one error or multiple errors. There is also the
question of do we need this framework at all. I think we can try to minimize
using it but in some places, plannerContext is the only way to surface the
error.
- second is improving each error or scenario itself. By improving, I mean
making it precise and as informative as possible. I believe this will be an
ongoing exercise. In this PR, I replaced `ISE/IAE` with a custom exception to
surface many errors. Going one step ahead and using calcite to surface these
errors is even better as that is possible in some places. And we can continue
to improve these errors and make them more precise in future improvements. I do
not think it will be possible to do all of that in this PR itself. :)
So here are the open questions
- retaining last planning error or last X number of planning errors - I am
in the camp of former since I did want to see some real-world usage first
before even trying to do the latter. since these are errors, we can always
change them later on. That being said, I am not very strongly opposed to the
idea of retaining multiple planning errors either.
- HTTP response code for an unplannable query - Right now, we are returning
500 but should we return 400 instead? In fact, we do return 400 as well for
some planning errors. Should we treat CannotPlanException, not as an internal
server exception but as a bad query request?
- Delegating the operand validation to calcite in some of the aggregation
functions - This I can do in this PR itself.
- scope of the initial PR - I have attempted to clarify that in the first
few paragraphs. Happy to hear any other thoughts on it.
--
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]
