-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
feat: add generic type to column payload #14547
Conversation
Codecov Report
@@ Coverage Diff @@
## master #14547 +/- ##
=======================================
Coverage 77.46% 77.46%
=======================================
Files 959 959
Lines 48450 48450
Branches 5680 5680
=======================================
Hits 37532 37532
Misses 10718 10718
Partials 200 200
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
1e44a08
to
181ab7d
Compare
/testenv up |
@junlincc Ephemeral environment spinning up at http://54.188.138.197:8080. Credentials are |
thank you Ville! for some reasons i am still seeing "?" as the icon locally |
The question mark won't go away until apache-superset/superset-ui#1102 is released and bumped on the main repo. But since this is not a breaking change I think it makes sense to merge this first to avoid having to make a |
181ab7d
to
a1625ac
Compare
gotcha, @zhaoyongjie please help review, i wanna get this PR and apache-superset/superset-ui#1102 in soon. 🙏 @villebro can we get the test pass? |
6f0615b
to
b4b79e6
Compare
/testenv up |
hmmm still seeing "?" this one seems low risk, i am comfortable of merging if code is good. will look at it right it's in master |
@junlincc Ephemeral environment spinning up at http://18.236.153.44:8080. Credentials are |
b4b79e6
to
545b954
Compare
superset-frontend/cypress-base/cypress/integration/dashboard/filter.test.ts
Outdated
Show resolved
Hide resolved
545b954
to
f7f252f
Compare
f7f252f
to
8853b1a
Compare
.get(`[data-test="chart-grid-component"][data-test-chart-name="${name}"]`) | ||
.get(`[data-test="chart-grid-component"][data-test-chart-name="${name}"]`, { | ||
timeout: 30000, | ||
}) |
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.
This was resolving very close to the default 5 secs, and apparently started causing widespread flakiness on cypress. I bumped this to the 30s seen a few lines below, now all tests pass.
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.
Ephemeral environment shutdown and build artifacts deleted. |
@@ -29,7 +29,8 @@ export const columns: ColumnMeta[] = [ | |||
id: 516, | |||
is_dttm: false, | |||
python_date_format: null, | |||
type: ColumnType.DOUBLE, | |||
type: 'DOUBLE', |
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.
Why don't we use ColumnType
here anymore? Comment applies to all other "ColumnType -> hardcoded string" changes
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.
ColumnType
has been removed from superset-ui/core
and replaced with string
, as it's the raw column type coming back from the database. ColumnType
was really just a generalization that mostly complied with ANSI SQL types, but this was not a scalable solution, as the frontend mostly isn't aware of which database is being used. As an example, the datatype DOUBLE PRECISION
wasn't being picked up on the frontend due to it being a datatype that's mostly only available on dialects of Postgres. In corner cases two datatypes with the same name can mean different things on different databases (the boolean type on MySQL comes to mind, which is actually an integer). As these are now handled in the backend and mapped to GenericDataType
, that's what we're now going to be using for the most part.
* feat: add generic type to column payload * feat: add generic type to column payload * xit flaky test
* feat: add generic type to column payload * feat: add generic type to column payload * xit flaky test
* feat: add generic type to column payload * feat: add generic type to column payload * xit flaky test
SUMMARY
Adds the generic datatype to the dataset column payload to avoid having to parse the raw datatypes in the frontend. Also bump
superset-ui
to bring in updates tosuperset-ui/core
that are required to make this function properly: apache-superset/superset-ui#1102BEFORE
Currently the numeric datatypes on the FCC 2018 example dataset show up as unidentified due to the frontend not being able to detect the generic datatype of the raw datatype from the database ("DOUBLE PRECISION"):
![image](https://user-images.githubusercontent.com/33317356/117661443-0d8e0c80-b1a7-11eb-9b6a-bed2b92d225c.png)
AFTER
Now numeric types are being picked up automatically. In the future only datatypes that the db engine spec doesn't identify are shown as question marks:
![image](https://user-images.githubusercontent.com/33317356/117667995-2d74fe80-b1ae-11eb-8d6a-5b2dbe9128ad.png)
TEST PLAN
Added datatype tests for Postgres
ADDITIONAL INFORMATION