Skip to content
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(db_engine_specs): mysql longtext type should not be numeric #10661

Merged
merged 1 commit into from
Aug 24, 2020

Conversation

villebro
Copy link
Member

SUMMARY

Currently the MySQL type LONGTEXT is flagged as is_numeric due to the .*LONG.* pattern for numeric types. This makes the generic matching rule more specific for LONG types, and adds BIT as a default numeric type, as it is commonly a numeric datatype.

TEST PLAN

CI + new tests. We should add similar tests to all officially supported databaases to ensure column types are flagged correctly.

ADDITIONAL INFORMATION

FYI: @abhishekkumaresan

@codecov-commenter
Copy link

Codecov Report

Merging #10661 into master will decrease coverage by 4.16%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10661      +/-   ##
==========================================
- Coverage   64.32%   60.16%   -4.17%     
==========================================
  Files         784      784              
  Lines       36952    36951       -1     
  Branches     3529     3529              
==========================================
- Hits        23769    22231    -1538     
- Misses      13074    14533    +1459     
- Partials      109      187      +78     
Flag Coverage Δ
#cypress ?
#javascript 60.83% <ø> (ø)
#python 59.76% <ø> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset/db_engine_specs/base.py 87.64% <ø> (ø)
superset-frontend/src/SqlLab/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/dashboard/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/index.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/dashboard/index.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/setup/setupColors.js 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/chart/ChartContainer.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/setup/setupFormatters.js 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/reducers/index.js 0.00% <0.00%> (-100.00%) ⬇️
... and 144 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f44d3e...b6e5a79. Read the comment docs.

@abhishekkumaresan
Copy link

@villebro Thanks for the fast fix, If possible please give suggestions on the following,

For my use case, I have multiple dashboards from different databases but the table name will be the same across databases, In order to make this possible I changed the unique constraint on table_name to unique (database_id,table_name), for now, I don't face any issues because of this, but is there any better way to do this

I have to create a user, role, and database programmatically in superset I have implemented this using #3083 (comment)
but /users/api/create does not work for me so I am using the /users/add , /roles/add, databaseview/add for User, Role and Database respectively,

I tried to identify the right rest endpoints but could not

I use superset version 0.36.0 from pip in windows 10
Thank you

@villebro
Copy link
Member Author

@abhishekkumaresan I agree, the unique table name constraint is problematic, and would optimally be replaced with (database_id, schema and table_name), but there are some database compatibility issues associated with it that I can't fully recall right now. I believe there are a few issues or PRs where there has been extensive dialogue on this topic that you can chime in on. I personally feel this topic should be revisited, as I feel it is a major hinderance for many users, so if you can't find those discussions feel free to add a new issue to get this conversation started again (I'll weigh in with my opinions, too).

@dpgaspar is probably the best person to help with programmatic creation of users.

@dpgaspar
Copy link
Member

dpgaspar commented Aug 24, 2020

@abhishekkumaresan, currently there is no "official" / "right" way to create users or roles programatically and remote ( via a REST API ). A database creation API endpoint is just around the corner on the roadmap so stay tuned.

@abhishekkumaresan
Copy link

@villebro Thank you, I will search for the PRs related to the table_name
@dpgaspar Thank you, will wait for the changes

@villebro villebro merged commit 0177c2f into apache:master Aug 24, 2020
@villebro villebro deleted the villebro/longtext branch August 24, 2020 18:24
@villebro villebro added the v0.38 label Sep 10, 2020
villebro added a commit to preset-io/superset that referenced this pull request Sep 11, 2020
Ofeknielsen pushed a commit to ofekisr/incubator-superset that referenced this pull request Oct 5, 2020
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.38.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M v0.38 🚢 0.38.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MySQL LONGTEXT type matches with LONG Pattern in is_db_column_type_match making is_numeric True
5 participants