bito-code-review[bot] commented on code in PR #38369:
URL: https://github.com/apache/superset/pull/38369#discussion_r2911386334
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:
##########
@@ -482,12 +482,77 @@ 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>SQL Identifier Quoting Removed</b></div>
<div id="fix">
Removing quotes around simple SQL expressions can break queries when column
names contain spaces or are reserved words, as SQL requires quoted identifiers
in such cases. The existing test 'should quote simple column names without
parentheses' expects this quoting behavior.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
const wrappedExpression = isExpression
? `(${expression})`
: `"${expression}"`;
````
</div>
</details>
</div>
<small><i>Code Review Run #1abcc2</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]
