-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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(sqllab): Replace stringified 'null' schema column values with NULL #18992
fix(sqllab): Replace stringified 'null' schema column values with NULL #18992
Conversation
__tablename__ = "saved_query" | ||
|
||
id = Column(Integer, primary_key=True) | ||
schema = Column(String(128)) |
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.
It's not evident why these tables use different string sizes for encoding the schema
.
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.
yikes
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.
can you run Beto's db migration benchmarking script and post the results?
__tablename__ = "saved_query" | ||
|
||
id = Column(Integer, primary_key=True) | ||
schema = Column(String(128)) |
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.
yikes
Codecov Report
@@ Coverage Diff @@
## master #18992 +/- ##
==========================================
- Coverage 66.58% 66.39% -0.20%
==========================================
Files 1641 1641
Lines 63524 63524
Branches 6421 6421
==========================================
- Hits 42299 42178 -121
- Misses 19548 19669 +121
Partials 1677 1677
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
@etr2460 I've addressed your comment. |
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 the cleanup migration
SUMMARY
I'm not sure exactly when this came about, though I believe the issue has been resolved recently (albeit not being able to identify the relevant PR(s)), but it seems at some stage we were stringifying an undefined (
NULL
) schema in SQL Lab as"null"
in both thequery
andsaved_query
tables resulting in some undesirable user behavior.This PR includes a migration which simply replaces the
null
string withNULL
, i.e., undefined.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
CI and ran the upgrade/downgrade manually. Also ran the bench-marking script (thanks @betodealmeida) which produced the following results:
ADDITIONAL INFORMATION