alexandrusoare commented on code in PR #38369:
URL: https://github.com/apache/superset/pull/38369#discussion_r2910736519
##########
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}"`;
+ resolved = resolved.replace(
+ new RegExp(`\\b${escapedLabel}\\b`, 'g'),
+ wrappedExpression,
+ );
+ }
+ });
+ return resolved;
+ };
Review Comment:
@amaannawab923 ?
##########
superset-frontend/src/pages/DatasetList/DatasetList.test.tsx:
##########
@@ -165,7 +165,7 @@ test('"Bulk select" button exists for export-only users',
async () => {
expect(
await screen.findByRole('button', { name: /bulk select/i }),
).toBeInTheDocument();
-});
+}, 30000);
Review Comment:
This is a separate matter and it's caused by some faulty tests. Since it's
not in the scope of the PR we can get rid of this
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:
##########
@@ -498,12 +562,13 @@ const buildQuery: BuildQuery<TableChartFormData> = (
};
}
Review Comment:
@amaannawab923 what do you think of this?
##########
superset-frontend/src/visualizations/presets/MainPreset.ts:
##########
@@ -101,7 +101,7 @@ export default class MainPreset extends Preset {
]
: [];
- const agGridTablePlugin = isFeatureEnabled(FeatureFlag.AgGridTableEnabled)
+ const agGridTablePlugin = true
Review Comment:
I agree with Bito, why are hardcoding it to True?
--
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]
