sadpandajoe commented on code in PR #44550:
URL: https://github.com/apache/superset/pull/44550#discussion_r4369581244
##########
superset-frontend/packages/superset-ui-chart-controls/src/operators/contributionOperator.ts:
##########
@@ -16,19 +16,72 @@
* specific language governing permissions and limitationsxw
* under the License.
*/
-import { PostProcessingContribution } from '@superset-ui/core';
+import {
+ ComparisonType,
+ ensureIsArray,
+ getMetricLabel,
+ PostProcessingContribution,
+ QueryFormData,
+ QueryObject,
+} from '@superset-ui/core';
import { PostProcessingFactory } from './types';
+import {
+ extractExtraMetrics,
+ isTimeComparison,
+ TIME_COMPARISON_SEPARATOR,
+} from './utils';
+
+/**
+ * Labels of the displayed metric columns present when the contribution runs,
+ * i.e. every pivoted column except the sort-only "Sort By" metric (see
+ * `extractExtraMetrics`). Row contribution divides by the sum of all numeric
+ * columns, so the hidden metric must be left out of the calculation.
+ */
+const getContributionColumns = (
+ formData: QueryFormData,
+ queryObject: QueryObject,
+): string[] | undefined => {
+ const metricLabels = ensureIsArray(queryObject.metrics).map(getMetricLabel);
+ const hasSortOnlyMetric = extractExtraMetrics(formData).some(
+ metric => !metricLabels.includes(getMetricLabel(metric)),
+ );
+ if (!hasSortOnlyMetric) {
+ return undefined;
+ }
+ if (!isTimeComparison(formData, queryObject)) {
+ return metricLabels;
+ }
+ const offsets = ensureIsArray(formData.time_compare);
+ const withOffsets = metricLabels.flatMap(metric =>
+ offsets.map(offset => [metric, offset].join(TIME_COMPARISON_SEPARATOR)),
+ );
+ if (formData.comparison_type === ComparisonType.Values) {
+ return [...metricLabels, ...withOffsets];
+ }
+ // the compare operator drops the originals and adds one derived column per
+ // metric and offset, named `<type>__<metric>__<metric>__<offset>`
+ return metricLabels.flatMap(metric =>
+ offsets.map(offset =>
+ [
+ formData.comparison_type,
+ metric,
+ [metric, offset].join(TIME_COMPARISON_SEPARATOR),
+ ].join(TIME_COMPARISON_SEPARATOR),
+ ),
+ );
+};
-/* eslint-disable @typescript-eslint/no-unused-vars */
export const contributionOperator: PostProcessingFactory<
PostProcessingContribution
> = (formData, queryObject, time_shifts) => {
if (formData.contributionMode) {
+ const columns = getContributionColumns(formData, queryObject);
return {
operation: 'contribution',
options: {
orientation: formData.contributionMode,
time_shifts,
+ ...(columns ? { columns } : {}),
Review Comment:
With a dimension set, Contribution Mode on, and a Sort By metric (for
example `metrics: ['count']`, `groupby: ['platform']`, Sort By `sales`), this
sends `columns: ['count']`. Contribution runs after the pivot and before the
flatten, so the backend's numeric columns are still tuples like `('count',
'X')` and `('count', 'Y')`, and its check of each requested column against that
list rejects the bare `'count'` ('Column "count" is not numeric or does not
exist in the query results'). The chart then fails to load in both row and
series mode, instead of just ignoring the hidden metric. The new operator tests
only assert the generated options, so they would not catch it. Could the
exclusion be done another way (for example by naming the pivoted tuple columns,
or leaving the hidden metric out of the contribution step on the backend), with
a test that runs the grouped case through the contribution function?
--
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]
