Repository navigation
fix(plugin-chart-ag-grid-table): send export column filters as structured filters - #44308
Conversation
…ured filters
The AG Grid table's "Export to CSV/Excel" re-queried /api/v1/chart/data
with the active header filters serialized into a raw extras.where/having
SQL string, interpolating the column identifier unquoted. A column name
containing spaces or a reserved word (e.g. a ClickHouse column
"Destination Address Street") produced an unparseable fragment like
`Destination Address Street ILIKE '%x%'`, which fails backend
sanitize_clause validation with "Invalid SQL clause" and returns a 400
with no file. This failed on every dialect, not only ClickHouse, and no
single identifier quote character is valid across ClickHouse, Postgres,
MySQL and BigQuery.
Route the download path through the plugin's existing
convertAgGridFiltersToSQL so simple, single-condition filters are sent as
structured `{ col, op, val }` filters on queryObject.filters. The backend
then quotes each identifier for the target dialect via SQLAlchemy, which
already worked for the live in-grid filtering. Compound (AND/OR) and
metric (HAVING) filters keep their existing free-form SQL handling, and
the totals/summary query still excludes the interactive filters.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code Review Agent Run #005d05Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| const metricColumns = (metrics || []).map(m => | ||
| typeof m === 'string' ? m : getMetricLabel(m), | ||
| ); |
There was a problem hiding this comment.
Suggestion: Metric filters use displayed labels directly in HAVING; calculated metrics are not resolved through sqlExpressionMap, causing invalid SQL or filtering the wrong expression during export. [api mismatch]
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts
**Line:** 634:636
**Comment:**
*Api Mismatch: Metric filters use displayed labels directly in `HAVING`; calculated metrics are not resolved through `sqlExpressionMap`, causing invalid SQL or filtering the wrong expression during export.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| const { simpleFilters, complexWhere, havingClause } = | ||
| convertAgGridFiltersToSQL( | ||
| ownState.agGridFilterModel as AgGridFilterModel, | ||
| metricColumns, | ||
| ); |
There was a problem hiding this comment.
Suggestion: An invalid persisted date filter can make getStartOfDay or getEndOfDay throw while building the download query, aborting the entire export. [possible bug]
Assessment: 🟠 Major · 🔁 Occurrence: Rarely
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts
**Line:** 637:641
**Comment:**
*Possible Bug: An invalid persisted date filter can make `getStartOfDay` or `getEndOfDay` throw while building the download query, aborting the entire export.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| if ( | ||
| agGridState.filterModel && | ||
| Object.keys(agGridState.filterModel).length > 0 | ||
| ) { | ||
| ownState.agGridFilterModel = agGridState.filterModel; | ||
| } |
There was a problem hiding this comment.
Suggestion: When the filter model is cleared, this converter omits agGridFilterModel; merging with existing ownState can retain the previous model and make later exports use stale filters. [stale reference]
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-ag-grid-table/src/stateConversion.ts
**Line:** 392:397
**Comment:**
*Stale Reference: When the filter model is cleared, this converter omits `agGridFilterModel`; merging with existing ownState can retain the previous model and make later exports use stale filters.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix…ort filters Address review of the structured-filter export change: - equals now emits the `==` FilterOperator value (not raw SQL `=`), and a numeric `inRange` is split into two bounded `>=`/`<=` filters -- there is no BETWEEN FilterOperator and a single structured filter can't carry both bounds. Fixed in convertAgGridFiltersToSQL so both the download and the live server-pagination paths emit backend-valid ops (this was a pre-existing defect the export change newly exposed). - A date "Not equal" filter now falls back to the raw (col < start OR col > end) clause instead of being silently dropped from exports. - Remove the now-dead raw-SQL clause builder (convertFilterModel and its chain) and the index.ts re-export; the download path forwards the filter model and converts via the structured path instead. - Tests: equals -> ==, inRange -> two bounds, date not-equal -> complexWhere, non-empty filter-model forwarding, and totals excluding the structured WHERE filter. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ort routing
Fixes CI (lint-frontend / tsc) and addresses automated review findings:
- Narrow filter.col to a string before Set.has(), fixing TS2345
("QueryFormColumn is not assignable to string") that failed the
type-check job. (jest runs via Babel and did not catch it.)
- Include percent-metric labels (keyed `%<label>`) in the metric column
set so a percent-metric download filter is routed to HAVING instead of
being emitted as a structured WHERE filter, restoring the prior
`startsWith('%')` classification.
- Guard getStartOfDay/getEndOfDay against invalid dates so a bad persisted
date filter returns the input instead of throwing on toISOString() and
aborting the whole export.
- Always forward the filter model (even when empty) so a cleared filter set
overwrites any stale persisted model on merge.
- Add a test asserting a percent-metric filter routes to HAVING, not WHERE.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code Review Agent Run #2d8adeActionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
Hi @rebenitez1802, thanks for chasing this one down. I checked the branch out and ran it rather than just reading it, and the core of the fix holds up: I reproduced the reported failure on the parent commit and confirmed it is gone at this head.
What I verified empirically:
- The old wire format fails exactly as you describe. Against a live Postgres instance,
extras.where=Destination Address Street ILIKE '%Main%'comes back asQueryObjectValidationError: Cannot parse SQL clause, and so doesselect ILIKE '%Main%'for a reserved word. The same clause with a plain column name runs fine, so it really is the unquoted identifier and not the dialect. - The structured format round trips. I drove the real
get_sqla_querywith{ col, op, val }for==,!=,ILIKE,NOT ILIKE,IS NULL,IS NOT NULL,IN,INwith anullelement,>,<,>=and<=across six column name shapes (spaces, a reserved word, a dot, an embedded double quote, accents, an apostrophe) and compiled each for SQLite, PostgreSQL, MySQL and BigQuery. Every identifier comes out correctly quoted. - The plugin suite is green at this head (23 suites, 363 tests), and 14 of the new or changed assertions fail on the parent commit, so they pin the fix rather than an invariant.
One thing I would fix before merge, plus a scope note and a nit, all inline.
Worth putting in the summary
This fixes more than the export path. On the parent commit convertAgGridFiltersToSQL emits op: '=' for equals and op: 'BETWEEN' for inRange, and those land directly in queryObject.filters on the live server paginated path. Neither is a member of FilterOperator, and I confirmed ChartDataQueryContextSchema rejects both, so an equals header filter on a server paginated AG Grid table 400s today. Your == and two bound filters change fixes that as well.
| ...(percentMetrics || []).map( | ||
| m => `%${typeof m === 'string' ? m : getMetricLabel(m)}`, | ||
| ), | ||
| ]; |
There was a problem hiding this 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.
| ] as QueryObject['filters']; | ||
| } | ||
|
|
||
| if (complexWhere || havingClause) { |
There was a problem hiding this 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.
| expect(totalsQuery.extras?.having).toBeUndefined(); | ||
| }); | ||
|
|
||
| test('sends filtered-download column filters as structured, dialect-safe filters (sc-112797)', () => { |
There was a problem hiding this 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?
…ters to HAVING
Time-comparison columns are keyed `% <label>` (with a space), distinct from
percent metrics (`%<label>`). The previous metric classification only matched
exact metric labels, so a header filter on a `% <label>` column fell through to
a structured WHERE filter on the download path. The backend can't resolve that
column and builds no predicate, silently returning 200 with unfiltered rows --
worse than the prior visible parse error.
Restore the `startsWith('%')` classification inside convertAgGridFiltersToSQL so
both `%<label>` and `% <label>` route to the raw HAVING path (staying loud if
unresolvable), and simplify the buildQuery metric-column set accordingly. This
also corrects the same case on the live server-pagination path.
Add a test for the time-comparison shape, and drop an internal tracker id from a
test name.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #fd8edeActionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
Thanks @rebenitez1802, this addresses everything I raised, and putting the check inside convertAgGridFiltersToSQL rather than in buildQuery is the better spot because it covers the live path too.
I re-ran the same comparison rather than trusting the diff, driving the real query pipeline against a live Postgres instance with the exact payload each revision emits:
% counton master:extras.having=% count > 5, backend answersCannot parse SQL clause, so a visible 400.% counton the previous head: structured{ col: '% count', op: '>', val: 5 }, backend answers 200 with noWHERE, noHAVINGand the full unfiltered row count.% countat this head: back toextras.having=% count > 5and the sameCannot parse SQL clause. Silent wrong data is gone.
Also re-checked the two secondary points. Compound AND/OR and date notEqual on a spaced column still raise Cannot parse SQL clause, which is what the new Scope section now says, so that reads accurately. The plugin suite is green here (23 suites, 364 tests), and your new test is the only one that fails against the previous head, so it pins this fix specifically rather than the invariant.
LGTM.
| // keeps an unresolvable filter loud (a parse error) instead of the backend | ||
| // silently dropping it and returning unfiltered rows. | ||
| const isMetric = | ||
| metricColumnsSet.has(columnName) || columnName.startsWith('%'); |
There was a problem hiding this comment.
One data point in case the % prefix heuristic gets questioned, since it is a fair thing to ask about. I measured the three colId shapes that could be affected on master, on the previous head and here:
%Growth, a genuine dimension whose name starts with%: master routes it toHAVINGand 400s, this head does the same. Identical, so no new misroute.# <label>and△ <label>: dropped byvalidateColumnNamebefore either branch on all three revisions, becauseCOLUMN_NAME_REGEXexcludes#and△. They never reach this classification, so not covering them here changes nothing.
So this line restores master's behavior rather than introducing a new rule, and the only revision that differed was the one in between. Worth knowing before anyone spends time on it.
| // Also treat any `%`-prefixed colId as a metric: percent metrics are keyed | ||
| // `%<label>` and time-comparison columns `% <label>` (see transformProps), | ||
| // and neither resolves as a dimension. Routing them to the raw HAVING path | ||
| // keeps an unresolvable filter loud (a parse error) instead of the backend |
There was a problem hiding this comment.
Not a blocker, just something the summary does not mention. Because this converter also feeds filterStateManager, the change reaches the live server paginated query, not only the download. I checked the converter output directly: on master a %count or % count header filter came out as simpleFilters, which the backend silently drops, so the grid showed unfiltered rows; at this head it becomes a havingClause, which flows through AgGridTableChart.tsx:305 into buildQuery.ts:596 and surfaces as an error instead.
That is the right direction and it makes the live and download paths agree for the first time. It does mean a user filtering a % column on a server paginated grid now sees an error where they previously saw (wrong) data, so it is worth a line in the summary.
SUMMARY
The AG Grid table's toolbar Export to CSV / Export to Excel re-queries
/api/v1/chart/datawith the active header filters. Those filters were serialized into a rawextras.where/havingSQL string with the column identifier interpolated unquoted. A column name containing spaces or a reserved word — e.g. a column namedDestination Address Street— produced an unparseable fragment like:which fails the backend's pre-execution
sanitize_clauseparse and returns a 400{"message":"Invalid SQL clause: ..."}with no file. This affects every SQL dialect (not only ClickHouse) whenever a filtered column name needs quoting, and there is no single identifier quote character valid across ClickHouse, Postgres, MySQL and BigQuery (double quotes are string literals in MySQL/BigQuery; backticks are invalid in Postgres). Unfiltered export and the live in-grid filtering both work, because only this path builds a raw identifier string.The download path now runs the plugin's existing
convertAgGridFiltersToSQL, so simple single-condition filters are sent as structured{ col, op, val }filters onqueryObject.filters. The backend then quotes each identifier for the target dialect via SQLAlchemy — exactly how the live in-grid filtering already worked, and it also removes the string-concatenation injection surface. Compound (AND/OR) and metric (HAVING) filters keep their existing free-form SQL handling, and the totals/summary query still excludes the interactive filters.Because
convertAgGridFiltersToSQLalso feeds the live server-paginated query (viafilterStateManager), this change reaches that path too: it fixes theequals/inRangeoperators (which emitted non-FilterOperatorvalues=/BETWEENand 400'd), and it routes%-prefixed columns (percent metrics / time comparison) to HAVING. One visible effect: filtering a%column on a server-paginated grid now surfaces an error instead of silently showing unfiltered rows — the live and download paths now agree.BEFORE/AFTER
Before: Apply a header filter ("contains" or "is one of") to a column whose name has spaces or is a reserved word, then Export to CSV/Excel → 400 "Invalid SQL clause", no file produced.
After: The export succeeds and returns the filtered rows, matching the grid view, across dialects — for the single-condition filter shapes that are converted to structured filters (contains / notContains / startsWith / endsWith / equals / notEqual / number comparisons / inRange / "is one of" / blank / notBlank).
Scope — still on the raw-SQL path: Compound per-column filters (two conditions joined by AND/OR) and a date "Not equal" filter are still emitted as free-form SQL, so for those specific shapes a column whose name contains spaces or is a reserved word can still fail backend clause validation. They fail loudly (a visible error) rather than being silently dropped, and are left as a follow-up (structured filters can't express per-column OR).
TESTING INSTRUCTIONS
Destination Address Street), build an AG Grid Table (Table V2) chart and place it on a dashboard.cd superset-frontend && npm run test -- plugins/plugin-chart-ag-grid-table(adds a regression test asserting spaced-column contains + set filters become structuredqueryObject.filterswith no rawextras.where).ADDITIONAL INFORMATION
🤖 Generated with Claude Code