EnxDev commented on code in PR #45397: URL: https://github.com/apache/superset/pull/45397#discussion_r4394434434
########## superset-frontend/plugins/plugin-chart-echarts/src/Histogram/crossFilter.ts: ########## @@ -0,0 +1,197 @@ +/** + * 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 { + DataRecordValue, + getColumnLabel, + getTimeFormatter, + QueryFormColumn, + QueryObjectFilterClause, +} from '@superset-ui/core'; +import { GenericDataType } from '@apache-superset/core/common'; +import { formatSeriesName } from '../utils/series'; + +/** Matches `optionLabel` in `src/utils/common.ts`. */ +const EMPTY_STRING = '<empty string>'; + +export type HistogramBin = { + start: number; + end: number; + /** Numpy's last bin includes its right edge. Earlier bins do not. */ + isLast: boolean; + label: string; +}; + +const BIN_SEPARATOR = ' - '; + +/** + * Bin columns are named `"<start> - <end>"` by the histogram post-processing. + * The first separator is the delimiter, so a negative edge stays intact. + */ +export function parseBinKey( + key: string, +): { start: number; end: number } | undefined { + const separator = key.indexOf(BIN_SEPARATOR); + if (separator <= 0) { + return undefined; + } + const start = Number(key.slice(0, separator)); + const end = Number(key.slice(separator + BIN_SEPARATOR.length)); + if (!Number.isFinite(start) || !Number.isFinite(end)) { + return undefined; + } + return { start, end }; +} + +function toKeyPart(value: DataRecordValue): string | number | boolean | null { + if (value == null) { + return null; + } + if (typeof value === 'bigint') { + return value.toString(); + } + return value instanceof Date ? value.toISOString() : value; +} + +/** + * `bigint` throws in `JSON.stringify`, and the data mask is serialized into + * dashboard state and the chart-data request. + */ +function toJsonSafeFilterValue( + value: Exclude<DataRecordValue, null>, +): Exclude<DataRecordValue, null | bigint> { + return typeof value === 'bigint' ? value.toString() : value; +} + +export function getHistogramCrossFilterKey( + bin: Pick<HistogramBin, 'start' | 'end'>, + groupbyValues: DataRecordValue[], +): string { + return JSON.stringify([ + bin.start, + bin.end, + ...groupbyValues.map(value => toKeyPart(value)), + ]); +} + +export function getSelectedHistogramKeys(selectedValues: unknown): string[] { + if (!selectedValues) { + return []; + } + if (Array.isArray(selectedValues)) { + return selectedValues.map(value => String(value)); + } + if (typeof selectedValues === 'object') { + return Object.values(selectedValues as Record<string, unknown>).map(value => + String(value), + ); + } + return []; +} + +function formatGroupbyValue( + col: QueryFormColumn, + value: DataRecordValue, + coltypeMapping?: Record<string, number>, +): string { + if (value === '') { + return EMPTY_STRING; + } + return formatSeriesName(value, { + timeFormatter: getTimeFormatter(), + coltype: coltypeMapping?.[getColumnLabel(col)] as + | GenericDataType + | undefined, + }); +} + +function binFilters( + column: QueryFormColumn, + bin: HistogramBin, +): QueryObjectFilterClause[] { + return [ + { col: column, op: '>=', val: bin.start }, + { col: column, op: bin.isLast ? '<=' : '<', val: bin.end }, Review Comment: With cumulative on, the bar shows everything up to `end`, but the click only filters `[start, end)`. So a bar reading 80 sends the other charts the 10 rows in that slice. I saw the description calls this out as intended. Is that what users will expect, or should cumulative drop the lower bound? ########## superset-frontend/plugins/plugin-chart-echarts/test/Histogram/transformProps.test.ts: ########## @@ -108,5 +109,43 @@ test('does not treat adhoc groupby labels as histogram bin edges', () => { expect(xAxis.data).toHaveLength(2); expect(xAxis.data.every(label => !label.includes('NaN'))).toBe(true); expect(series[0].name).toBe('WEST'); - expect(series[0].data).toEqual([3, 7]); + expect(series[0].data).toEqual([{ value: 3 }, { value: 7 }]); +}); + +test('dims bars outside the selected bin and uses a pointer cursor', () => { + const props = new ChartProps({ + formData, + width: 800, + height: 600, + queriesData: [{ data: [{ '0 - 5': 3, '5 - 10': 7 }] }], + theme: supersetTheme, + filterState: { + selectedValues: [getHistogramCrossFilterKey({ start: 0, end: 5 }, [])], + }, + emitCrossFilters: true, + }); + const { series } = readChart(props); + const points = series[0].data as { + value: number; + itemStyle?: { opacity?: number }; + }[]; + + expect(points[0].itemStyle?.opacity).toBeUndefined(); + expect(points[1].itemStyle?.opacity).toBe(0.3); Review Comment: This only covers the no-groupby case, and the groupby values are what make the key differ between series. Mind adding a case where selecting WEST's `0 - 5` bar dims EAST's `0 - 5`? That's what breaks if transformProps and Histogram.tsx ever build the key from different values. ########## superset-frontend/plugins/plugin-chart-echarts/src/Histogram/transformProps.ts: ########## @@ -81,23 +103,47 @@ export default function transformProps( const percentFormatter = getPercentFormatter(NumberFormats.PERCENT_2_POINT); const groupbySet = new Set(groupby.map(getColumnLabel)); - const xAxisData: string[] = Object.keys(data[0]) - .filter(key => !groupbySet.has(key)) - .map(key => { - const array = key.split(' - ').map(value => parseFloat(value)); - return `${xAxisFormatter(array[0])} - ${xAxisFormatter(array[1])}`; - }); + const firstRow = data[0]; + const binKeys = firstRow + ? Object.keys(firstRow).filter( + key => !groupbySet.has(key) && parseBinKey(key) !== undefined, + ) + : []; + const bins: HistogramBin[] = binKeys.map((key, index) => { + const edges = parseBinKey(key); + const start = edges?.start ?? Number.NaN; + const end = edges?.end ?? Number.NaN; + const startLabel = xAxisFormatter(start); + const endLabel = xAxisFormatter(end); + return { + start, + end, + isLast: index === binKeys.length - 1, + label: `${startLabel} - ${endLabel}`, + }; + }); + const xAxisData = bins.map(bin => bin.label); + const seriesGroupby: DataRecordValue[][] = []; const barSeries: BarSeriesOption[] = data.map(datum => { + const groupbyValues = groupby.map( + key => datum[getColumnLabel(key)] as DataRecordValue, + ); + seriesGroupby.push(groupbyValues); const seriesName = - groupby.length > 0 - ? groupby.map(key => datum[getColumnLabel(key)]).join(', ') - : getColumnLabel(column); - const seriesData = Object.keys(datum) - .filter(key => groupbySet.has(key) === false) - .map(key => datum[key] as number); + groupby.length > 0 ? groupbyValues.join(', ') : getColumnLabel(column); + const seriesData = bins.map((bin, index) => { + const filterKey = getHistogramCrossFilterKey(bin, groupbyValues); + const isDimmed = + selectedKeys.length > 0 && !selectedKeys.includes(filterKey); Review Comment: The edges come from the data's min and max, so a native filter or new data that moves them leaves the stored key matching no bar. Every bar renders dimmed while the old range keeps filtering the other charts. Could we only dim when at least one bar's key is in `selectedKeys`? A test with a key from different edges would pin it. -- 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]
