You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "Always-prog (via GitHub)" <gi...@apache.org> on 2023/07/16 10:06:54 UTC
[GitHub] [superset] Always-prog opened a new pull request, #24710: fix(pvt2): migrations from legacy pivot table error when form_data have pieces of pvt2
Always-prog opened a new pull request, #24710:
URL: https://github.com/apache/superset/pull/24710
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
Hello!
Migration from legacy pivot table v2 sometimes returns an error due to pieces of Pivot Table V2 form_data in the legacy pivot. This PR fixes it. Pieces of form_data can occur when users switch from pvt2 tables to legacy pivot.
### TESTING INSTRUCTIONS
1. Create an save Pivot Table v2 visualization
2. Switch to the legacy pivot table and save
3. Try to migrate and see the error "Dublicate key".
Or just try to migrate form_data that is already generated by me and see the error.
```python
{'adhoc_filters': [], 'aggregateFunction': 'Sum', 'applied_time_extras': {}, 'colOrder': 'key_a_to_z', 'colTotals': True, 'columns': ['date_obr'], 'conditional_formatting': [], 'dashboards': [], 'datasource': '2858__table', 'date_format': 'smart_date', 'granularity_sqla': 'date_', 'groupby': ['Name_gr'], 'groupbyColumns': ['date_obr'], 'groupbyRows': ['id_group', 'TT'], 'label_colors': {}, 'metrics': ['count'], 'metricsLayout': 'COLUMNS', 'number_format': 'SMART_NUMBER', 'order_desc': True, 'pandas_aggfunc': 'Sum', 'pivot_margins': True, 'rowOrder': 'value_z_to_a', 'rowTotals': True, 'row_limit': 1000, 'series_limit_metric': ['count'], 'shared_label_colors': {}, 'slice_id': 16619, 'time_grain_sqla': 'P1D', 'time_range': 'Last week', 'timeseries_limit_metric': 'count', 'valueFormat': ',d', 'viz_type': 'pivot_table_v2', 'extra_filters': [], 'dataMask': {}, 'extraControls': {}}
```
### 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 #24710: fix(pvt2): migrations from legacy pivot table error when form_data have pieces of pvt2
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #24710:
URL: https://github.com/apache/superset/pull/24710#issuecomment-1637044395
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/24710?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#24710](https://app.codecov.io/gh/apache/superset/pull/24710?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (215c08f) into [master](https://app.codecov.io/gh/apache/superset/commit/0328dd270467e71260bfa85078beb7b38a87877b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (0328dd2) will **decrease** coverage by `10.81%`.
> The diff coverage is `66.66%`.
> :exclamation: Current head 215c08f differs from pull request most recent head ea947c6. Consider uploading reports for the commit ea947c6 to get more accurate results
```diff
@@ Coverage Diff @@
## master #24710 +/- ##
===========================================
- Coverage 68.97% 58.17% -10.81%
===========================================
Files 1901 1901
Lines 74008 74011 +3
Branches 8183 8183
===========================================
- Hits 51047 43053 -7994
- Misses 20840 28837 +7997
Partials 2121 2121
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `?` | |
| postgres | `?` | |
| presto | `54.01% <0.00%> (-0.01%)` | :arrow_down: |
| python | `60.85% <66.66%> (-22.60%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `54.83% <66.66%> (+<0.01%)` | :arrow_up: |
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=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/superset/pull/24710?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...perset/migrations/shared/migrate\_viz/processors.py](https://app.codecov.io/gh/apache/superset/pull/24710?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWlncmF0aW9ucy9zaGFyZWQvbWlncmF0ZV92aXovcHJvY2Vzc29ycy5weQ==) | `78.18% <66.66%> (-21.82%)` | :arrow_down: |
... and [292 files with indirect coverage changes](https://app.codecov.io/gh/apache/superset/pull/24710/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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] michael-s-molina merged pull request #24710: fix(pvt2): migrations from legacy pivot table error when form_data have pieces of pvt2
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina merged PR #24710:
URL: https://github.com/apache/superset/pull/24710
--
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] michael-s-molina commented on a diff in pull request #24710: fix(pvt2): migrations from legacy pivot table error when form_data have pieces of pvt2
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #24710:
URL: https://github.com/apache/superset/pull/24710#discussion_r1265601017
##########
superset/migrations/shared/migrate_viz/base.py:
##########
@@ -64,6 +64,12 @@ def _migrate(self) -> None:
if "viz_type" in self.data:
self.data["viz_type"] = self.target_viz_type
+ # Sometimes visualizations have same keys in the source form_data and rename_keys
+ # We need to remove them from data to allow the migration to work properly with rename_keys
+ for key in self.rename_keys.values():
Review Comment:
I'm worried we might have keys that belong to the new version but not a corresponding legacy key. In that case, removing the keys will make us lose information. Taking a Pivot Table as example, we might have `series_limit` (new) but no `row_limit` (legacy). For safety, could we only remove keys if its legacy counterpart also exists?
--
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] Always-prog commented on pull request #24710: fix(pvt2): migrations from legacy pivot table error when form_data have pieces of pvt2
Posted by "Always-prog (via GitHub)" <gi...@apache.org>.
Always-prog commented on PR #24710:
URL: https://github.com/apache/superset/pull/24710#issuecomment-1644359628
@michael-s-molina Got it, thank you. Fixed!
--
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] Always-prog commented on pull request #24710: fix(pvt2): migrations from legacy pivot table error when form_data have pieces of pvt2
Posted by "Always-prog (via GitHub)" <gi...@apache.org>.
Always-prog commented on PR #24710:
URL: https://github.com/apache/superset/pull/24710#issuecomment-1643406761
@michael-s-molina Hi! If everything is ok with my PR now, can we merge it?
--
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] Always-prog commented on pull request #24710: fix(pvt2): migrations from legacy pivot table error when form_data have pieces of pvt2
Posted by "Always-prog (via GitHub)" <gi...@apache.org>.
Always-prog commented on PR #24710:
URL: https://github.com/apache/superset/pull/24710#issuecomment-1644168405
@michael-s-molina There is no need to rebase or merge (I already tried). I created a branch on the latest version of the code.
Looks like CI broke accidentally, just needs a rerun.
--
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] Always-prog commented on pull request #24710: fix(pvt2): migrations from legacy pivot table error when form_data have pieces of pvt2
Posted by "Always-prog (via GitHub)" <gi...@apache.org>.
Always-prog commented on PR #24710:
URL: https://github.com/apache/superset/pull/24710#issuecomment-1638115305
> > Pieces of form_data can occur when users switch from pvt2 tables to legacy pivot.
>
> Thanks for the fix @Always-prog! If this behavior happens for Pivot Table, it may also happen for other chart types. Would you mind resolving this in a more global way? You can skip rename keys in `base.py`:
>
> ```
> def _migrate(self) -> None:
> ...
> rv_data = {}
> for key, value in self.data.items():
> // ADD SOMETHING HERE TO SKIP NEW VERSION KEYS
>
> if key in self.rename_keys and self.rename_keys[key] in rv_data:
> raise ValueError("Duplicate key in target viz")
> ```
Then, can we just instead of an error "Duplicate key in target viz" just skip that keys?
--
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] Always-prog commented on pull request #24710: fix(pvt2): migrations from legacy pivot table error when form_data have pieces of pvt2
Posted by "Always-prog (via GitHub)" <gi...@apache.org>.
Always-prog commented on PR #24710:
URL: https://github.com/apache/superset/pull/24710#issuecomment-1638320119
@michael-s-molina I moved removing rename keys from data to the base.py, but before `for key, value in self.data.items():`. Can you look at the code?
--
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] michael-s-molina commented on pull request #24710: fix(pvt2): migrations from legacy pivot table error when form_data have pieces of pvt2
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #24710:
URL: https://github.com/apache/superset/pull/24710#issuecomment-1637978990
> Pieces of form_data can occur when users switch from pvt2 tables to legacy pivot.
Thanks for the fix @Always-prog! If this behavior happens for Pivot Table, it may also happen for other chart types. Would you mind resolving this in a more global way? You can skip rename keys in `base.py`:
```
def _migrate(self) -> None:
...
rv_data = {}
for key, value in self.data.items():
// ADD SOMETHING HERE TO SKIP NEW VERSION KEYS
if key in self.rename_keys and self.rename_keys[key] in rv_data:
raise ValueError("Duplicate key in target viz")
```
--
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] michael-s-molina commented on a diff in pull request #24710: fix(pvt2): migrations from legacy pivot table error when form_data have pieces of pvt2
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #24710:
URL: https://github.com/apache/superset/pull/24710#discussion_r1265601017
##########
superset/migrations/shared/migrate_viz/base.py:
##########
@@ -64,6 +64,12 @@ def _migrate(self) -> None:
if "viz_type" in self.data:
self.data["viz_type"] = self.target_viz_type
+ # Sometimes visualizations have same keys in the source form_data and rename_keys
+ # We need to remove them from data to allow the migration to work properly with rename_keys
+ for key in self.rename_keys.values():
Review Comment:
I'm worried we might have keys that belong to the new version but not a corresponding legacy key. In that case, removing the keys will make us lose information. Taking a Pivot Table as example, we might have `series_limit` (new) but no `row_limit` (legacy). For safety, could only remove keys if its legacy counterpart also exists?
--
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] michael-s-molina commented on pull request #24710: fix(pvt2): migrations from legacy pivot table error when form_data have pieces of pvt2
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #24710:
URL: https://github.com/apache/superset/pull/24710#issuecomment-1644191423
> @michael-s-molina There is no need to rebase or merge (I already tried). I created a branch on the latest version of the code. Looks like CI broke accidentally, just needs a rerun.
You need to have https://github.com/apache/superset/pull/24731 in your commit history to fix the CI error. That's why a rebase is needed.
--
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] michael-s-molina commented on pull request #24710: fix(pvt2): migrations from legacy pivot table error when form_data have pieces of pvt2
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #24710:
URL: https://github.com/apache/superset/pull/24710#issuecomment-1643999901
> @michael-s-molina Hi! If everything is ok with my PR now, can we merge it?
Hey. Could you rebase it to fix the Docker issue? I can't merge it with that CI check failing.
--
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] michael-s-molina commented on pull request #24710: fix(pvt2): migrations from legacy pivot table error when form_data have pieces of pvt2
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #24710:
URL: https://github.com/apache/superset/pull/24710#issuecomment-1638125063
> Then, can we just instead of an error "Duplicate key in target viz" just skip keys which is in rename dictionary?
No because that check is to prevent that different pieces of information are mapped to the same key which is a development error.
--
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] Always-prog commented on a diff in pull request #24710: fix(pvt2): migrations from legacy pivot table error when form_data have pieces of pvt2
Posted by "Always-prog (via GitHub)" <gi...@apache.org>.
Always-prog commented on code in PR #24710:
URL: https://github.com/apache/superset/pull/24710#discussion_r1266303109
##########
superset/migrations/shared/migrate_viz/base.py:
##########
@@ -64,6 +64,12 @@ def _migrate(self) -> None:
if "viz_type" in self.data:
self.data["viz_type"] = self.target_viz_type
+ # Sometimes visualizations have same keys in the source form_data and rename_keys
+ # We need to remove them from data to allow the migration to work properly with rename_keys
+ for key in self.rename_keys.values():
Review Comment:
Got it! I added removing target key only if source and target rename keys exists in form_data
--
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