aminghadersohi commented on code in PR #45612:
URL: https://github.com/apache/superset/pull/45612#discussion_r4379855945
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:
##########
@@ -679,41 +683,64 @@ export default function EchartsTimeseries({
const xAxisCol =
// if the xAxis is '__timestamp', granularity_sqla will be the
column of filter
xAxis.label === DTTM_ALIAS ? formData.granularitySqla : xAxis.label;
- if (data && xAxis.type === AxisType.Time && xAxisCol) {
- // For horizontal orientation the [x, value] pair is swapped
- const xValue = Array.isArray(data) ? data[xAxisValueIndex] : data;
- const xAxisFilter = getTemporalXAxisDrillByFilter(
- xAxisCol,
+ const isTemporalCategory =
+ xAxis.type === AxisType.Category &&
+ coltypeMapping?.[getColumnLabel(xAxis.label)] ===
+ GenericDataType.Temporal;
+ if ((xAxis.type === AxisType.Time || isTemporalCategory) && xAxisCol) {
+ // A markerless line/area event identifies the series but no point.
+ // Snap its pixel coordinate to an actual bucket in that series.
+ const series = ensureIsArray(echartOptions.series)[
+ eventParams.seriesIndex
+ ];
+ const xValue = getTimeAxisDrillByValue(
+ data ??
+ (isTemporalCategory && eventParams.name
+ ? eventParams.name
+ : undefined),
+ series,
+ data
+ ? undefined
+ : echartRef.current
+ ?.getEchartInstance()
+ ?.convertFromPixel({ seriesIndex: eventParams.seriesIndex },
[
+ eventParams.event.offsetX,
+ eventParams.event.offsetY,
+ ]),
+ xAxisValueIndex,
+ isTemporalCategory,
+ );
+ const xAxisFilter = getTemporalBucketDrillByFilter(
+ // A grained '__timestamp' bucket resolves to the time column the
+ // query groups by, including a dashboard Time Column override.
+ resolvedTimeGrain ? xAxis.label : xAxisCol,
Review Comment:
Fixed in 195a3a38834842260fe7f03cbb622c6606d402ab. I now emit the effective
real time column for legacy `__timestamp` axes (dashboard Time Column override
first, then `granularitySqla`) in both Timeseries and Mixed charts. Explicit
x-axis labels remain unchanged, including SQL axes.
Regression test: `Drill by uses the dashboard time grain for the clicked
time-axis bucket`
Its real-column assertion fails with the original source and passes here. I
also added the legacy Timeseries and Mixed Edit-chart cases, each with and
without a dashboard Time Column override; all four failed with `col:
__timestamp` before the change and pass with the real column. The scoped Jest
suites pass (481 tests), including query/CSV/Explore predicate transport;
Docker backend sibling suites pass (338 tests).
Siblings checked:
-
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:683
— fixed in 195a3a38834842260fe7f03cbb622c6606d402ab
-
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/EchartsMixedTimeseries.tsx:225
— fixed in 195a3a38834842260fe7f03cbb622c6606d402ab
-
superset-frontend/plugins/plugin-chart-echarts/src/utils/xAxisDrillByFilter.ts:197
— not affected: accepts its caller's column unchanged; both production callers
are covered above.
- superset-frontend/src/components/Chart/DrillBy/DrillBySubmenu.tsx:156 —
not affected: forwards the resolved bucket filter without changing its column.
- superset-frontend/src/components/Chart/DrillBy/DrillByModal.tsx:111 — not
affected: query, downloads and Edit chart share the drilled form data; it does
not create timestamp aliases.
-
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/mixins.tsx:47
— not affected: Explore can still initialize x_axis from granularity_sqla; the
persisted filter names the real column rather than the legacy alias.
- superset/models/helpers.py:5454 — not affected: physical time-column
filters already resolve in both legacy and non-legacy queries; backend changes
are unnecessary.
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/xAxisDrillByFilter.ts:
##########
@@ -165,3 +165,72 @@ export const getTemporalXAxisDrillByFilter = (
formattedVal,
};
};
+
+/**
+ * Build the Drill by filter for a clicked temporal bucket. With a time grain
+ * it matches the grained column, like Drill to detail: the backend compares
+ * the bucket the chart grouped by (same grain, Hours offset and Time Column)
+ * and, unlike a TEMPORAL_RANGE, never replaces it with a query time range, so
+ * the drill keeps every other filter of the chart as the chart applied it.
+ * Without a grain it falls back to an exact match on the timestamp.
+ */
+export const getTemporalBucketDrillByFilter = (
+ col: QueryFormColumn,
+ value: unknown,
+ grain?: TimeGranularity,
+ formattedVal?: string,
+): BinaryQueryObjectFilterClause | undefined => {
+ const bucket = parseTemporalValue(value);
+ if (!col || !bucket) {
+ return undefined;
+ }
+ return grain
+ ? { col, op: '==', val: bucket.getTime(), grain, formattedVal }
+ : { col, op: '==', val: formatNaiveDateTime(bucket), formattedVal };
+};
+
+/** Resolve a markerless line/area click to a real bucket in that series. */
+export const getTimeAxisDrillByValue = (
+ data: unknown,
+ series: unknown,
+ coordinate: unknown,
+ valueIndex = 0,
+ categorical = false,
+): unknown => {
+ if (Array.isArray(data)) {
+ return data[valueIndex];
+ }
+ if (typeof data === 'number' || typeof data === 'string') {
+ return data;
+ }
+ const axisValue = Array.isArray(coordinate)
+ ? coordinate[valueIndex]
+ : coordinate;
+ if (
+ typeof axisValue !== 'number' ||
+ !Number.isFinite(axisValue) ||
+ typeof series !== 'object' ||
+ series === null ||
+ !('data' in series) ||
+ !Array.isArray(series.data)
+ ) {
+ return undefined;
+ }
+ if (categorical) {
+ const point: unknown = series.data[Math.round(axisValue)];
Review Comment:
Fixed in 195a3a38834842260fe7f03cbb622c6606d402ab. I now resolve the
category index through ECharts' shared axis domain, then look up that
category's point in the clicked series. An absent point produces no bucket
instead of selecting the next month. The axis model supplies auto-collected
categories, which are not present in getOption().
Regression test: `Mixed categorical markerless Drill by uses shared axis
categories for a sparse query B`
It checks February, March and absent January. It fails with the original
source and passes here. The horizontal/vertical helper regression also fails
before and passes after; a real ECharts SSR test checks auto-collected
categories in both orientations. All 481 scoped Jest tests pass.
Siblings checked:
-
superset-frontend/plugins/plugin-chart-echarts/src/utils/xAxisDrillByFilter.ts:213
— fixed in 195a3a38834842260fe7f03cbb622c6606d402ab
-
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/EchartsMixedTimeseries.tsx:239
— fixed in 195a3a38834842260fe7f03cbb622c6606d402ab
-
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:700
— fixed in 195a3a38834842260fe7f03cbb622c6606d402ab
-
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:253
— not affected: percentage-baseline dragging does not call the drill helper or
index a sparse series by axis index.
-
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:761
— not affected: cross-filter handlers read the event's value, not a markerless
category index.
- superset-frontend/src/components/Chart/DrillBy/DrillBySubmenu.tsx:156 —
not affected: consumes the resolved filter and performs no pixel or
series-index lookup.
--
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]
