perf(sqla): avoid unnecessary type check on adhoc column #23491
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
villebro
changed the title
zhaoyongjie
approved these changes
Mar 28, 2023
villebro
force-pushed
the
villebro/base_axis_timegrain
branch
from
2f32401
to
de0c2bf
Compare
Codecov Report
@@ Coverage Diff @@
## master #23491 +/- ##
==========================================
- Coverage 67.66% 67.53% -0.14%
==========================================
Files 1914 1914
Lines 73936 73962 +26
Branches 8022 8029 +7
==========================================
- Hits 50028 49949 -79
- Misses 21864 21967 +103
- Partials 2044 2046 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 9 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
villebro
changed the title
villebro
force-pushed
the
villebro/base_axis_timegrain
branch
from
ec6fa0f
to
3a8b4db
Compare
villebro
commented
Mar 31, 2023
| sqla_col = self.adhoc_column_to_sqla( | ||
| col=flt_col, | ||
| force_type_check=True, | ||
| template_processor=template_processor, |
There was a problem hiding this comment.
bycatch - the template processor was missing here
villebro
commented
Mar 31, 2023
| @@ -28,7 +28,7 @@ | |||
|
|
|||
|
|
|||
| @validate_column_args("index", "columns") | |||
| def pivot( # pylint: disable=too-many-arguments,too-many-locals | |||
| def pivot( # pylint: disable=too-many-arguments | |||
There was a problem hiding this comment.
bycatch: a warning that pylint picked up but wasn't regarded as an error
kgabryje
approved these changes
Mar 31, 2023
mistercrunch
added
🏷️ bot
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.
SUMMARY
PR #21163 introduced support for time grains to the base axis for both regular and adhoc columns. This caused a performance regression that executes a query to the analytical database for every adhoc column to check if it's a temporal expression or not.
This PR changes the logic slightly so that the query is executed only if needed, i.e. the base column is adhoc and has a time grain, or if the adhoc column is part of an adhoc filter. To review the PR, please use "Hide whitespace" to see the real changes more clearly.
AFTER
On a chart with two normal adhoc columns, the query now executes in 7 seconds with only a single query being sent to the database:
The same chart previously executed in 22 seconds (rougly 3 x 7 seconds with 3 queries sent to the database):
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION