-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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(sql-lab): remove redundant onChange schema property #24422
fix(sql-lab): remove redundant onChange schema property #24422
Conversation
Codecov Report
@@ Coverage Diff @@
## master #24422 +/- ##
==========================================
- Coverage 68.96% 68.91% -0.06%
==========================================
Files 1904 1904
Lines 74088 73920 -168
Branches 8120 8119 -1
==========================================
- Hits 51096 50943 -153
+ Misses 20880 20866 -14
+ Partials 2112 2111 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
d89fa08
to
308c5c6
Compare
SUMMARY
While creating a Java Client from the the Superset OpenAPI spec with the OpenAPI codegen, a collision between the
changedOn
andchanged_on
properties in the SQL LabQueryRestultSchema
caused an error. This happened because it automatically converts the snake_case property to a camelCase one. This type of overlap doesn't exist in other schemas. In addition, when checking other similar schemas, all the other ones are of typefields.DateTime
, exceptSliceSchema
andChartEntityResponseSchema
, which werefields.String
. For this reason, the redundantchangedOn
property was removed fromQueryResultSchema
, and the type ofchanged_on
was updated toDateTime
forQueryResultSchema
,SliceSchema
andChartEntityResponseSchema
.Furthermore, when looking at the SQL Lab code, I noticed inconsistencies in the test fixtures and the reducer:
SouthPane
test didn't havechanged_on
in the fixture, onlychangedOn
. Furthermore, the type was wrong - the fixture had an epoch, while the value is in fact an ISO8601 string (see screenshot). These were updated.SqlLab
fixture also showed thechangedOn
property as an epoch. However, thechanged_on
property there was in correct format, i.e. ISO-8601 string. To comply with the updated schema,changedOn
was removed.changedOn
with an epoch-valuedqueriesLastUpdate
. I'm not sure what kind of results that resulted in, but to fix this, we now refer tochanged_on
, and convert it to epoch format first before doing the comparison.This inconsistency in types has been verified on both a SQLite and Postgres metastore backend, so it shouldn't be affected by the metastore's temporal column logic.
SCREENSHOTS
Currently on master, the values of
changedOn
andchanged_on
are identical in the query response:Notice how
queriesLastUpdate
is an epoch, butchangedOn
which it's being compared to in the reducer is an ISO-8601 string?TESTING INSTRUCTIONS
ADDITIONAL INFORMATION