SEPURI-SAI-KRISHNA commented on issue #43356:
URL: https://github.com/apache/superset/issues/43356#issuecomment-5351982395
Thanks @dosu, the mechanism is right, and I reproduced it end to end on a
real
app boot rather than by reading the code. Two corrections, one to my own
issue.
**Correcting my reproduction steps.** The `PT10M` example in the issue body
is
wrong: `PT10M` is `TimeGrain.TEN_MINUTES`, already a built-in key in
`PROPHET_TIME_GRAIN_MAP`, so that grain forecasts fine and the bug does not
reproduce. It only reproduces for an addon key that is *not* also a built-in.
Corrected in the description. With `TIME_GRAIN_ADDONS = {"PT7M": "7
minute"}`:
```
PT7M is a built-in mapped grain? False
schema.load('PT7M') : ACCEPTED
prophet('PT7M') : InvalidPostProcessingError -> Unsupported time grain:
PT7M
```
Worth noting the choices are frozen at import: `get_time_grain_choices()` is
called at class-definition time in the `OneOf(...)` argument, not per
request. I
checked that addons still land in them under a normal `create_app()` boot,
since
the schema module is imported inside an app context with config already
loaded,
so the advertised set really does include addon keys.
**One caveat on Option 1.** `get_time_grain_choices()` has two callers, not
one:
- `schemas.py:720`, Prophet's `time_grain`
- `schemas.py:1085`, `time_grain_sqla` on `ChartDataQueryObjectSchema`
Addon grains are legitimately supported at the second one, serving custom SQL
time grains is the whole point of `TIME_GRAIN_ADDONS`. So narrowing that
shared
function would break the feature for every chart in order to fix Prophet.
Option
1 needs a separate, Prophet-only choices source rather than an edit to the
shared helper. `TimeGrain` is a `StrEnum`, so the map's keys can be used as
choices directly:
```python
validate=validate.OneOf(choices=list(PROPHET_TIME_GRAIN_MAP)),
```
That also makes the schema self-maintaining: adding a grain to the map
advertises
it, and nothing else has to be kept in sync.
One consequence to flag rather than hide: after #43205 the map holds 21 keys
against 19 built-ins, the extras being `PT0.5H` (`HALF_HOUR`) and `P0.25Y`
(`QUARTER_YEAR`), alternate ISO-8601 spellings that some engine specs expose.
Deriving Prophet's choices from the map therefore advertises a superset of
`time_grain_sqla`'s list. That is correct rather than a defect, since
`prophet()`
does resolve both, but it is a visible difference between the two fields and
should be a deliberate choice, not a surprise.
**On the built-in gap.** Worth separating from the addon case: on master
today
three *built-in* grains are advertised and unmapped, `PT5S`, `PT30S` and
`PT6H`, so the contract gap exists with no addons configured at all. #43205
closes exactly that: I verified every one of the 19 built-ins resolves on
that
branch, with nothing left over. This issue is only about the addon case that
remains afterwards.
I lean to Option 1 as well. Option 2 is the more complete answer but the
ISO-8601
to pandas mapping is not total, `P0.25Y` is the obvious example, so it would
need a fallback to the same "unsupported" error for the cases it cannot
convert,
which is most of the value of Option 1 with considerably more surface area.
--
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]
