gabotorresruiz commented on code in PR #44308:
URL: https://github.com/apache/superset/pull/44308#discussion_r4064938343
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:
##########
@@ -649,20 +621,63 @@ export const buildQueryUncached:
BuildQuery<TableChartFormData> = (
];
}
- // Apply AG Grid filters as SQL WHERE/HAVING clauses
- if (ownState.sqlClauses) {
- const { whereClause, havingClause } = classifySQLClauses(
- ownState.sqlClauses as Record<string, string>,
- );
+ // Apply AG Grid header filters. Simple single-condition, non-metric
+ // filters are sent as structured `{ col, op, val }` filters so the
+ // backend (SQLAlchemy) quotes each identifier for the target dialect.
+ // Unlike a raw `extras.where` string, this works for column names with
+ // spaces or reserved words across ClickHouse/Postgres/MySQL/BigQuery --
+ // a raw fragment like `Destination Address Street ILIKE '%x%'` fails
+ // backend clause validation, and no fixed quote character is valid for
+ // every dialect. Compound (AND/OR) and metric (HAVING) filters remain
+ // free-form SQL, matching the live in-grid path.
+ if (ownState.agGridFilterModel) {
+ // Percent metrics are keyed as `%<label>` (see transformProps), so
+ // include them alongside regular metric labels; otherwise a
+ // percent-metric filter would be emitted as a structured WHERE filter
+ // instead of routed to HAVING, changing the results.
+ const metricColumns = [
+ ...(metrics || []).map(m =>
+ typeof m === 'string' ? m : getMetricLabel(m),
+ ),
+ ...(percentMetrics || []).map(
+ m => `%${typeof m === 'string' ? m : getMetricLabel(m)}`,
+ ),
+ ];
Review Comment:
This block worries me a bit. It replaces `colId.startsWith('%')`, which
caught two shapes: percent metrics keyed `%<label>` (covered here) and the time
comparison columns, which `transformProps.ts:319` keys as `% <label>` with a
space. `% <label>` is not in `metricColumns`, so it now falls through to
`simpleFilters` as a dimension.
I ran both sides. On the parent commit a header filter on `% count` becomes
`extras.having` = `% count > 5` and the backend answers `Cannot parse SQL
clause`, so the user gets a visible 400. At this head it becomes `{ col: '%
count', op: '>', val: 5 }`, and `models/helpers.py:5012` only builds a
predicate when `col_obj or sqla_col is not None`, with no else branch. I
confirmed against a live instance that the request then returns 200 with no
`WHERE` clause and the full unfiltered row count. So a time comparison chart
trades a visible error for an export that quietly holds the wrong rows.
Keeping a `colId.startsWith('%')` fallback in the classification restores
the old routing. More generally I would only emit a structured filter for a
colId you know the backend can resolve, and leave anything else on the raw path
so it stays loud. A test named something like `routes a "% <metric>" time
comparison download filter to HAVING, not a structured WHERE filter` would pin
it. The existing percent metric test passes either way, because `%count` has no
space.
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:
##########
@@ -649,20 +621,63 @@ export const buildQueryUncached:
BuildQuery<TableChartFormData> = (
];
}
- // Apply AG Grid filters as SQL WHERE/HAVING clauses
- if (ownState.sqlClauses) {
- const { whereClause, havingClause } = classifySQLClauses(
- ownState.sqlClauses as Record<string, string>,
- );
+ // Apply AG Grid header filters. Simple single-condition, non-metric
+ // filters are sent as structured `{ col, op, val }` filters so the
+ // backend (SQLAlchemy) quotes each identifier for the target dialect.
+ // Unlike a raw `extras.where` string, this works for column names with
+ // spaces or reserved words across ClickHouse/Postgres/MySQL/BigQuery --
+ // a raw fragment like `Destination Address Street ILIKE '%x%'` fails
+ // backend clause validation, and no fixed quote character is valid for
+ // every dialect. Compound (AND/OR) and metric (HAVING) filters remain
+ // free-form SQL, matching the live in-grid path.
+ if (ownState.agGridFilterModel) {
+ // Percent metrics are keyed as `%<label>` (see transformProps), so
+ // include them alongside regular metric labels; otherwise a
+ // percent-metric filter would be emitted as a structured WHERE filter
+ // instead of routed to HAVING, changing the results.
+ const metricColumns = [
+ ...(metrics || []).map(m =>
+ typeof m === 'string' ? m : getMetricLabel(m),
+ ),
+ ...(percentMetrics || []).map(
+ m => `%${typeof m === 'string' ? m : getMetricLabel(m)}`,
+ ),
+ ];
+ const { simpleFilters, complexWhere, havingClause } =
+ convertAgGridFiltersToSQL(
+ ownState.agGridFilterModel as AgGridFilterModel,
+ metricColumns,
+ );
- if (whereClause || havingClause) {
+ if (simpleFilters.length > 0) {
+ // Drop any placeholder TEMPORAL_RANGE filters on the same columns so
+ // an AG Grid date filter fully replaces the "No filter" default.
+ const filteredCols = new Set(simpleFilters.map(f => f.col));
+ const existingFilters = (queryObject.filters || []).filter(
+ filter =>
+ !(
+ filter &&
+ typeof filter === 'object' &&
+ typeof filter.col === 'string' &&
+ filter.op === 'TEMPORAL_RANGE' &&
+ filteredCols.has(filter.col)
+ ),
+ );
+ agGridDownloadSimpleFilters.push(...simpleFilters);
+ queryObject.filters = [
+ ...existingFilters,
+ ...simpleFilters,
+ ] as QueryObject['filters'];
+ }
+
+ if (complexWhere || havingClause) {
Review Comment:
Not a blocker, just a scope note for the description. Compound (AND/OR)
filters and a date `notEqual` still leave through this branch with the
identifier unquoted, so on a spaced or reserved word column they still 400. I
checked at this head: a two condition text filter on `Destination Address
Street` produces `(Destination Address Street ILIKE '%a%' OR Destination
Address Street ILIKE '%b%')`, and the backend answers `Cannot parse SQL clause`.
The BEFORE/AFTER section currently reads as if filtered export is fixed for
every filter shape. Worth saying which shapes stay on the raw path.
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/test/buildQuery.test.ts:
##########
@@ -879,6 +885,170 @@ describe('plugin-chart-ag-grid-table', () => {
expect(totalsQuery.extras?.having).toBeUndefined();
});
+ test('sends filtered-download column filters as structured, dialect-safe
filters (sc-112797)', () => {
Review Comment:
Just a small NIT: the test name ends with an internal tracker id, which will
not mean anything to anyone reading it here. Could we describe the behavior
instead, something like `sends filtered download column filters as structured,
dialect safe filters`?
--
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]
