-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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: Address performance regression introduced in #11785 #20893
fix: Address performance regression introduced in #11785 #20893
Conversation
if not self.schema: | ||
return self.table_name | ||
return "{}.{}".format(self.schema, self.table_name) | ||
return self.schema + "." + self.table_name if self.schema else self.table_name |
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.
The logic needed to be translated into a function that SQLAlchemy could understand as we're using this hybrid property as a filter.
422a29d
to
37a19c4
Compare
Codecov Report
@@ Coverage Diff @@
## master #20893 +/- ##
===========================================
- Coverage 66.27% 54.80% -11.48%
===========================================
Files 1758 1758
Lines 67072 67072
Branches 7122 7122
===========================================
- Hits 44453 36759 -7694
- Misses 20800 28494 +7694
Partials 1819 1819
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. |
25dbfcd
to
64e5572
Compare
64e5572
to
a9d2e54
Compare
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.
thanks for the fix!
SUMMARY
This PR fixes a SQL Lab performance regression introduced in https://github.com/apache/superset/pull/11785—a mere two years ago—which impacts the populating the tables in the SQL Lab LHS.
Specially at Airbnb for the Presto database the following line
would take over 5 seconds to execute given we have over 100,000 tables registered with said database in Superset. The fix is to bypass the SQLAlchemy ORM and fetch only the subset of relevant tables using the SQLAlchemy Query API. The resulting refactor reduced the statement execution time down to << 0.01 seconds.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
CI.
ADDITIONAL INFORMATION