Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error handling during determinism analysis #15056

Merged
merged 1 commit into from
Aug 25, 2020

Conversation

caithagoras
Copy link
Contributor

@caithagoras caithagoras commented Aug 19, 2020

We rerun control queries first and then the limit query analysis,

In one case, we see a determinism analysis failed because the control
rerun failed due to a transient generic internal error. The query
contains a LIMIT clause, but limit query analysis was never run as
the exception is caught and the analysis is marked as
ANALYSIS_FAILED_QUERY_FAILURE.

To mitigate this issue:

  • Run limit query analysis before control reruns, since it is a
    light-weight analysis.
  • If analysis query is failing during limit query analysis, do
    not return and proceed to the control reruns.
== RELEASE NOTES ==

Verifier Changes
* Fix an issue during determinism analysis where queries with LIMIT clause are not identified
  as non-deterministic when a rerun of the control query fails.

We rerun control queries first and then the limit query analysis,

In one case, we see a determinism analysis failed because the control
rerun failed due to a transient generic internal error. The query
contains a LIMIT clause, but limit query analysis was never run as
the exception is caught and the analysis is marked as
ANALYSIS_FAILED_QUERY_FAILURE.

To mitigate this issue:
- Run limit query analysis before control reruns, since it is a
  light-weight analysis.
- If analysis query is failing during limit query analysis, do
  not return and proceed to the control reruns.
Copy link
Contributor

@ajaygeorge ajaygeorge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@caithagoras
Copy link
Contributor Author

cc @mbasmanova for committer approval. Thanks!

@caithagoras
Copy link
Contributor Author

cc @wenleix @rschlussel Can you take final look since @ajaygeorge has already reviewed the diff? Thanks!

@caithagoras caithagoras merged commit f8793a7 into prestodb:master Aug 25, 2020
@caithagoras caithagoras deleted the s2 branch August 25, 2020 17:48
@caithagoras caithagoras mentioned this pull request Sep 9, 2020
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants