SEPURI-SAI-KRISHNA commented on PR #43225:
URL: https://github.com/apache/superset/pull/43225#issuecomment-5565568676
Thanks for the round-3 measurement, and for being explicit that the three
open threads are @sadpandajoe's to close rather than yours. Nothing for me to
action there.
On your non-blocking observation about
`ChartDataPivotOptionsSchema.aggregates`: I ran it, and I agree it should not
go into this PR. But it is worth recording that the defect is both wider and
more severe than the way it was flagged, because that changes how a follow-up
should be scoped.
**It is not one field.** I swept all twelve `ChartData*OptionsSchema`
classes, comparing each declared field against the operation's signature, and
asked which parameters have no default while the field is optional:
| Schema | Field(s) |
|---|---|
| `ChartDataAggregateOptionsSchema` | `aggregates` |
| `ChartDataBoxplotOptionsSchema` | `groupby`, `metrics` |
| `ChartDataPivotOptionsSchema` | `aggregates` |
Four fields, three schemas. Every other option schema is consistent with its
operation.
**The omitted-key case is a 500, not a 400.** `pivot()`'s `"Pivot operation
must include at least one aggregate"` guard never runs, because the call never
gets that far. `exec_post_processing` dispatches as `func(df,
**post_process.get("options", {}))` (`superset/common/query_object.py:668`)
with no `try`, so a missing key fails at binding time:
```
aggregate {"groupby": ["g"]} -> TypeError: aggregate() missing 1
required positional argument: 'aggregates'
pivot {"index": ["g"]} -> TypeError: pivot() missing 1
required positional argument: 'aggregates'
boxplot {"whisker_type": "tukey"} -> TypeError: boxplot() missing 2
required positional arguments: 'groupby' and 'metrics'
```
A bare `TypeError` is not an `InvalidPostProcessingError`, so these surface
as an unhandled 500 rather than a validation error. The published spec
advertises four payloads that crash the chart-data endpoint.
**The empty-value case splits three ways**, which is why `required=True`
alone is not the whole fix:
```
pivot {"index": ["g"], "aggregates": {}} ->
InvalidPostProcessingError (correct, a clean 400)
aggregate {"groupby": ["g"], "aggregates": {}} -> TypeError: Must
provide 'func' or tuples of '(column, aggfunc)
boxplot {"groupby": [], "metrics": [], ...} -> TypeError: Must
provide 'func' or tuples of '(column, aggfunc)
```
Pivot already handles empty correctly, your `minItems` reasoning on
`groupby` applies here in reverse, and its guard is the model the other two
lack. Aggregate and boxplot leak a raw pandas message instead.
**Marking them required is safe for existing charts.** Every frontend
operator that emits these operations sets the keys unconditionally:
`aggregateOperator.ts:55`, `pivotOperator.ts:48`,
`timeComparePivotOperator.ts:52`, and
`boxplotOperator.ts:60-61` (both via `ensureIsArray`, so possibly empty but
never absent). No saved `query_context` that the frontend produced can be
relying on the omission.
So: agreed, out of scope here, and I am not asking for it to be pulled in. I
will take it as a separate change, `required=True` on the four fields for the
omitted case, and a decision on the empty case for aggregate and boxplot,
rather than leaving it as a loose end.
--
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]
