-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Attempt at fixing scheduled parameterized queries #6063
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
@myonlylonely would you be willing to test this PR and see if the changes fix the original issue (from 2019! 😬 )? @remil1000 , would you mind merging |
@guidopetri |
Greetings, I tried to first rebase but finally updated the whole MR content, description and added some explanation Tested locally with a regular query and a dashboard - the Please let me know if you can't reproduce |
I tested the latest PR, queries with or without parameters both works! Great job! 👍 |
@myonlylonely thanks for testing! @remil1000 unfortunately it looks like quite a few of our backend unit tests are failing - would you mind taking a look? |
@eradman 😅 |
I'll aim to test this change out early next week |
- generating query_hash with query expanded with default parameters values to match query_result with queries hash - added default parameters related properties in Query model class
@remil1000 I rebased this change and the backend unit tests are failing. Can you investiate? |
Tested this change; it does seem to work! Queries with parameters are refreshed Logs
I'll take a closer look at the test failures next week |
@@ -810,6 +810,18 @@ def lowercase_name(cls): | |||
def parameters(self): | |||
return self.options.get("parameters", []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests require that we handle the case where options is NULL
This issue was resolved in #6683 Closing, but we would be interested to know how this works for others. |
What type of PR is this?
Description
How is this tested?
Tested queries with parameters and without, through dashboard and not and with changing parameter for ad-hoc query run/refresh
Extra info
The query hash is generated by removing any SQL comment and striping spaces then getting the md5 of the query text. This PR does the same only replacing parameters in braces
{{ }}
by their default valueExample below with a simple remote JSON API and parameter being
{{ n }}
, the participants number - the other is a static/no param JSON API exampleRelated Tickets & Documents
#4182