You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "michael-s-molina (via GitHub)" <gi...@apache.org> on 2023/02/07 16:05:40 UTC
[GitHub] [superset] michael-s-molina opened a new pull request, #23021: fix: Time Column on Generic X-axis
michael-s-molina opened a new pull request, #23021:
URL: https://github.com/apache/superset/pull/23021
### SUMMARY
TODO
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TODO
### TESTING INSTRUCTIONS
TODO
### ADDITIONAL INFORMATION
- [ ] 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] michael-s-molina commented on a diff in pull request #23021: fix: Time Column on Generic X-axis
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #23021:
URL: https://github.com/apache/superset/pull/23021#discussion_r1310337797
##########
superset/common/query_context_factory.py:
##########
@@ -99,3 +103,89 @@ def _convert_to_model(self, datasource: DatasourceDict) -> BaseDatasource:
def _get_slice(self, slice_id: Any) -> Optional[Slice]:
return ChartDAO.find_by_id(slice_id)
+
+ def _process_query_object(
+ self,
+ datasource: BaseDatasource,
+ form_data: Optional[Dict[str, Any]],
+ query_object: QueryObject,
+ ) -> QueryObject:
+ self._apply_granularity(query_object, form_data, datasource)
+ self._apply_filters(query_object)
+ return query_object
+
+ def _apply_granularity(
+ self,
+ query_object: QueryObject,
+ form_data: Optional[Dict[str, Any]],
+ datasource: BaseDatasource,
+ ) -> None:
+ temporal_columns = {
+ column.column_name
+ for column in datasource.columns
+ if (column["is_dttm"] if isinstance(column, dict) else column.is_dttm)
+ }
+ granularity = query_object.granularity
+ x_axis = form_data and form_data.get("x_axis")
+
+ if granularity:
+ filter_to_remove = None
+ if x_axis and x_axis in temporal_columns:
Review Comment:
Hi @zephyring. Can you open a draft PR with your changes (`x_axis["sqlExpression"]`)? Can you also add a comment to your PR explaining what do you mean by
> it doesn't seem to have the same effect
--
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 #23021: fix: Time Column on Generic X-axis
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #23021:
URL: https://github.com/apache/superset/pull/23021#issuecomment-1421473045
# [Codecov](https://codecov.io/gh/apache/superset/pull/23021?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 [#23021](https://codecov.io/gh/apache/superset/pull/23021?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b4a38c9) into [master](https://codecov.io/gh/apache/superset/commit/79114bcd29b0ce1685828ff9a60791d13a7459c9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (79114bc) will **decrease** coverage by `11.03%`.
> The diff coverage is `56.75%`.
> :exclamation: Current head b4a38c9 differs from pull request most recent head 070ffc5. Consider uploading reports for the commit 070ffc5 to get more accurate results
```diff
@@ Coverage Diff @@
## master #23021 +/- ##
===========================================
- Coverage 67.42% 56.40% -11.03%
===========================================
Files 1878 1878
Lines 72116 72170 +54
Branches 7864 7864
===========================================
- Hits 48625 40706 -7919
- Misses 21472 29445 +7973
Partials 2019 2019
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `52.73% <58.33%> (-0.01%)` | :arrow_down: |
| mysql | `?` | |
| postgres | `?` | |
| presto | `52.63% <58.33%> (-0.01%)` | :arrow_down: |
| python | `59.19% <58.33%> (-22.92%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `52.47% <13.88%> (+0.02%)` | :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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/23021?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...es/superset-ui-core/src/query/buildQueryContext.ts](https://codecov.io/gh/apache/superset/pull/23021?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvcXVlcnkvYnVpbGRRdWVyeUNvbnRleHQudHM=) | `100.00% <ø> (ø)` | |
| [...rset-frontend/src/explore/components/SaveModal.tsx](https://codecov.io/gh/apache/superset/pull/23021?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9TYXZlTW9kYWwudHN4) | `39.04% <0.00%> (ø)` | |
| [superset/common/query\_context\_factory.py](https://codecov.io/gh/apache/superset/pull/23021?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29tbW9uL3F1ZXJ5X2NvbnRleHRfZmFjdG9yeS5weQ==) | `74.64% <58.33%> (-22.50%)` | :arrow_down: |
| [superset/utils/dashboard\_import\_export.py](https://codecov.io/gh/apache/superset/pull/23021?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvZGFzaGJvYXJkX2ltcG9ydF9leHBvcnQucHk=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset/tags/core.py](https://codecov.io/gh/apache/superset/pull/23021?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdGFncy9jb3JlLnB5) | `4.54% <0.00%> (-95.46%)` | :arrow_down: |
| [superset/key\_value/commands/update.py](https://codecov.io/gh/apache/superset/pull/23021?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL3VwZGF0ZS5weQ==) | `0.00% <0.00%> (-90.91%)` | :arrow_down: |
| [superset/key\_value/commands/delete.py](https://codecov.io/gh/apache/superset/pull/23021?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL2RlbGV0ZS5weQ==) | `0.00% <0.00%> (-87.88%)` | :arrow_down: |
| [superset/key\_value/commands/delete\_expired.py](https://codecov.io/gh/apache/superset/pull/23021?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL2RlbGV0ZV9leHBpcmVkLnB5) | `0.00% <0.00%> (-84.00%)` | :arrow_down: |
| [superset/dashboards/commands/importers/v0.py](https://codecov.io/gh/apache/superset/pull/23021?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9jb21tYW5kcy9pbXBvcnRlcnMvdjAucHk=) | `15.62% <0.00%> (-76.25%)` | :arrow_down: |
| [superset/datasets/commands/create.py](https://codecov.io/gh/apache/superset/pull/23021?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvY3JlYXRlLnB5) | `30.61% <0.00%> (-69.39%)` | :arrow_down: |
| ... and [290 more](https://codecov.io/gh/apache/superset/pull/23021?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
: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=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] zephyring commented on a diff in pull request #23021: fix: Time Column on Generic X-axis
Posted by "zephyring (via GitHub)" <gi...@apache.org>.
zephyring commented on code in PR #23021:
URL: https://github.com/apache/superset/pull/23021#discussion_r1309449085
##########
superset/common/query_context_factory.py:
##########
@@ -99,3 +103,89 @@ def _convert_to_model(self, datasource: DatasourceDict) -> BaseDatasource:
def _get_slice(self, slice_id: Any) -> Optional[Slice]:
return ChartDAO.find_by_id(slice_id)
+
+ def _process_query_object(
+ self,
+ datasource: BaseDatasource,
+ form_data: Optional[Dict[str, Any]],
+ query_object: QueryObject,
+ ) -> QueryObject:
+ self._apply_granularity(query_object, form_data, datasource)
+ self._apply_filters(query_object)
+ return query_object
+
+ def _apply_granularity(
+ self,
+ query_object: QueryObject,
+ form_data: Optional[Dict[str, Any]],
+ datasource: BaseDatasource,
+ ) -> None:
+ temporal_columns = {
+ column.column_name
+ for column in datasource.columns
+ if (column["is_dttm"] if isinstance(column, dict) else column.is_dttm)
+ }
+ granularity = query_object.granularity
+ x_axis = form_data and form_data.get("x_axis")
+
+ if granularity:
+ filter_to_remove = None
+ if x_axis and x_axis in temporal_columns:
Review Comment:
hey @michael-s-molina we recently found a regression where the `x_axis` could be a `dict` while `temporal_columns` is a set of string and it throws.
Steps to reproduce:
```
1. Create a Bar Chart using the Vehicle Sales dataset.
2. Use Custom SQL for the X-Axis field and type in order_date.
3. Use SUM(price_each) for the Metrics field.
4. Use the SQL Lab to duplicate the Vehicle Sales dataset.
5. Perform a dataset swap to the duplicated Vehicle Sales dataset.
6. Update and Save the chart.
```
I am trying to use `x_axis["sqlExpression"]` to get the proper str to compare down the road and it doesn't seem to have the same effect.
Would love your thoughts here 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] michael-s-molina merged pull request #23021: fix: Time Column on Generic X-axis
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina merged PR #23021:
URL: https://github.com/apache/superset/pull/23021
--
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 a diff in pull request #23021: fix: Time Column on Generic X-axis
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #23021:
URL: https://github.com/apache/superset/pull/23021#discussion_r1310484513
##########
superset/common/query_context_factory.py:
##########
@@ -99,3 +103,89 @@ def _convert_to_model(self, datasource: DatasourceDict) -> BaseDatasource:
def _get_slice(self, slice_id: Any) -> Optional[Slice]:
return ChartDAO.find_by_id(slice_id)
+
+ def _process_query_object(
+ self,
+ datasource: BaseDatasource,
+ form_data: Optional[Dict[str, Any]],
+ query_object: QueryObject,
+ ) -> QueryObject:
+ self._apply_granularity(query_object, form_data, datasource)
+ self._apply_filters(query_object)
+ return query_object
+
+ def _apply_granularity(
+ self,
+ query_object: QueryObject,
+ form_data: Optional[Dict[str, Any]],
+ datasource: BaseDatasource,
+ ) -> None:
+ temporal_columns = {
+ column.column_name
+ for column in datasource.columns
+ if (column["is_dttm"] if isinstance(column, dict) else column.is_dttm)
+ }
+ granularity = query_object.granularity
+ x_axis = form_data and form_data.get("x_axis")
+
+ if granularity:
+ filter_to_remove = None
+ if x_axis and x_axis in temporal_columns:
Review Comment:
@michael-s-molina @zephyring It appears the issue here happens due to using an adhoc column as an x-axis. Previously I've also noticed that adhoc x-axes may not always work as expected, and this logic probably needs to be refined somewhat. Maybe we should have a Zoom session to work on a proper fix for this?
--
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
Re: [PR] fix: Time Column on Generic X-axis [superset]
Posted by "emirkmo (via GitHub)" <gi...@apache.org>.
emirkmo commented on code in PR #23021:
URL: https://github.com/apache/superset/pull/23021#discussion_r1461918740
##########
superset/common/query_context_factory.py:
##########
@@ -99,3 +103,89 @@ def _convert_to_model(self, datasource: DatasourceDict) -> BaseDatasource:
def _get_slice(self, slice_id: Any) -> Optional[Slice]:
return ChartDAO.find_by_id(slice_id)
+
+ def _process_query_object(
+ self,
+ datasource: BaseDatasource,
+ form_data: Optional[Dict[str, Any]],
+ query_object: QueryObject,
+ ) -> QueryObject:
+ self._apply_granularity(query_object, form_data, datasource)
+ self._apply_filters(query_object)
+ return query_object
+
+ def _apply_granularity(
+ self,
+ query_object: QueryObject,
+ form_data: Optional[Dict[str, Any]],
+ datasource: BaseDatasource,
+ ) -> None:
+ temporal_columns = {
+ column.column_name
+ for column in datasource.columns
+ if (column["is_dttm"] if isinstance(column, dict) else column.is_dttm)
+ }
+ granularity = query_object.granularity
+ x_axis = form_data and form_data.get("x_axis")
+
+ if granularity:
+ filter_to_remove = None
+ if x_axis and x_axis in temporal_columns:
Review Comment:
Is there an issue or PR linked to this?
--
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