Antonio-RiveroMartnez commented on code in PR #32767:
URL: https://github.com/apache/superset/pull/32767#discussion_r2010267751
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/buildQuery.ts:
##########
@@ -47,5 +48,19 @@ export default function buildQuery(formData: QueryFormData) {
flattenOperator(formData, baseQueryObject),
],
},
+
+ {
Review Comment:
Do we need this block here or is it OK just to include the new
`aggregationOperator` in the list above afer or before the `flattenOperator` if
teh value is not `LAST_VALUE`?
##########
superset-frontend/packages/superset-ui-core/test/query/types/PostProcessing.test.ts:
##########
@@ -61,7 +61,7 @@ const AGGREGATES_OPTION: Aggregates = {
};
const AGGREGATE_RULE: PostProcessingAggregation = {
- operation: 'aggregation',
+ operation: 'aggregate',
Review Comment:
❤️ oh so there was a mismatch between the UI and how the BE was expecting
this operator to work eh? Thanks for fixing that
##########
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/customControls.tsx:
##########
@@ -61,6 +61,30 @@ const xAxisSortVisibility = ({ controls }: { controls:
ControlStateMapping }) =>
ensureIsArray(controls?.groupby?.value).length === 0 &&
ensureIsArray(controls?.metrics?.value).length === 1;
+export const aggregationControl = {
+ name: 'aggregation',
+ config: {
+ type: 'SelectControl',
+ label: t('Aggregation Method'),
+ default: 'LAST_VALUE',
Review Comment:
DO we need any special handling for this value since it's not a valid
`Numpy` function to run aggregations with? Like, we need to make sure we don't
send the operator when building the query if this is the value in formaData.
##########
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/customControls.tsx:
##########
@@ -61,6 +61,30 @@ const xAxisSortVisibility = ({ controls }: { controls:
ControlStateMapping }) =>
ensureIsArray(controls?.groupby?.value).length === 0 &&
ensureIsArray(controls?.metrics?.value).length === 1;
+export const aggregationControl = {
Review Comment:
Could we consider adding the list of options that are supported by the
backend instead of this shortened version so we could easily match those there
thus we can reuse this control later down the road on Pivot charts and others
if needed? Like step 1: This PR with the base shared control and using the
backend, step 2 (follow up): replace any aggregation control (from Pivot table
for example) to use this new control and use the backend for aggregation like
in this PR.
--
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]
