aminghadersohi commented on code in PR #45612: URL: https://github.com/apache/superset/pull/45612#discussion_r4364245125
########## superset-frontend/src/components/Chart/DrillBy/drillByTimeRange.ts: ########## @@ -0,0 +1,209 @@ +/** + * 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 rison from 'rison'; +import { + BinaryQueryObjectFilterClause, + ContextMenuFilters, + JsonObject, + NO_TIME_RANGE, + QueryFormData, + SupersetClient, + buildQueryObject, + ensureIsArray, + getColumnLabel, +} from '@superset-ui/core'; + +type DrillByConfig = ContextMenuFilters['drillBy']; + +const TIME_RANGE_SEPARATOR = ' : '; + +/** + * Return the innermost temporal bucket filter a Drill by was started from, + * i.e. the last TEMPORAL_RANGE drill filter. Each further drill narrows the + * previous one, so the last bucket is the one to query. + */ +export const getDrillByTimeFilter = ( + configs: DrillByConfig[], +): BinaryQueryObjectFilterClause | undefined => { + let timeFilter: BinaryQueryObjectFilterClause | undefined; + configs.forEach(config => { + ensureIsArray(config?.filters).forEach(filter => { + if ( + filter.op === 'TEMPORAL_RANGE' && + typeof filter.val === 'string' && + filter.val + ) { + timeFilter = filter; + } + }); + }); + return timeFilter; +}; + +/** + * Return the time range of the innermost temporal bucket a Drill by was + * started from. + */ +export const getDrillByTimeRange = ( + configs: DrillByConfig[], +): string | undefined => + getDrillByTimeFilter(configs)?.val as string | undefined; + +/** + * Return the time range the chart displayed before the drill, using the same + * precedence as the chart's own query: a dashboard override, a legacy + * dashboard time filter or the chart's time_range, and otherwise the chart's + * TEMPORAL_RANGE filter on the drilled column. Returns undefined when the + * chart is not limited in time. + */ +export const getDrillByOuterTimeRange = ( + formData: QueryFormData, + column?: BinaryQueryObjectFilterClause['col'], +): string | undefined => { + let timeRange: unknown; + try { + const queryObject = buildQueryObject(formData); + const columnLabel = + column === undefined ? undefined : getColumnLabel(column); + timeRange = + queryObject.time_range ?? + ensureIsArray(queryObject.filters).find( + (filter): filter is BinaryQueryObjectFilterClause => + filter.op === 'TEMPORAL_RANGE' && + (columnLabel === undefined || + getColumnLabel(filter.col) === columnLabel), + )?.val; + } catch { + return undefined; + } + return typeof timeRange === 'string' && + timeRange.trim() && + timeRange !== NO_TIME_RANGE + ? timeRange + : undefined; +}; + +const TIMEZONE_SUFFIX_RE = /(Z|[+-]\d{2}:?\d{2})$/; + +interface TimeBound { + value: string; + time: number; +} + +const toBound = (value: string | undefined): TimeBound | undefined => { + if (!value) { + return undefined; + } + const time = new Date( + TIMEZONE_SUFFIX_RE.test(value) ? value : `${value}Z`, + ).getTime(); + return Number.isNaN(time) ? undefined : { value, time }; +}; + +/** + * Intersect a bucket time range ("since : until", naive ISO datetimes) with + * resolved outer bounds. An empty outer bound is unbounded on that side. A + * bucket that does not overlap the outer range yields an empty range. + */ +export const intersectTimeRanges = ( + bucket: string, + outerSince: string | undefined, + outerUntil: string | undefined, +): string => { + const [bucketSince, bucketUntil] = bucket.split(TIME_RANGE_SEPARATOR); + let since = toBound(bucketSince); + let until = toBound(bucketUntil); + if (!since || !until) { + return bucket; + } + const outerStart = toBound(outerSince); + const outerEnd = toBound(outerUntil); + if (outerStart && outerStart.time > since.time) { + since = outerStart; + } + if (outerEnd && outerEnd.time < until.time) { + until = outerEnd; + } + return since.time < until.time + ? `${since.value}${TIME_RANGE_SEPARATOR}${until.value}` + : `${since.value}${TIME_RANGE_SEPARATOR}${since.value}`; +}; + +/** + * Resolve the outer time range (which may be relative, e.g. "Last year") and + * return the part of the bucket that lies inside it, so a bucket that is + * only partly inside the chart's or dashboard's range does not return rows + * the chart never displayed. Falls back to the bucket if the range cannot be + * resolved. + */ +export const fetchDrillByTimeRange = async ( + bucket: string, + outerTimeRange: string, +): Promise<string> => { + try { + const { json } = await SupersetClient.get({ + endpoint: `/api/v1/time_range/?q=${rison.encode_uri(outerTimeRange)}`, + }); + const result = ensureIsArray(json?.result)[0]; + if (!result) { + return bucket; + } + return intersectTimeRanges(bucket, result.since, result.until); + } catch { + return bucket; + } +}; + +/** + * Make a drilled query keep the clicked temporal bucket. + * + * The backend replaces the value of every TEMPORAL_RANGE filter with the + * query's time_range, so a bucket sent only as a TEMPORAL_RANGE filter is + * widened to the chart's (or dashboard's) whole time range. Carrying the + * bucket as the query's time_range keeps it. A dashboard time range arrives + * through extra_form_data, which overrides time_range, so it is replaced + * there as well. Legacy dashboard filters can also override the range + * through extra_filters, so their reserved __time_range value is replaced. + * + * TEMPORAL_RANGE is kept, rather than a >= / < pair, because only the + * TEMPORAL_RANGE path applies the dataset's timezone or Hours offset to the + * bounds. + */ +export const applyDrillByTimeRange = <T extends JsonObject>( + formData: T, + timeRange: string | undefined, +): T => { + if (!timeRange) { + return formData; + } + const extraFormData = formData.extra_form_data as JsonObject | undefined; + const extraFilters = formData.extra_filters as QueryFormData['extra_filters']; Review Comment: My previous reply overstated two things. "Every temporal bucket retains its clipped range" was wrong in two setups: - A chart with a query-wide range (saved, dashboard or legacy), a `ship_date` filter and no date filter on the `order_date` x-axis. The chart applies that range only to `ship_date`, but the drill still trimmed the clicked `order_date` bucket to it (Dec 2003 bar 3 rows, drill 0). The test `nested temporal drills retain every outer bucket clip with independent date filters` asserted that trim. - A chart with a time column (saved `granularity_sqla`, dashboard Time Column or legacy `__time_col`), an `order_date` filter, a `ship_date` filter and a range. The backend drops every x-axis filter, the bucket included, and filters the time column by the first one, which the drill had set to the whole range (Jan 2004 bar 2 rows, drill 4). Fixed in a6eaa9133e7479d9eb9e5e723a45b5a56aab72ac: `getDrillByOuterTimeRange` follows the backend's per-column rules instead of always using the query-wide range. A range limits a column only through its TEMPORAL_RANGE filter; with a time column it limits the x-axis (or, without an x-axis, the time column) by the time range, else the first x-axis filter, else the first date filter. With a time column, `applyDrillByTimeRange` gives every x-axis filter the intersected bucket, so the filter the backend reads carries it. Without an x-axis, the time column filter goes first, and a filter the chart dropped stays dropped. (live: gh pr view 45612 -R apache/superset @ 2026-10-23) Regression tests: `getDrillByOuterTimeRange ignores a query-wide range on a column without a date filter` and `with a time column every x-axis filter carries the intersected buckets` in superset-frontend/src/components/Chart/DrillBy/drillByTimeRange.test.ts, `a time column without an x-axis filters the drill by the bucket, not an independent date filter` in superset-frontend/src/components/Chart/DrillBy/DrillByModal.test.tsx. I also changed the nested test above to the x-axis setup, renamed it `nested temporal drills keep an unfiltered x-axis bucket whole beside independent date filters: %s`, and added `a %s time column filters the drill by the clipped bucket beside an independent date filter` (saved, dashboard, legacy). With drillByTimeRange.ts from df626c43a4e33b496ec911c53399d2faca7dd9a2, 17 DrillBy Jest tests fail; all 108 pass at a6eaa9133e7479d9eb9e5e723a45b5a56aab72ac. `test_time_column_filters_the_drill_by_the_first_x_axis_filter` and `test_query_wide_range_does_not_limit_ an_unfiltered_x_axis` in tests/unit_tests/common/test_drill_by_time_column.py run both drill queries through the real query object pipeline on SQLite and compare rows with the chart's bar; they pin the backend behavior the frontend relies on and also pass before this commit. Matrix: the real Drill by modal and the real Timeseries buildQuery, then the real QueryContextFactory processing and SQL on SQLite with 9 synthetic orders. 28 setups (no range, saved, `No filter`, dashboard and legacy ranges; with and without an `order_date` filter; saved, dashboard and legacy time columns, including one on `ship_date`; legacy charts without an x-axis) × 4 month buckets, each with a nested drill (same bucket plus status, a `ship_date` bucket, or a narrower `order_date` range): 336 drills. Bucket count vs drill count: 41 of 112 mismatched at df626c43a4e33b496ec911c53399d2faca7dd9a2 (nested: 0, 3 and 49 more), 0 at a6eaa9133e7479d9eb9e5e723a45b5a56aab72ac, also 0 with TZ=America/New_York. Siblings checked: - superset-frontend/src/components/Chart/DrillBy/drillByTimeRange.ts — fixed in a6eaa9133e7479d9eb9e5e723a45b5a56aab72ac - superset-frontend/src/components/Chart/DrillBy/DrillByModal.tsx — not affected: it passes the original chart's form data and each drill's filter field to the two helpers, unchanged - superset-frontend/src/components/Chart/DrillBy/useResultsTableView.tsx — not affected: CSV and XLSX export the same drilled form data; `temporal drill retains the effective independent date range in query CSV and Explore: %j` asserts the export filters equal the drill query - superset-frontend/src/explore/exploreUtils/formData.ts — not affected: Edit chart posts the same drilled form data, asserted by the same test - superset-frontend/src/explore/actions/saveModalActions.ts — not affected: a chart saved from the drill stores that form data - superset/commands/explore/get.py — not affected: the merge keeps adhoc filters in order, pinned by test_drilled_temporal_ranges_survive_explore_filter_merge - superset/mcp_service/chart/tool/get_chart_data.py — not affected: a form_data_key replays the cached Explore form data - superset-frontend/src/components/Chart/DrillDetail/utils.ts — not affected: Drill to detail sends equality filters with a grain and never calls these helpers - superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/EchartsMixedTimeseries.tsx — not affected: the time column model is built per filter field, so query B reads adhoc_filters_b Remaining: charts with a custom SQL x-axis, where the backend infers the dataset's main datetime column as the time column, are not modelled or in the matrix. Live UI and SQL acceptance on a running Superset was not run. -- 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]
