villebro commented on code in PR #24176:
URL: https://github.com/apache/superset/pull/24176#discussion_r1203575002
##########
superset/common/query_context_processor.py:
##########
@@ -328,7 +325,25 @@ def processing_time_offsets( # pylint:
disable=too-many-locals,too-many-stateme
"when using a Time Comparison."
)
)
- for offset in time_offsets:
+
+ columns = df.columns
+ time_grain = query_object.extras["time_grain_sqla"]
+ use_aggregated_join_column = any(
+ grain in time_grain for grain in ("P1W", "P1M", "P3M", "P1Y")
+ )
Review Comment:
Could this be made more explicit and at the same time slightly easier to
read? I think it might improve readability if we list all the time grains that
are applicable, and then just check if `time_grain` is contained in that list.
Something like
```python
AGGREGATED_JOIN_GRAINS = {
"1969-12-28T00:00:00Z/P1W",
"1969-12-29T00:00:00Z/P1W",
"P1W/1970-01-03T00:00:00Z",
"P1W/1970-01-04T00:00:00Z",
"P1W",
"P1M",
"P3M",
"P1Y",
}
...
use_aggregated_join_column = time_grain in AGGREGATED_JOIN_GRAINS
```
##########
superset/common/query_context_processor.py:
##########
@@ -87,12 +87,10 @@ class QueryContextProcessor:
to retrieve the data payload for a given viz.
"""
+ AGGREGATED_JOIN_COLUMN = "$aggregated_join_column"
Review Comment:
In general we've assumed that double underscores mean Superset internal
columns. For consistency, do you think it could be possible to call this
"__aggregated_join_column"?
--
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]
