SEPURI-SAI-KRISHNA commented on PR #43225:
URL: https://github.com/apache/superset/pull/43225#issuecomment-5351302239
Both questions were right, and they were right in opposite directions.
thanks,
this needed the second look. Fixed and pushed.
**`groupby` must not carry a lower bound.** Confirmed exactly as you
described,
and it is stronger than a hypothetical client: `aggregateOperator` hardcodes
it.
`superset-ui-chart-controls/src/operators/aggregateOperator.ts:52`
```ts
return {
operation: 'aggregate',
options: {
groupby: [],
aggregates,
},
};
```
That fires for every aggregation that is not `LAST_VALUE` or `raw`. And
`aggregate()` reads it as a deliberate branch rather than tolerating it:
```python
if groupby:
df_groupby = df.groupby(by=groupby)
else:
df_groupby = df.groupby(lambda _: True)
```
So `minItems: 1` would have published Superset's own request as invalid. The
validator is gone. `required=True` stays, since the parameter has no default.
**`rolling.columns` should be required.** Agreed. `rolling()` takes it
positionally with no default, exactly as it takes `rolling_type`. which this
same schema already marked required. The two were inconsistent with each
other,
not just with the operation.
**`pivot.index` keeps its bound**, since `pivot()` does enforce it:
```python
if not index:
raise InvalidPostProcessingError(
_("Pivot operation requires at least one index")
)
```
Verified against the generated spec rather than the field declarations:
| field | required | minItems |
|---|---|---|
| `Aggregate.groupby` | true | *(none)* |
| `Rolling.columns` | true | n/a (object) |
| `Pivot.index` | true | 1 |
The test that asserted `groupby: []` is rejected encoded the bug, so it is
now
`test_aggregate_accepts_an_empty_groupby` asserting the opposite. Making
`columns` required also surfaced
`test_chart_data_rolling_options_schema_window_range`
on master, which built its payload from `{"rolling_type": "mean"}` alone; its
base now includes `columns`.
@aminghadersohi your correction is right and I have confirmed it. the only
references to these three schemas anywhere in the tree are the OpenAPI
component registration list in `schemas.py` and this one test file. Nothing
calls `.load()` on them in production, so this changes the emitted spec and
nothing else. I will reword the PR description, which implies lost runtime
validation. Your point about
`test_pivot_and_aggregate_reject_an_empty_column_list`
being weaker than it reads is well taken and is part of why the `groupby`
half
of it was wrong. it exercised a path no caller reaches, so nothing
contradicted
it. Noted separately on `docs/static/resources/openapi.json`: this PR does
not
regenerate it, so that snapshot still drifts.
Verified: 219 passed in `tests/unit_tests/charts`, ruff 0.9.7 clean, mypy
1.15.0
clean, pylint import-outside-toplevel down from 6 to 0 (the new tests'
imports
are hoisted to module scope, matching the rest of the file).
--
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]
