gabotorresruiz commented on code in PR #44308:
URL: https://github.com/apache/superset/pull/44308#discussion_r4073504866
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/agGridFilterConverter.ts:
##########
@@ -629,7 +649,13 @@ export function convertAgGridFiltersToSQL(
return;
}
- const isMetric = metricColumnsSet.has(columnName);
+ // 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
+ // silently dropping it and returning unfiltered rows.
+ const isMetric =
+ metricColumnsSet.has(columnName) || columnName.startsWith('%');
Review 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 to `HAVING` and 400s, this head does the same. Identical, so no new misroute.
* `# <label>` and `△ <label>`: dropped by `validateColumnName` before either
branch on all three revisions, because `COLUMN_NAME_REGEX` excludes `#` 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.
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/agGridFilterConverter.ts:
##########
@@ -629,7 +649,13 @@ export function convertAgGridFiltersToSQL(
return;
}
- const isMetric = metricColumnsSet.has(columnName);
+ // 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
Review 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.
--
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]
