bito-code-review[bot] commented on code in PR #38369:
URL: https://github.com/apache/superset/pull/38369#discussion_r2880520251
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx:
##########
@@ -187,6 +188,7 @@ export default function TableChart<D extends DataRecord =
DataRecord>(
lastFilteredColumn: completeFilterState.lastFilteredColumn,
lastFilteredInputPosition: completeFilterState.inputPosition,
currentPage: 0, // Reset to first page when filtering
+ metricSqlExpressions,
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Inconsistent metricSqlExpressions in state
updates</b></div>
<div id="fix">
The diff adds `metricSqlExpressions` to the `modifiedOwnState` in
`handleFilterChanged`, but other handlers like `handleServerPaginationChange`,
`handlePageSizeChange`, `handleChangeSearchCol`, `handleSearch`, and
`handleSortByChange` do not include it. Since `metricSqlExpressions` is
required for query building in `buildQuery` for all server-side operations, it
should be included in all `modifiedOwnState` objects to ensure consistent
behavior and prevent potential query failures or incorrect results when users
perform pagination, search, or sorting without first applying filters.
</div>
</div>
<small><i>Code Review Run #c39480</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:
##########
@@ -482,12 +482,76 @@ const buildQuery: BuildQuery<TableChartFormData> = (
};
}
- // Add AG Grid complex WHERE clause from ownState (non-metric filters)
+ // Map metric/column labels to SQL expressions for WHERE/HAVING
resolution
+ const sqlExpressionMap: Record<string, string> = {};
+ (metrics || []).forEach((m: QueryFormMetric) => {
+ if (typeof m === 'object' && 'expressionType' in m) {
+ const label = getMetricLabel(m);
+ if (m.expressionType === 'SQL' && m.sqlExpression) {
+ sqlExpressionMap[label] = m.sqlExpression;
+ } else if (
+ m.expressionType === 'SIMPLE' &&
+ m.aggregate &&
+ m.column?.column_name
+ ) {
+ sqlExpressionMap[label] =
+ `${m.aggregate}(${m.column.column_name})`;
+ }
+ }
+ });
+ // Map dimension columns with custom SQL expressions
+ (columns || []).forEach((col: QueryFormColumn) => {
+ if (typeof col === 'object' && 'sqlExpression' in col) {
+ const label = getColumnLabel(col);
+ if (col.sqlExpression) {
+ sqlExpressionMap[label] = col.sqlExpression;
+ }
+ }
+ });
+ // Merge datasource-level saved metrics and calculated columns
+ if (ownState.metricSqlExpressions) {
+ Object.entries(
+ ownState.metricSqlExpressions as Record<string, string>,
+ ).forEach(([label, expression]) => {
+ if (!sqlExpressionMap[label]) {
+ sqlExpressionMap[label] = expression;
+ }
+ });
+ }
+
+ const resolveLabelsToSQL = (clause: string): string => {
+ let resolved = clause;
+ // Sort by label length descending to prevent substring false positives
+ const sortedEntries = Object.entries(sqlExpressionMap).sort(
+ ([a], [b]) => b.length - a.length,
+ );
+ sortedEntries.forEach(([label, expression]) => {
+ if (resolved.includes(label)) {
+ const escapedLabel = label.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
+ // Wrap complex expressions in parentheses for valid SQL
+ const isExpression =
+ expression.includes('(') ||
+ expression.toUpperCase().includes('CASE') ||
+ expression.includes('\n');
+ const wrappedExpression = isExpression
+ ? `(${expression})`
+ : `"${expression}"`;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Invalid SQL generation for column identifiers</b></div>
<div id="fix">
Change wrappedExpression to use expression directly for non-expressions
instead of quoting it.
</div>
</div>
<small><i>Code Review Run #c39480</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
