sadpandajoe commented on code in PR #45680:
URL: https://github.com/apache/superset/pull/45680#discussion_r4448953734
##########
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) => {
Review Comment:
`categoryAt` maps every series hit to a category by `dataIndex`, including
the transparent `ASSIST_MARK` spacer series. Those spacers are drawn with a
`transparent` fill but are still hit-testable and carry the pointer cursor, so
with values `10` then `3` the second visible bar spans 10–13, and clicking the
empty area under it (0–10) sets or clears that category's cross-filter;
right-clicking it opens the drill menu. Should the handlers ignore events from
the assist series (e.g. check `event.seriesName`) while still accepting the
colored sign-crossing segments?
##########
superset-frontend/plugins/plugin-chart-echarts/test/Waterfall/EchartsWaterfall.test.tsx:
##########
@@ -0,0 +1,336 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { GenericDataType } from '@apache-superset/core/common';
+import { supersetTheme } from '@apache-superset/core/theme';
+import { ChartProps, SqlaFormData, TimeGranularity } from '@superset-ui/core';
+import { render } from '@testing-library/react';
+import EchartsWaterfall from '../../src/Waterfall/EchartsWaterfall';
+import transformProps from '../../src/Waterfall/transformProps';
+import { EchartsWaterfallChartProps } from '../../src/Waterfall/types';
+import Echart from '../../src/components/Echart';
+import { EventHandlers } from '../../src/types';
+
+jest.mock('../../src/components/Echart', () => ({
+ __esModule: true,
+ default: jest.fn(() => null),
+}));
+
+const mockedEchart = jest.mocked(Echart);
+
+const rows = [
+ { year: '2019', name: 'Sylvester', sum: 10 },
+ { year: '2019', name: 'Arnold', sum: 3 },
+ { year: '2020', name: 'Sylvester', sum: -10 },
+ { year: '2020', name: 'Arnold', sum: 5 },
+];
+
+const formData = {
+ colorScheme: 'bnbColors',
+ datasource: '3__table',
+ granularity_sqla: 'ds',
+ metric: 'sum',
+ x_axis: 'year',
+ groupby: 'name',
+ showTotal: true,
+};
+
+function renderChart(
+ overrides: Partial<ChartProps> & { emitCrossFilters?: boolean } = {},
+) {
+ const setDataMask = jest.fn();
+ const onContextMenu = jest.fn();
+ const chartProps = new ChartProps({
+ formData: formData as unknown as SqlaFormData,
+ width: 800,
+ height: 600,
+ queriesData: [{ data: rows }],
+ hooks: { setDataMask, onContextMenu },
+ theme: supersetTheme,
+ ...overrides,
+ });
+ const props = transformProps(
+ chartProps as unknown as EchartsWaterfallChartProps,
+ );
+ render(<EchartsWaterfall {...props} />);
+ const lastCall = mockedEchart.mock.calls[mockedEchart.mock.calls.length - 1];
+ const { eventHandlers } = lastCall[0] as { eventHandlers: EventHandlers };
+ return { eventHandlers, setDataMask, onContextMenu };
+}
+
+beforeEach(() => {
+ mockedEchart.mockClear();
+});
+
+function pointerEvent(dataIndex: number) {
+ return {
+ dataIndex,
+ event: {
+ stop: jest.fn(),
+ event: { clientX: 12, clientY: 24 } as PointerEvent,
+ },
+ };
+}
+
+test('clicking a breakdown bar filters the period and the breakdown', () => {
+ const { eventHandlers, setDataMask } = renderChart({
+ emitCrossFilters: true,
+ });
+ eventHandlers.click(pointerEvent(0));
+ expect(setDataMask).toHaveBeenCalledWith(
+ expect.objectContaining({
+ extraFormData: {
+ filters: [
+ { col: 'year', op: 'IN', val: ['2019'] },
+ { col: 'name', op: 'IN', val: ['Sylvester'] },
+ ],
+ },
+ }),
+ );
+});
+
+test('clicking a period total filters the period only', () => {
+ const { eventHandlers, setDataMask } = renderChart({
+ emitCrossFilters: true,
+ });
+ eventHandlers.click(pointerEvent(2));
+ expect(setDataMask).toHaveBeenCalledWith(
+ expect.objectContaining({
+ extraFormData: {
+ filters: [{ col: 'year', op: 'IN', val: ['2019'] }],
+ },
+ }),
+ );
+});
+
+test('clicking the same bar again clears the filter', () => {
+ const { eventHandlers, setDataMask } = renderChart({
+ emitCrossFilters: true,
+ filterState: { selectedValues: [JSON.stringify(['2019', 'Sylvester'])] },
+ });
+ eventHandlers.click(pointerEvent(0));
+ expect(setDataMask).toHaveBeenCalledWith(
+ expect.objectContaining({
+ extraFormData: { filters: [] },
+ }),
+ );
+});
+
+test('does not emit a filter when cross-filtering is disabled', () => {
+ const { eventHandlers, setDataMask } = renderChart({
+ emitCrossFilters: false,
+ });
+ eventHandlers.click(pointerEvent(0));
+ expect(setDataMask).not.toHaveBeenCalled();
+});
+
+test('clicking the grand total does not change the current filter', () => {
+ const { eventHandlers, setDataMask } = renderChart({
+ emitCrossFilters: true,
+ formData: { ...formData, groupby: undefined } as unknown as SqlaFormData,
+ });
+ eventHandlers.click(pointerEvent(2));
+ expect(setDataMask).not.toHaveBeenCalled();
+});
+
+const breakdownCrossFilter = {
+ dataMask: {
+ extraFormData: {
+ filters: [
+ { col: 'year', op: 'IN', val: ['2019'] },
+ { col: 'name', op: 'IN', val: ['Sylvester'] },
+ ],
+ },
+ filterState: {
+ label: '2019, Sylvester',
+ value: '2019, Sylvester',
+ selectedValues: [JSON.stringify(['2019', 'Sylvester'])],
+ },
+ },
+ isCurrentValueSelected: false,
+};
+
+test('a context menu carries the drill filters for the clicked bar', () => {
Review Comment:
Every context-menu test here renders with `emitCrossFilters: true`, so
nothing covers Drill to detail / Drill by when cross-filtering is off, which
the PR says should still work. If the `contextmenu` handler were gated on
`emitCrossFilters`, these tests would still pass. Could you add a case with
`emitCrossFilters: false` that right-clicks a breakdown bar and asserts
`onContextMenu` still receives the `drillToDetail` and `drillBy` filters while
`setDataMask` is not called?
##########
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: {
+ ...filterClause.col,
+ columnType: 'BASE_AXIS',
+ timeGrain: filterClause.col.timeGrain || filterClause.grain,
+ },
+ timeGrain: filterClause.grain,
comparator: 'val' in filterClause ? filterClause.val : undefined,
- } as SimpleAdhocFilter),
- };
+ } as unknown as SimpleAdhocFilter;
+ } else {
+ result = {
+ expressionType: 'SQL',
+ clause,
+ sqlExpression: translateToSql({
Review Comment:
For a Custom SQL breakdown such as `COALESCE(name, '')`, Drill by on a bar
whose value is `''` reaches this branch with `val: ''`. `translateToSql` goes
through `getSimpleSQLExpression`, which formats comparators with `optionLabel`,
so the empty string becomes the display marker `<empty string>` and (since
`Number('')` is not NaN) it is left unquoted, giving `(COALESCE(name, '')) =
<empty string>`. That is invalid SQL, so the drilled chart errors instead of
showing the matching rows. Could this branch keep a structured filter, or
serialize the real SQL literal for empty strings?
--
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]
