abhishekagarwal87 edited a comment on pull request #11911:
URL: https://github.com/apache/druid/pull/11911#issuecomment-967036681
> The technique here is cool and an interesting way around the fact that
these rules can be "exploratory" and therefore they can't simply throw
exceptions.
>
> But, is there a potential for bizarre errors that relate to rules or query
types that were explored, but didn't work out, and appear like nonsense to the
user? We'd want to make sure that every time `setPlanningError` is called, it's
something so fundamental that the overall query can't be handled by a different
planner path. The idea is that we want to avoid a situation where there are two
paths A and B, and each has an error, and the error from A is reported but
doesn't make sense to the user because it is sort of path-A-specific.
Yes. It is possible. Here is one such example query
```
SELECT dim2, COUNT(distinct dim1), COUNT(distinct dim2) FROM druid.foo GROUP
BY dim2
```
This query should ideally throw an error that suggests that multiple exact
distinct count are not supported unless you set a custom flag. However, the
error that the user gets is below
```
Only equal conditions are supported in joins
```
This would, of course, look bizarre to end users since they have not put a
join in the query and it wouldn't be clear to them that a query with multiple
exact distinct counts could be planned using a join. I am thinking of some ways
of how we can make this experience better
- Be smarter about setting these errors and improve on a case-by-case basis.
E.g. in the above example, I might check if the join condition is of type
`is-distinct-from` and if so, I add an additional note to the error message
such as below
```
if you are using multiple exact distinct counts, please make sure to turn
flag X on.
```
- Use error codes alongside error messages and document any such gotchas in
our user documentation. So when user searches for an error code, they can get
more details about the error message and possible query shapes that could fail
with that error.
- Re-word the error such that the user knows it is a possible error but may
not be the actual reason. E.g. an error such as below
```
Possible error: Only equal conditions are supported in joins
```
None of these are perfect solutions but will minimize the confusion to a
good extent. Your suggestion about being conservative about setting the
planning error makes sense so long we cover most of the common SQL errors that
our users run into. any other suggestions will be helpful.
>
> In other words: we should make sure the error messages always refer to
things that the user actually wrote in their query, and that we know we don't
support. I suggest adding something about this to the javadocs for the method
so people don't add new ones that end up being confusing.
Yes. SGTM.
> Although, that also makes me think: if these errors are going to be really
unrecoverable, maybe we _should_ actually make them regular exceptions that
halt planning immediately…?
I thought about it but the risk of a false positive (falsely declaring a
query as unsupported) becomes very high with that approach. In the current
scheme, setting the planning error is a no-op if the query does get planned
eventually. If we are halting the planning immediately, we may end up killing
genuine queries. The current approach, I feel, is safer in that way. And in the
future, a developer, trying to add an error message, will not have to struggle
with the dilemma of breaking some genuine query in exchange for an informative
error message.
--
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]
