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

Change the remaning columns that has text type in mysql,mariadb to longtext #26223

Merged
merged 5 commits into from
Nov 8, 2022

Conversation

qnkhuat
Copy link
Contributor

@qnkhuat qnkhuat commented Nov 3, 2022

Fixes #26212 and is a follow up of #18749

Sanity check

SELECT table_schema, table_name, column_name, data_type
FROM INFORMATION_SCHEMA.COLUMNS
WHERE (data_type='text' OR data_type='blob') AND table_schema='metabase';

-- before
+--------------+--------------------------------------+-------------+-----------+
| TABLE_SCHEMA | TABLE_NAME                           | COLUMN_NAME | DATA_TYPE |
+--------------+--------------------------------------+-------------+-----------+
| metabase     | collection_permission_graph_revision | after       | text      |
| metabase     | collection_permission_graph_revision | before      | text      |
| metabase     | collection_permission_graph_revision | remark      | text      |
| metabase     | permissions_revision                 | after       | text      |
| metabase     | permissions_revision                 | before      | text      |
| metabase     | permissions_revision                 | remark      | text      |
| metabase     | QRTZ_BLOB_TRIGGERS                   | BLOB_DATA   | blob      |
| metabase     | QRTZ_CALENDARS                       | CALENDAR    | blob      |
| metabase     | QRTZ_JOB_DETAILS                     | JOB_DATA    | blob      |
| metabase     | QRTZ_TRIGGERS                        | JOB_DATA    | blob      |
| metabase     | secret                               | value       | blob      |
+--------------+--------------------------------------+-------------+-----------+

-- after
+--------------+--------------------+-------------+-----------+
| TABLE_SCHEMA | TABLE_NAME         | COLUMN_NAME | DATA_TYPE |
+--------------+--------------------+-------------+-----------+
| metabase     | QRTZ_BLOB_TRIGGERS | BLOB_DATA   | blob      |
| metabase     | QRTZ_CALENDARS     | CALENDAR    | blob      |
| metabase     | QRTZ_JOB_DETAILS   | JOB_DATA    | blob      |
| metabase     | QRTZ_TRIGGERS      | JOB_DATA    | blob      |
+--------------+--------------------+-------------+-----------+

@qnkhuat qnkhuat added the backport Automatically create PR on current release branch on merge label Nov 3, 2022
@codecov
Copy link

codecov bot commented Nov 3, 2022

Codecov Report

Base: 64.28% // Head: 64.28% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (def7695) compared to base (cc589cf).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #26223   +/-   ##
=======================================
  Coverage   64.28%   64.28%           
=======================================
  Files        3148     3148           
  Lines       92202    92202           
  Branches    11699    11699           
=======================================
+ Hits        59269    59270    +1     
+ Misses      28243    28242    -1     
  Partials     4690     4690           
Flag Coverage Δ
back-end 85.56% <ø> (+<0.01%) ⬆️
front-end 45.13% <ø> (ø)

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

Impacted Files Coverage Δ
...etabase_enterprise/serialization/v2/utils/yaml.clj 100.00% <0.00%> (+2.12%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@qnkhuat qnkhuat requested review from a team and flamber November 3, 2022 11:36
tbl-nm
col-nm
exp-type
(get tbl-cols col-nm))))))))))))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need this test? It is relatively costly to execute, and we don't run tests like this for all migrations. I feel the same way about convert-text-to-longtext-migration-test actually.

I can see the need to test more complicated migrations that involve SQL like v45.00-049 but in this case I think your sense check in the PR description is sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point, I'll remove this test.

@calherries calherries requested a review from a team November 7, 2022 14:17
Copy link
Contributor

@calherries calherries left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, approving even if I question whether the test can be deleted.

@qnkhuat qnkhuat enabled auto-merge (squash) November 8, 2022 09:43
@qnkhuat qnkhuat merged commit d3283c8 into master Nov 8, 2022
@qnkhuat qnkhuat deleted the ngoc-fix-26212 branch November 8, 2022 11:30
github-actions bot pushed a commit that referenced this pull request Nov 8, 2022
…ngtext (#26223)

* Change the remaning columns that has text type in mysql,mariadb to
longtext
metabase-bot bot added a commit that referenced this pull request Nov 8, 2022
…ngtext (#26223) (#26287)

* Change the remaning columns that has text type in mysql,mariadb to
longtext

Co-authored-by: Ngoc Khuat <qn.khuat@gmail.com>
qnkhuat added a commit that referenced this pull request Nov 9, 2022
…ngtext (#26223)

* Change the remaning columns that has text type in mysql,mariadb to
longtext
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport Automatically create PR on current release branch on merge
Projects
None yet
2 participants