trakshan-mishra opened a new pull request, #44773:
URL: https://github.com/apache/superset/pull/44773
### SUMMARY
Follow-up to #44460 for #42926.
#44460 recovers a legacy chart's time column by testing `granularity`,
`granularity_sqla` and `main_dttm_col` against `temporal_columns`, a set of
column names. `granularity_sqla` is not always a string. A saved `form_data`
can carry an adhoc column there, and the viz migrations already handle that
shape (`isinstance(granularity_sqla, dict)` in
`superset/migrations/shared/migrate_viz/base.py`). For such a chart the
membership test raises `TypeError: unhashable type: 'dict'`, so the stored
query fails instead of resolving a column.
This matches an adhoc candidate by its `sqlExpression`, the same way
`_apply_granularity` already unwraps an adhoc `x_axis` further down, and skips
any candidate that is still not a string:
- An adhoc column over a plain temporal column (`{"sqlExpression": "ds",
...}`) resolves to that column.
- An adhoc expression that is not a dataset column (`DATE_TRUNC('day', ds)`)
falls through to `main_dttm_col`, as a stale column name already does.
String values behave exactly as before.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable. Backend only, no UI change.
### TESTING INSTRUCTIONS
```
pytest tests/unit_tests/common/test_query_context_factory.py -q
```
`test_apply_granularity_matches_adhoc_legacy_granularity_sqla` covers both
cases above. On current master both raise `TypeError: unhashable type: 'dict'`
at `query_context_factory.py:306`. With this change the whole file passes (59
tests).
### ADDITIONAL INFORMATION
- [x] Has associated issue: #42926 (follow-up to #44460)
- [ ] 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
I had the same guard in #44526, which I'm closing now that #44460 is merged.
cc @rusackas and @shoemoney, since you reviewed and wrote #44460.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
