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 2021/11/30 20:07:07 UTC
[GitHub] [superset] AAfghahi opened a new pull request #17600: fix: Ch31968query context
AAfghahi opened a new pull request #17600:
URL: https://github.com/apache/superset/pull/17600
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### 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] AAfghahi commented on a change in pull request #17600: fix: Ch31968query context
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #17600:
URL: https://github.com/apache/superset/pull/17600#discussion_r759750341
##########
File path: superset/connectors/base/models.py
##########
@@ -319,8 +335,14 @@ def data_for_slices( # pylint: disable=too-many-locals
if "column" in filter_config
)
+ # for legacy dashboard imports which have the wrong query_context in them
+ try:
Review comment:
This process was emulating what was set out here:
https://github.com/apache/superset/pull/17461
--
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] sadpandajoe commented on pull request #17600: fix: Ch31968query context
Posted by GitBox <gi...@apache.org>.
sadpandajoe commented on pull request #17600:
URL: https://github.com/apache/superset/pull/17600#issuecomment-983178559
🏷️ 2021.46
--
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] betodealmeida commented on a change in pull request #17600: fix: Ch31968query context
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #17600:
URL: https://github.com/apache/superset/pull/17600#discussion_r759640507
##########
File path: superset/charts/commands/importers/v1/__init__.py
##########
@@ -96,10 +96,5 @@ def _import(
}
)
config["params"].update({"datasource": dataset.uid})
- if config["query_context"]:
Review comment:
We need to delete query_context if it exists:
```python
# fix old exports before PR
if config["query_context"]:
config["query_context"] = None
```
--
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] AAfghahi commented on a change in pull request #17600: fix: Ch31968query context
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #17600:
URL: https://github.com/apache/superset/pull/17600#discussion_r759750341
##########
File path: superset/connectors/base/models.py
##########
@@ -319,8 +335,14 @@ def data_for_slices( # pylint: disable=too-many-locals
if "column" in filter_config
)
+ # for legacy dashboard imports which have the wrong query_context in them
+ try:
Review comment:
This process was emulating what was set out here:
https://github.com/apache/superset/pull/17461
--
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] betodealmeida commented on a change in pull request #17600: fix: Ch31968query context
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #17600:
URL: https://github.com/apache/superset/pull/17600#discussion_r759635563
##########
File path: superset/charts/commands/export.py
##########
@@ -63,6 +63,9 @@ def _export(model: Slice) -> Iterator[Tuple[str, str]]:
except json.decoder.JSONDecodeError:
logger.info("Unable to decode `params` field: %s", payload["params"])
+ if payload.get("query_context"):
Review comment:
```python
payload = {
key: value
for key, value in payload.items()
if key not in REMOVE_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] AAfghahi commented on a change in pull request #17600: fix: Ch31968query context
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #17600:
URL: https://github.com/apache/superset/pull/17600#discussion_r759701136
##########
File path: superset/charts/commands/importers/v1/__init__.py
##########
@@ -96,10 +96,5 @@ def _import(
}
)
config["params"].update({"datasource": dataset.uid})
- if config["query_context"]:
Review comment:
Would it be better if this was
```python
if config["query_context"]:
del config["query_context"]
```
--
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] eschutho commented on a change in pull request #17600: fix: Ch31968query context
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #17600:
URL: https://github.com/apache/superset/pull/17600#discussion_r759769713
##########
File path: superset/connectors/base/models.py
##########
@@ -319,8 +320,13 @@ def data_for_slices( # pylint: disable=too-many-locals
if "column" in filter_config
)
+ # for legacy dashboard imports which have the wrong query_context in them
+ try:
+ query_context = slc.get_query_context()
+ except DatasetNotFoundError:
+ query_context = None
Review comment:
@AAfghahi and I also talked about maybe writing a script/migration as a follow up that will remove any existing query_context fields from charts if they contain a dataset id. Either way, it seems that if query context is failing in any way here, that it would be just as fine not to use 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] eschutho merged pull request #17600: fix: Ch31968query context
Posted by GitBox <gi...@apache.org>.
eschutho merged pull request #17600:
URL: https://github.com/apache/superset/pull/17600
--
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] eschutho commented on a change in pull request #17600: fix: Ch31968query context
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #17600:
URL: https://github.com/apache/superset/pull/17600#discussion_r759769713
##########
File path: superset/connectors/base/models.py
##########
@@ -319,8 +320,13 @@ def data_for_slices( # pylint: disable=too-many-locals
if "column" in filter_config
)
+ # for legacy dashboard imports which have the wrong query_context in them
+ try:
+ query_context = slc.get_query_context()
+ except DatasetNotFoundError:
+ query_context = None
Review comment:
@AAfghahi and I also talked about maybe writing a script/migration as a follow up that will remove any existing query_context fields from charts if they contain an invalid dataset id. Either way, it seems that if query context is failing in any way here, that it would be just as fine not to use 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] eschutho commented on a change in pull request #17600: fix: Ch31968query context
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #17600:
URL: https://github.com/apache/superset/pull/17600#discussion_r759769993
##########
File path: tests/integration_tests/charts/commands_tests.py
##########
@@ -191,34 +190,6 @@ def test_import_v1_chart(self, mock_g):
)
assert dataset.table_name == "imported_dataset"
assert chart.table == dataset
- assert json.loads(chart.query_context) == {
Review comment:
also @AAfghahi is going to fast-follow with a test for these changes.
--
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