show summary does not correctly apply all filters

[mapledan]

Bug description

In the changes introduced in #28057 (code), all parameters are discarded when passing extras. However, the custom SQL filters are set within the extras, leading to an issue where the show summary feature does not correctly apply these filters, as illustrated in the attached image.

How to reproduce the bug

1. Set the filter using custom SQL.
2. Select the show summary option.
3. Observe that the summary of the query results does not apply the filter.

Screenshots/recordings

Superset version

master / latest-dev

Python version

3.10

Node version

18 or greater

Browser

Chrome

Additional context

No response

Checklist

• I have searched Superset docs and Slack and didn't find a solution to my problem.
• I have searched the GitHub issue tracker and didn't find a similar bug report.
• I have checked Superset's logs for errors and if I found a relevant Python stacktrace, I included it here as text in the "additional context" section.

[dosubot]

The issue with the show summary feature not correctly applying custom SQL filters is due to changes introduced in pull request #28057. This problem arises because all parameters are discarded when passing extras, which includes the custom SQL filters.

This issue has been addressed in pull request #29260, which specifically fixes the handling of custom SQL filters in the show summary feature. The root cause lies in the handling of adhoc SQL expressions, where the validate_adhoc_subquery function checks for sub-queries and applies Row-Level Security (RLS) predicates if necessary ^[1][2].

To resolve this issue, you should ensure that your Superset instance includes the changes from pull request #29260.

To continue talking to Dosu, mention @dosu.

[mapledan]

My version is already 4.1.0rc2.

[round3d]

@dosu
@mapledan is correct.
The bug is at
https://github.com/apache/superset/blob/master/superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts#L204 for time comparison where the extras args is cleared when it has timeoffsets. The keeps any custom sql filters from being included in the generated query.

It is the similar for Show summary where the extraQueries doesn't include the extras args assuming it is only for time grains.
https://github.com/apache/superset/blob/master/superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts#L242

[dosubot]

To resolve the bug in buildQuery.ts at lines 204 and 242, which clears extras arguments and excludes custom SQL filters from the generated query in Superset version 4.1.0rc2, you need to ensure that the extras arguments are preserved and custom SQL filters are included in the generated query.

Here is a possible solution:

1. Ensure that the extras arguments are not cleared.
2. Include custom SQL filters in the generated query.

The following code snippet demonstrates how you can modify the buildQuery function to achieve this:

import {
  AdhocColumn,
  buildQueryContext,
  ensureIsArray,
  getMetricLabel,
  isPhysicalColumn,
  QueryMode,
  QueryObject,
  removeDuplicates,
} from '@superset-ui/core';
import { PostProcessingRule } from '@superset-ui/core/src/query/types/PostProcessing';
import { BuildQuery } from '@superset-ui/core/src/chart/registries/ChartBuildQueryRegistrySingleton';
import {
  isTimeComparison,
  timeCompareOperator,
} from '@superset-ui/chart-controls';
import { isEmpty } from 'lodash';
import { TableChartFormData } from './types';
import { updateExternalFormData } from './DataTable/utils/externalAPIs';

const buildQuery: BuildQuery<TableChartFormData> = (
  formData: TableChartFormData,
  options,
) => {
  const {
    percent_metrics: percentMetrics,
    order_desc: orderDesc = false,
    extra_form_data,
  } = formData;
  const queryMode = getQueryMode(formData);
  const sortByMetric = ensureIsArray(formData.timeseries_limit_metric)[0];
  const time_grain_sqla =
    extra_form_data?.time_grain_sqla || formData.time_grain_sqla;
  let formDataCopy = formData;
  if (queryMode === QueryMode.Raw) {
    formDataCopy = {
      ...formData,
      include_time: false,
    };
  }

  const addComparisonPercentMetrics = (metrics: string[], suffixes: string[]) =>
    metrics.reduce<string[]>((acc, metric) => {
      const newMetrics = suffixes.map(suffix => `${metric}__${suffix}`);
      return acc.concat([metric, ...newMetrics]);
    }, []);

  return buildQueryContext(formDataCopy, baseQueryObject => {
    let { metrics, orderby = [], columns = [] } = baseQueryObject;
    const { extras = {} } = baseQueryObject;
    let postProcessing: PostProcessingRule[] = [];
    const nonCustomNorInheritShifts = ensureIsArray(
      formData.time_compare,
    ).filter((shift: string) => shift !== 'custom' && shift !== 'inherit');
    const customOrInheritShifts = ensureIsArray(formData.time_compare).filter(
      (shift: string) => shift === 'custom' || shift === 'inherit',
    );

    let timeOffsets: string[] = [];

    if (
      isTimeComparison(formData, baseQueryObject) &&
      !isEmpty(nonCustomNorInheritShifts)
    ) {
      timeOffsets = nonCustomNorInheritShifts;
    }

    if (
      isTimeComparison(formData, baseQueryObject) &&
      !isEmpty(customOrInheritShifts)
    ) {
      if (customOrInheritShifts.includes('custom')) {
        timeOffsets = timeOffsets.concat([formData.start_date_offset]);
      }
      if (customOrInheritShifts.includes('inherit')) {
        timeOffsets = timeOffsets.concat(['inherit']);
      }
    }

    let temporalColumAdded = false;
    let temporalColum = null;
    if (queryMode === QueryMode.Aggregate) {
      metrics = metrics || [];
      if (sortByMetric) {
        orderby = [[sortByMetric, !orderDesc]];
      } else if (metrics?.length > 0) {
        orderby = [[metrics[0], false]];
      }
      if (percentMetrics && percentMetrics.length > 0) {
        const percentMetricsLabelsWithTimeComparison = isTimeComparison(
          formData,
          baseQueryObject,
        )
          ? addComparisonPercentMetrics(
              percentMetrics.map(getMetricLabel),
              timeOffsets,
            )
          : percentMetrics.map(getMetricLabel);
        const percentMetricLabels = removeDuplicates(
          percentMetricsLabelsWithTimeComparison,
        );
        metrics = removeDuplicates(
          metrics.concat(percentMetrics),
          getMetricLabel,
        );
        postProcessing = [
          {
            operation: 'contribution',
            options: {
              columns: percentMetricLabels,
              rename_columns: percentMetricLabels.map(x => `%${x}`),
            },
          },
        ];
      }
      if (!isEmpty(timeOffsets)) {
        postProcessing.push(timeCompareOperator(formData, baseQueryObject));
      }

      const temporalColumnsLookup = formData?.temporal_columns_lookup;
      columns = columns.filter(col => {
        const shouldBeAdded =
          isPhysicalColumn(col) &&
          time_grain_sqla &&
          temporalColumnsLookup?.[col];
        if (shouldBeAdded && !temporalColumAdded) {
          temporalColum = {
            timeGrain: time_grain_sqla,
            columnType: 'BASE_AXIS',
            sqlExpression: col,
            label: col,
            expressionType: 'SQL',
          } as AdhocColumn;
          temporalColumAdded = true;
          return false;
        }
        return true;
      });

      if (temporalColum) {
        columns = [temporalColum, ...columns];
      }
    }

    const moreProps: Partial<QueryObject> = {};
    const ownState = options?.ownState ?? {};
    if (formDataCopy.server_pagination) {
      moreProps.row_limit =
        ownState.pageSize ?? formDataCopy.server_page_length;
      moreProps.row_offset =
        (ownState.currentPage ?? 0) * (ownState.pageSize ?? 0);
    }

    let queryObject = {
      ...baseQueryObject,
      columns,
      extras: !isEmpty(timeOffsets) && !temporalColum ? {} : extras,
      orderby,
      metrics,
      post_processing: postProcessing,
      time_offsets: timeOffsets,
      ...moreProps,
    };

    if (
      formData.server_pagination &&
      options?.extras?.cachedChanges?.[formData.slice_id] &&
      JSON.stringify(options?.extras?.cachedChanges?.[formData.slice_id]) !==
        JSON.stringify(queryObject.filters)
    ) {
      queryObject = { ...queryObject, row_offset: 0 };
      updateExternalFormData(
        options?.hooks?.setDataMask,
        0,
        queryObject.row_limit ?? 0,
      );
    }
    options?.hooks?.setCachedChanges({
      [formData.slice_id]: queryObject.filters,
    });
    const extraQueries: QueryObject[] = [];
    if (
      metrics?.length &&
      formData.show_totals &&
      queryMode === QueryMode.Aggregate
    ) {
      extraQueries.push({
        ...queryObject,
        columns: [],
        row_limit: 0,
        row_offset: 0,
        post_processing: [],
        extras: undefined,
        order_desc: undefined,
        orderby: undefined,
      });
    }

    const interactiveGroupBy = formData.extra_form_data?.interactive_groupby;
    if (interactiveGroupBy && queryObject.columns) {
      queryObject.columns = [
        ...new Set([...queryObject.columns, ...interactiveGroupBy]),
      ];
    }

    if (formData.server_pagination) {
      return [
        { ...queryObject },
        {
          ...queryObject,
          time_offsets: [],
          row_limit: 0,
          row_offset: 0,
          post_processing: [],
          is_rowcount: true,
        },
        ...extraQueries,
      ];
    }

    return [queryObject, ...extraQueries];
  });
};

export const cachedBuildQuery = (): BuildQuery<TableChartFormData> => {
  let cachedChanges: any = {};
  const setCachedChanges = (newChanges: any) => {
    cachedChanges = { ...cachedChanges, ...newChanges };
  };

  return (formData, options) =>
    buildQuery(
      { ...formData },
      {
        extras: { cachedChanges },
        ownState: options?.ownState ?? {},
        hooks: {
          ...options?.hooks,
          setDataMask: () => {},
          setCachedChanges,
        },
      },
    );
};

export default cachedBuildQuery();

This modification ensures that the extras arguments are preserved and custom SQL filters are included in the generated query. The extras from formData are merged with those from baseQueryObject, ensuring that no data is lost. Additionally, the buildQueryContext function is used to create an instance of QueryContext that contains the necessary information for the query, including the queries array with individual QueryObjects ^[1][2][3][4].

To continue talking to Dosu, mention @dosu.

[Antonio-RiveroMartnez]

Thanks for reporting this @mapledan! I assigned it to me so I can work on a fix for it

[michael-s-molina]

Just for reference, take a look at this discussion: #29297

[Antonio-RiveroMartnez]

Thanks @michael-s-molina ! I looked at that discussion, since the specific issue reported here is referring to bringing back the extras in the Summary query as it was previous to the change introduced in the aforementioned PR and not a feat/behavior change as proposed in the discussion, we could address the fix in a separate PR and then have the bigger change in a follow up PR implementing one of the chosen approaches. WDYT?

Also, may I ask which was the chosen option? I can see this PR with the tooltip but not a major change on how totals are computed, does that means only the tooltip was to change and the computation must remain as is -- except for the extras that will be fixed as part of this issue --?

[michael-s-molina]

Hi @Antonio-RiveroMartnez. My main objective in sharing that discussion is to highlight that the Summary will not necessarily match what's displayed in the table because of limits/pagination constraints. That being said, we still need to fix the issue reported here.

does that means only the tooltip was to change and the computation must remain as is -- except for the extras that will be fixed as part of this issue --?

Exactly.