codeant-ai-for-open-source[bot] commented on code in PR #44308:
URL: https://github.com/apache/superset/pull/44308#discussion_r4015676554
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:
##########
@@ -649,20 +621,54 @@ 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) {
+ const metricColumns = (metrics || []).map(m =>
+ typeof m === 'string' ? m : getMetricLabel(m),
);
+ const { simpleFilters, complexWhere, havingClause } =
+ convertAgGridFiltersToSQL(
+ ownState.agGridFilterModel as AgGridFilterModel,
+ metricColumns,
+ );
Review 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`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c43bc07979244d6ea0e483a454e5594b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c43bc07979244d6ea0e483a454e5594b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44308&comment_hash=6e412ce757013297ec9fae257efa2427484347135d7e919a4c4c9d0bca784aa0&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44308&comment_hash=6e412ce757013297ec9fae257efa2427484347135d7e919a4c4c9d0bca784aa0&reaction=dislike'>๐</a>
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:
##########
@@ -649,20 +621,54 @@ 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) {
+ const metricColumns = (metrics || []).map(m =>
+ typeof m === 'string' ? m : getMetricLabel(m),
);
Review 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`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a48911f9b07b4e59a57fc76fa3c9bf47&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a48911f9b07b4e59a57fc76fa3c9bf47&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44308&comment_hash=64f05e89db8d13736762d10e1dc96b6161b67c7d77527ebaac96861bad161bf9&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44308&comment_hash=64f05e89db8d13736762d10e1dc96b6161b67c7d77527ebaac96861bad161bf9&reaction=dislike'>๐</a>
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/stateConversion.ts:
##########
@@ -384,9 +384,16 @@ export function convertAgGridStateToOwnState(
ownState.columnOrder = columnOrder;
}
- const filterConversion = convertFilterModel(agGridState.filterModel);
- if (filterConversion?.sqlClauses) {
- ownState.sqlClauses = filterConversion.sqlClauses;
+ // Forward the raw AG Grid filter model so buildQuery can convert header
+ // filters into structured, dialect-safe `{ col, op, val }` filters for
+ // download queries (see buildQuery's isDownloadQuery branch). Serializing
raw
+ // SQL clauses here instead leaves column identifiers unquoted, which breaks
+ // CSV/Excel export for column names with spaces or reserved words.
+ if (
+ agGridState.filterModel &&
+ Object.keys(agGridState.filterModel).length > 0
+ ) {
+ ownState.agGridFilterModel = agGridState.filterModel;
}
Review 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`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=eb182f37566b42e082702574c44eacda&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=eb182f37566b42e082702574c44eacda&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44308&comment_hash=e3822372ea5c00105bde2d75630b99dd4da15f8148486b2f0f192c9e1ffe47bc&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44308&comment_hash=e3822372ea5c00105bde2d75630b99dd4da15f8148486b2f0f192c9e1ffe47bc&reaction=dislike'>๐</a>
--
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]
