gianm commented on pull request #11911: URL: https://github.com/apache/druid/pull/11911#issuecomment-966584311
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. 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. 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…? -- 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]
