-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
fix(Jinja): Extra cache keys for Jinja columns #30715
Conversation
/testenv up |
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.
LGTM but I suggest using a type guard for the check
@geido Ephemeral environment spinning up at http://34.210.89.190:8080. Credentials are |
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.
LGTM, thanks for this critical fix!
Ephemeral environment shutdown and build artifacts deleted. |
@sadpandajoe I'm adding this PR to the release blockers based on this comment. |
Thank you so much for working on this!!! |
SUMMARY
This PR fixes an issue where Jinja in columns was not considered during cache key checks, leading to cache spillover.
TESTING INSTRUCTIONS
Vehicle Sales
dataset from theexamples
connection.a.
'{{current_user_id()}}'
asid
a.
'{{current_username()}}'
asusername
a.
'{{current_user_email()}}'
asemail
a.
'{{url_param('test', 'default')}}'
asurl_param
url_param
column will showdefault
(which is set in the macro to be returned when the param is not defined).&test=blah
to it (after thenative_filters_key
value) and access it.url_param
column should now how the param value (blah
).ADDITIONAL INFORMATION