You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/04/21 14:06:44 UTC
[GitHub] [superset] villebro opened a new pull request, #19805: fix(key_value): use longblob on mysql
villebro opened a new pull request, #19805:
URL: https://github.com/apache/superset/pull/19805
### SUMMARY
On Postgres `LongBinary` equates to `BYTEA` on Postgres which can store 1 Gb values. However, it turns out `LongBinary` creates `BLOB` on MySQL, which is limited to 64 kb. To make sure the `value` column on the `key_value` table is as wide on MySQL as it is on Postgres, we use [`with_variant`](https://docs.sqlalchemy.org/en/14/core/type_api.html#sqlalchemy.types.TypeEngine.with_variant) to use the `LONGBLOB` type for the `mysql` dialect, which can hold 4 Gb values.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #19805: fix(key_value): use longblob on mysql
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #19805:
URL: https://github.com/apache/superset/pull/19805#issuecomment-1105301590
# [Codecov](https://codecov.io/gh/apache/superset/pull/19805?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#19805](https://codecov.io/gh/apache/superset/pull/19805?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6239e2f) into [master](https://codecov.io/gh/apache/superset/commit/3db4a1cb8016dae71b5bfca09ef723c042dff671?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3db4a1c) will **decrease** coverage by `0.32%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #19805 +/- ##
==========================================
- Coverage 66.54% 66.22% -0.33%
==========================================
Files 1692 1692
Lines 64807 64807
Branches 6661 6661
==========================================
- Hits 43129 42920 -209
- Misses 19978 20187 +209
Partials 1700 1700
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `?` | |
| postgres | `?` | |
| presto | `?` | |
| python | `81.71% <ø> (-0.66%)` | :arrow_down: |
| sqlite | `81.71% <ø> (ø)` | |
| unit | `47.95% <ø> (ø)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/19805?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/19805/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.19%)` | :arrow_down: |
| [superset/sql\_validators/postgres.py](https://codecov.io/gh/apache/superset/pull/19805/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsX3ZhbGlkYXRvcnMvcG9zdGdyZXMucHk=) | `50.00% <0.00%> (-50.00%)` | :arrow_down: |
| [superset/databases/commands/create.py](https://codecov.io/gh/apache/superset/pull/19805/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2NvbW1hbmRzL2NyZWF0ZS5weQ==) | `64.70% <0.00%> (-27.46%)` | :arrow_down: |
| [superset/views/database/mixins.py](https://codecov.io/gh/apache/superset/pull/19805/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `60.34% <0.00%> (-20.69%)` | :arrow_down: |
| [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/superset/pull/19805/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `70.11% <0.00%> (-15.71%)` | :arrow_down: |
| [superset/databases/commands/update.py](https://codecov.io/gh/apache/superset/pull/19805/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2NvbW1hbmRzL3VwZGF0ZS5weQ==) | `85.71% <0.00%> (-8.17%)` | :arrow_down: |
| [superset/common/utils/dataframe\_utils.py](https://codecov.io/gh/apache/superset/pull/19805/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29tbW9uL3V0aWxzL2RhdGFmcmFtZV91dGlscy5weQ==) | `85.71% <0.00%> (-7.15%)` | :arrow_down: |
| [superset/databases/api.py](https://codecov.io/gh/apache/superset/pull/19805/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2FwaS5weQ==) | `88.02% <0.00%> (-5.99%)` | :arrow_down: |
| [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/superset/pull/19805/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `83.36% <0.00%> (-5.34%)` | :arrow_down: |
| [superset/databases/schemas.py](https://codecov.io/gh/apache/superset/pull/19805/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YWJhc2VzL3NjaGVtYXMucHk=) | `94.42% <0.00%> (-4.09%)` | :arrow_down: |
| ... and [15 more](https://codecov.io/gh/apache/superset/pull/19805/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/19805?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/19805?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [3db4a1c...6239e2f](https://codecov.io/gh/apache/superset/pull/19805?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on pull request #19805: fix(key_value): use longblob on mysql
Posted by GitBox <gi...@apache.org>.
ktmud commented on PR #19805:
URL: https://github.com/apache/superset/pull/19805#issuecomment-1105416580
Should we add a migration for existing users?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] iercan commented on pull request #19805: fix(key_value): use longblob on mysql
Posted by GitBox <gi...@apache.org>.
iercan commented on PR #19805:
URL: https://github.com/apache/superset/pull/19805#issuecomment-1105283518
I've confirmed it works. Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro merged pull request #19805: fix(key_value): use longblob on mysql
Posted by GitBox <gi...@apache.org>.
villebro merged PR #19805:
URL: https://github.com/apache/superset/pull/19805
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on pull request #19805: fix(key_value): use longblob on mysql
Posted by GitBox <gi...@apache.org>.
ktmud commented on PR #19805:
URL: https://github.com/apache/superset/pull/19805#issuecomment-1105532304
Hmm... since the migration is not that complicated I guess it'd be okay to ask users to do it manually. For those who choose to deploy on `master`, such risks are implied.
That said, do you mind changing the solution to follow the same convention like [`MediumText`](https://github.com/apache/superset/blob/8865656e06e9d098d255b0da4bc3d3982b40f76d/superset/utils/core.py#L1457-L1458)? We'd need to update the type [here](https://github.com/apache/superset/blob/b7a0559aaf5ff4266baf5069b93379fbecfb4a00/superset/key_value/models.py#L31) as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on pull request #19805: fix(key_value): use longblob on mysql
Posted by GitBox <gi...@apache.org>.
ktmud commented on PR #19805:
URL: https://github.com/apache/superset/pull/19805#issuecomment-1105651614
It seems another solution to this is to [specify a length](https://docs.sqlalchemy.org/en/14/core/type_basics.html?highlight=largebinary#sqlalchemy.types.LargeBinary) for `LargeBinary`. MySQL will automatically choose whether to use MEDIUMBLOB or LOBGBLOB depending on the length specified.
Would that be preferred?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #19805: fix(key_value): use longblob on mysql
Posted by GitBox <gi...@apache.org>.
villebro commented on PR #19805:
URL: https://github.com/apache/superset/pull/19805#issuecomment-1106060099
> It seems another solution to this is to [specify a length](https://docs.sqlalchemy.org/en/14/core/type_basics.html?highlight=largebinary#sqlalchemy.types.LargeBinary) for `LargeBinary`. MySQL will automatically choose whether to use MEDIUMBLOB or LOBGBLOB depending on the length specified.
>
> Would that be preferred?
I think that's perfect 👍 I considered adding a `types.py` under `superset/migrations`, but I ended up settling for just specifying `length=2**31` (2 Gb).
I wrote the following test script:
```python
import sqlalchemy as sa
from sqlalchemy.schema import CreateTable
from sqlalchemy.types import LargeBinary
tbl = sa.Table('tbl', sa.MetaData(), sa.Column("value", sa.LargeBinary(length=2**31)))
mysql_engine = sa.create_engine('mysql://uid:pwd@localhost')
pg_engine = sa.create_engine('postgresql://uid:pwd@localhost')
print("-- MySQL:")
print(CreateTable(tbl).compile(pg_engine))
print("-- Postgres:")
print(CreateTable(tbl).compile(mysql_engine))
```
which results in this:
```
-- MySQL:
CREATE TABLE tbl (
value BYTEA
)
-- Postgres:
CREATE TABLE tbl (
value BLOB(2147483648)
)
```
And when the MySQL one is executed, it results in `LONGBLOB` (@zhaoyongjie was kind enough to test this as I didn't have a MySQL instance handy)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #19805: fix(key_value): use longblob on mysql
Posted by GitBox <gi...@apache.org>.
villebro commented on PR #19805:
URL: https://github.com/apache/superset/pull/19805#issuecomment-1105421952
> Should we add a migration for existing users?
@ktmud I was wondering the same, and I unfortunately don't have a good answer. @iercan already did a manual migration (see https://apache-superset.slack.com/archives/CH307T4JG/p1650463618742279?thread_ts=1650031133.456189&cid=CH307T4JG ), and I kinda feel like orgs that are running off of master might need a different process for these types of migration improvements. The problem is, if I make a new migration for this, it'll be hell cherrying it into all the various release branches. But I'm open to suggestions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org