aminghadersohi commented on code in PR #45612: URL: https://github.com/apache/superset/pull/45612#discussion_r4395272049
########## 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: Correction to my earlier file/test references: `drillByTimeRange.ts` and its tests are historical references to deleted files. At 56240a49dc2ca1962a95f274f14850afff2b8a74, Drill by keeps the chart's time settings and appends a grained equality bucket in `DrillByModal.tsx`; current coverage includes `Sunburst Drill by keeps the %s time range beside its grained bucket`. This updates my description, not the behavior. https://github.com/apache/superset/blob/56240a49dc2ca1962a95f274f14850afff2b8a74/superset-frontend/src/components/Chart/DrillBy/DrillByModal.tsx#L354 https://github.com/apache/superset/blob/56240a49dc2ca1962a95f274f14850afff2b8a74/superset-frontend/src/components/Chart/DrillBy/DrillByTemporalFilters.test.tsx#L191 ########## 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: Correction to my reply above: `getDrillByOuterTimeRange`, `applyDrillByTimeRange` and their deleted tests are historical references, not the implementation at 56240a49dc2ca1962a95f274f14850afff2b8a74. The bucket is a grained equality from `getTemporalBucketDrillByFilter`, alongside the chart's existing filters; `test_drill_by_returns_the_rows_of_the_clicked_bucket` covers independent date filters and Time Column overrides. No new behavior change is being claimed. https://github.com/apache/superset/blob/56240a49dc2ca1962a95f274f14850afff2b8a74/superset-frontend/plugins/plugin-chart-echarts/src/utils/xAxisDrillByFilter.ts#L197 https://github.com/apache/superset/blob/56240a49dc2ca1962a95f274f14850afff2b8a74/tests/unit_tests/common/test_drill_by_time_column.py#L247 ########## superset-frontend/src/explore/components/controls/FilterControl/AdhocFilter/index.ts: ########## @@ -140,6 +146,11 @@ export default class AdhocFilter { operator: this.operator, operatorId: this.operatorId, comparator: this.comparator, + // A grain belongs to its subject's time buckets. + timeGrain: + nextFields.subject === undefined || nextFields.subject === this.subject + ? this.timeGrain + : undefined, Review Comment: Correction to my earlier reply: changing a bucket filter's value drops its grain; only unchanged predicates and metadata-only edits retain it. At 56240a49dc2ca1962a95f274f14850afff2b8a74, `duplicateWith` checks the comparator too, and `Explore filter duplicates preserve grains but bucket edits clear them` asserts this. This is a wording update, not a new behavior change. https://github.com/apache/superset/blob/56240a49dc2ca1962a95f274f14850afff2b8a74/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilter/index.ts#L139 https://github.com/apache/superset/blob/56240a49dc2ca1962a95f274f14850afff2b8a74/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilter/AdhocFilter.test.ts#L436 -- 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]
