Allow NewQueryEngine to return an error #319
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some errors only bubble up from pretty deep. A "SELECT COUNT(*)" was ending up as a panic, when instead it should have been returned as a query error to the user. This adds plumbing for returning an error from NewQueryEngine.
|
Also, I only tested this in local shard mode, so this is untested in a cluster setup. |
|
Thanks @zorkian for your contribution. It'd be appreciated to include tests with your pr to make it easier for us to validate the patch and make sure we don't have regressions in the future. I went ahead and added a test and merged your changes. I think some code changed on master that broke the propagation of the error, so i had to make these changes to fix the test case 7b74e5d Cheers, |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some errors only appear pretty deep. A "SELECT COUNT(*)" was ending up as a panic, when instead it should have been returned as a query error to the user. This adds plumbing for returning an error from NewQueryEngine.
Reproduction: execute a "SELECT COUNT(*) FROM proc.stat.cpu" and, pre-patch, you get a panic in your terminal and nothing in your browser. With this patch, you get "ERROR: function count() doesn't work with wildcards".
This also adds early-exit logic to
yieldSeriesDatawhere errors were being collected but not processed. I don't know if this is correct. All of my manual tests pass, and the non-coordinator tests seem to pass.