EnxDev commented on code in PR #45680:
URL: https://github.com/apache/superset/pull/45680#discussion_r4415423588


##########
superset-frontend/src/utils/simpleFilterToAdhoc.ts:
##########
@@ -75,16 +75,36 @@ export const simpleFilterToAdhoc = (
 
   let result: AdhocFilter;
   if (isAdhocColumn(filterClause.col)) {
-    result = {
-      expressionType: 'SQL',
-      clause,
-      sqlExpression: translateToSql({
-        expressionType: ExpressionTypes.Simple,
-        subject: `(${filterClause.col.sqlExpression})`,
+    // A grain on an adhoc expression has to stay on the column. Compiling it
+    // to raw SQL drops the grain, so the bucket is no longer truncated.
+    if (filterClause.grain) {
+      result = {
+        expressionType: 'SIMPLE',
+        clause,
         operator: filterClause.op,
+        operatorId: Object.entries(OPERATOR_ENUM_TO_OPERATOR_TYPE).find(
+          operatorEntry => operatorEntry[1].operation === filterClause.op,
+        )?.[0],
+        subject: {

Review Comment:
   This is the first path that puts an adhoc column object into a SIMPLE 
filter's `subject` (hence the `as unknown as`), and `convertFilter` passes it 
through fine for the query.
   
   The Explore pill only reads `column_name` though 
(`explore/exploreUtils/index.ts:472`), so after Drill by > Edit chart, or 
opening Explore from a dashboard with this cross-filter on, it shows something 
like `[P1M] = '2021-01-01T00:00:00'` with no column. Could that fallback use 
the adhoc `label`, plus a `getDefaultLabel` test for this shape?



##########
superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/EchartsWaterfall.tsx:
##########
@@ -16,21 +16,146 @@
  * specific language governing permissions and limitations
  * under the License.
  */
+import { useMemo } from 'react';
+import {
+  ContextMenuFilters,
+  getColumnLabel,
+  TimeGranularity,
+} from '@superset-ui/core';
 import Echart from '../components/Echart';
 import { WaterfallChartTransformedProps } from './types';
 import { EventHandlers } from '../types';
 import { useLegendEventHandlers } from '../utils/legendEventHandlers';
+import {
+  getSelectedWaterfallKeys,
+  getWaterfallCrossFilterDataMask,
+  getWaterfallDrillFilters,
+} from './crossFilter';
+
+const noop = () => {};
+
+type WaterfallBarEvent = {
+  dataIndex?: number;
+  event?: { stop: () => void; event: PointerEvent };
+};
 
 export default function EchartsWaterfall(
   props: WaterfallChartTransformedProps,
 ) {
-  const { height, width, echartOptions, refs, onLegendStateChanged, formData } 
=
-    props;
+  const {
+    categories,
+    coltypeMapping,
+    echartOptions,
+    emitCrossFilters,
+    filterState,
+    formData,
+    height,
+    onContextMenu,
+    onLegendStateChanged,
+    refs,
+    setDataMask = noop,
+    width,
+  } = props;
+  const { xAxisTimeFormat } = formData;
+  const timeGrain: TimeGranularity | undefined =
+    formData.extraFormData?.time_grain_sqla ?? formData.timeGrainSqla;
 
   // The waterfall legend has a fixed set of three entries, so it never
   // paginates and has no scroll position to preserve.
-  const eventHandlers: EventHandlers =
-    useLegendEventHandlers(onLegendStateChanged);
+  const legendEventHandlers = useLegendEventHandlers(onLegendStateChanged);
+
+  const crossFilterHandlers: EventHandlers = useMemo(() => {
+    const selectedKeys = getSelectedWaterfallKeys(filterState?.selectedValues);
+    const categoryAt = (event: WaterfallBarEvent) => {
+      if (event.dataIndex == null) {
+        return undefined;
+      }
+      const category = categories[event.dataIndex];
+      if (!category || category.isGrandTotal) {
+        return undefined;
+      }
+      return category;
+    };
+
+    return {
+      click: (event: WaterfallBarEvent) => {
+        if (!emitCrossFilters) {
+          return;
+        }
+        const category = categoryAt(event);
+        if (!category) {
+          return;
+        }
+        setDataMask(
+          getWaterfallCrossFilterDataMask({
+            category,
+            selectedKeys,
+            coltypeMapping,
+            timeFormat: xAxisTimeFormat,
+            timeGrain,
+          }).dataMask,
+        );
+      },
+      contextmenu: (event: WaterfallBarEvent) => {
+        if (!onContextMenu) {
+          return;
+        }
+        const category = categoryAt(event);
+        if (!category) {
+          return;
+        }
+        event.event?.stop();
+        const pointerEvent = event.event?.event;
+        if (!pointerEvent) {
+          return;
+        }
+        // IS NULL is unary. The menu types only binary operators, and both
+        // drill paths forward `op` unchanged.
+        const drillFilters = getWaterfallDrillFilters({
+          category,
+          coltypeMapping,
+          timeFormat: xAxisTimeFormat,
+          timeGrain,
+        }) as NonNullable<ContextMenuFilters['drillToDetail']>;
+        const xColumnLabel = getColumnLabel(category.xColumn);

Review Comment:
   Nothing tests this with a Custom SQL X axis: the component tests all use 
string columns, where the old `filter.col === category.xColumn` check behaves 
the same, so a revert would still pass.
   
   `adhocBucketColumn` returns a copy, so this label match is what keeps an 
adhoc X clause in `xAxisFilters` (the Drill by scope selector needs it there). 
Worth one `contextmenu` case with an adhoc temporal X and a month grain?



-- 
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]

Reply via email to