michael-s-molina commented on code in PR #42088: URL: https://github.com/apache/superset/pull/42088#discussion_r3624260212
########## superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getTimeRangeFromGranularity.ts: ########## @@ -0,0 +1,80 @@ +/** + * 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 { TimeGranularity } from '@superset-ui/core'; + +/** + * Calculates the inclusive/exclusive temporal range for a bucket. + * standard SQL range pattern: [start, end) + */ +export default function getTimeRangeFromGranularity( + startTime: Date, + granularity: TimeGranularity, +): [Date, Date] { + const time = startTime.getTime(); + const date = startTime.getUTCDate(); + const month = startTime.getUTCMonth(); + const year = startTime.getUTCFullYear(); + + // Constants + const MS_IN_SECOND = 1000; + const MS_IN_MINUTE = 60 * MS_IN_SECOND; + const MS_IN_HOUR = 60 * MS_IN_MINUTE; + + switch (granularity) { + case TimeGranularity.SECOND: + return [startTime, new Date(time + MS_IN_SECOND)]; + case TimeGranularity.MINUTE: + return [startTime, new Date(time + MS_IN_MINUTE)]; + case TimeGranularity.FIVE_MINUTES: + return [startTime, new Date(time + MS_IN_MINUTE * 5)]; + case TimeGranularity.TEN_MINUTES: + return [startTime, new Date(time + MS_IN_MINUTE * 10)]; + case TimeGranularity.FIFTEEN_MINUTES: + return [startTime, new Date(time + MS_IN_MINUTE * 15)]; + case TimeGranularity.THIRTY_MINUTES: + return [startTime, new Date(time + MS_IN_MINUTE * 30)]; + case TimeGranularity.HOUR: + return [startTime, new Date(time + MS_IN_HOUR)]; + case TimeGranularity.DAY: + case TimeGranularity.DATE: + return [startTime, new Date(Date.UTC(year, month, date + 1))]; + case TimeGranularity.WEEK: + case TimeGranularity.WEEK_STARTING_SUNDAY: + case TimeGranularity.WEEK_STARTING_MONDAY: + return [startTime, new Date(Date.UTC(year, month, date + 7))]; + case TimeGranularity.WEEK_ENDING_SATURDAY: + case TimeGranularity.WEEK_ENDING_SUNDAY: + // Week-ending buckets are labeled by the bucket's final day. + return [ + new Date(Date.UTC(year, month, date - 6)), + new Date(Date.UTC(year, month, date + 1)), + ]; + case TimeGranularity.MONTH: + return [startTime, new Date(Date.UTC(year, month + 1, 1))]; + case TimeGranularity.QUARTER: + return [ + startTime, + new Date(Date.UTC(year, Math.floor(month / 3) * 3 + 3, 1)), + ]; + case TimeGranularity.YEAR: + return [startTime, new Date(Date.UTC(year + 1, 0, 1))]; Review Comment: Looked into this closely — `getTimeRangeFromGranularity.ts` is a byte-identical copy of the already-shipped V1 function (`plugin-chart-table/src/TableChart.tsx:330-385`), and in practice `startTime` always comes from a backend `GROUP BY time_grain_sqla` column, which is already truncated to the bucket boundary before it reaches the frontend — the noon-value scenario in the repro isn't realistic input for a bucketed column. There's also an existing test (`drillToDetail.test.tsx`, "right-clicking a temporal cell with a time grain emits a TEMPORAL_RANGE filter") that explicitly asserts this exact non-truncating behavior as correct, matching V1. Given that, and that "fixing" this would diverge V2 from V1's long-shipped, working behavior without evidence it's actually broken in practice, leaving as-is. ########## superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx: ########## @@ -368,75 +395,200 @@ export default function TableChart<D extends DataRecord = DataRecord>( [emitCrossFilters, setDataMask, timeGrain, timestampFormatter], ); + const drillColumns = isUsingTimeComparison + ? (filteredColumns as InputColumn[]) + : (columns as InputColumn[]); + + const handleContextMenu = useCallback( + (event: CellContextMenuEvent) => { + if (!onContextMenu || isRawRecords || !event.column || !event.data) { + return; + } + const nativeEvent = event.event as MouseEvent | null | undefined; + if (!nativeEvent) return; + nativeEvent.preventDefault(); + nativeEvent.stopPropagation(); + + const rowData = event.data as Record<string, DataRecordValue>; + const key = event.column.getColId(); + const cellValue = event.value as DataRecordValue; + const colDef = event.column.getColDef(); + const isMetric = Boolean( + colDef.context?.isMetric || colDef.context?.isPercentMetric, + ); + + const drillToDetailFilters: BinaryQueryObjectFilterClause[] = []; + drillColumns.forEach(col => { + if (col.isMetric || col.isPercentMetric) return; + const dataRecordValue = rowData[col.key]; + + if ( + dataRecordValue == null || + (dataRecordValue instanceof DateWithFormatter && + dataRecordValue.input == null) + ) { + drillToDetailFilters.push({ + col: col.key, + op: 'IS NULL' as any, + val: null, + }); + } else if (col.dataType === GenericDataType.Temporal && timeGrain) { + const startTime = + dataRecordValue instanceof Date + ? dataRecordValue + : new Date(dataRecordValue as string | number); + + if (Number.isNaN(startTime.getTime())) { Review Comment: Checked this one carefully and I believe it's a false positive: `DateWithFormatter extends Date` (see `utils/DateWithFormatter.ts:29`), so `dataRecordValue instanceof Date` is `true` for a `DateWithFormatter` instance — verified this directly (`new DateWithFormatter(...) instanceof Date === true`). The existing ternary (`dataRecordValue instanceof Date ? dataRecordValue : new Date(...)`) already returns the `DateWithFormatter` unchanged without ever reaching `new Date(object)`, so there's no unwrap needed. V1's identical code (`plugin-chart-table/src/TableChart.tsx:679`) uses the exact same check without unwrapping either. Leaving as-is. ########## superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx: ########## @@ -368,75 +395,200 @@ export default function TableChart<D extends DataRecord = DataRecord>( [emitCrossFilters, setDataMask, timeGrain, timestampFormatter], ); + const drillColumns = isUsingTimeComparison + ? (filteredColumns as InputColumn[]) + : (columns as InputColumn[]); + + const handleContextMenu = useCallback( + (event: CellContextMenuEvent) => { + if (!onContextMenu || isRawRecords || !event.column || !event.data) { + return; + } + const nativeEvent = event.event as MouseEvent | null | undefined; + if (!nativeEvent) return; + nativeEvent.preventDefault(); + nativeEvent.stopPropagation(); + + const rowData = event.data as Record<string, DataRecordValue>; + const key = event.column.getColId(); + const cellValue = event.value as DataRecordValue; + const colDef = event.column.getColDef(); + const isMetric = Boolean( + colDef.context?.isMetric || colDef.context?.isPercentMetric, + ); + + const drillToDetailFilters: BinaryQueryObjectFilterClause[] = []; + drillColumns.forEach(col => { + if (col.isMetric || col.isPercentMetric) return; + const dataRecordValue = rowData[col.key]; + + if ( + dataRecordValue == null || + (dataRecordValue instanceof DateWithFormatter && + dataRecordValue.input == null) + ) { + drillToDetailFilters.push({ + col: col.key, + op: 'IS NULL' as any, + val: null, + }); + } else if (col.dataType === GenericDataType.Temporal && timeGrain) { + const startTime = + dataRecordValue instanceof Date + ? dataRecordValue + : new Date(dataRecordValue as string | number); + + if (Number.isNaN(startTime.getTime())) { + // Malformed temporal value: fall back to an equality filter + // instead of building a TEMPORAL_RANGE, since toISOString() + // throws on an Invalid Date and would crash the context menu. + const sanitizedValue = extractTextFromHTML(dataRecordValue); + drillToDetailFilters.push({ + col: col.key, + op: '==', + val: sanitizedValue as string | number | boolean, + formattedVal: formatColumnValue(col, sanitizedValue)[1], + }); + } else { + const [rangeStartTime, rangeEndTime] = getTimeRangeFromGranularity( + startTime, + timeGrain, + ); + const timeRangeValue = `${rangeStartTime.toISOString()} : ${rangeEndTime.toISOString()}`; + + drillToDetailFilters.push({ + col: col.key, + op: 'TEMPORAL_RANGE', + val: timeRangeValue, + grain: timeGrain, + formattedVal: formatColumnValue(col, dataRecordValue)[1], + }); + } + } else { + const sanitizedValue = extractTextFromHTML(dataRecordValue); + drillToDetailFilters.push({ + col: col.key, + op: '==', + val: sanitizedValue as string | number | boolean, + formattedVal: formatColumnValue(col, sanitizedValue)[1], + }); + } + }); + + onContextMenu(nativeEvent.clientX, nativeEvent.clientY, { + drillToDetail: drillToDetailFilters, + crossFilter: isMetric + ? undefined + : getCrossFilterDataMask({ + key, + value: cellValue, + filters, + timeGrain, + isActiveFilterValue, + timestampFormatter, + }), + drillBy: isMetric + ? undefined + : { + filters: [ + { + col: key, + op: (cellValue == null || + (cellValue instanceof DateWithFormatter && + cellValue.input == null) + ? 'IS NULL' + : '==') as any, + val: extractTextFromHTML(cellValue), + }, + ], Review Comment: Confirmed and fixed in 4979813 — the null branch now explicitly sets `val: null` alongside `op: 'IS NULL'` instead of falling through to `extractTextFromHTML(cellValue)` (which returns the `DateWithFormatter` wrapper object itself, not `null`, when the null value is a wrapped temporal). Added a regression test that right-clicks a null cell and asserts `drillBy` carries `val: null`. ########## superset-frontend/plugins/plugin-chart-ag-grid-table/src/controlPanel.tsx: ########## @@ -717,24 +742,68 @@ const config: ControlPanelConfig = { : []; const chartStatus = chart?.chartStatus; + // Normalize legacy `toAllRow`/`toTextColor` flags saved before + // `columnFormatting`/`objectFormatting` existed, so "entire row" + // formatters set under the old schema keep working. + const value = _?.value ?? []; + if (value && Array.isArray(value)) { + value.forEach( + (item: ConditionalFormattingConfig, index, array) => { + if ( + item.colorScheme && + !['Green', 'Red'].includes(item.colorScheme) + ) { + if (item.columnFormatting === undefined) { + // eslint-disable-next-line no-param-reassign + array[index] = { + ...item, + ...(item.toTextColor === true && { + objectFormatting: ObjectFormattingEnum.TEXT_COLOR, + }), + ...(item.toAllRow === true && { + columnFormatting: ObjectFormattingEnum.ENTIRE_ROW, + }), + }; + } + } + }, + ); + } const { colnames, coltypes } = chart?.queriesResponse?.[0] ?? {}; - const numericColumns = - Array.isArray(colnames) && Array.isArray(coltypes) - ? colnames - .filter( - (colname: string, index: number) => - coltypes[index] === GenericDataType.Numeric, - ) - .map((colname: string) => ({ - value: colname, - label: Array.isArray(verboseMap) - ? colname - : (verboseMap[colname] ?? colname), - dataType: - colnames && coltypes[colnames?.indexOf(colname)], - })) - : []; + const hasColumns = + Array.isArray(colnames) && Array.isArray(coltypes); + const allColumns = hasColumns + ? [ + { + value: ObjectFormattingEnum.ENTIRE_ROW, + label: t('entire row'), + dataType: GenericDataType.String, + }, + ...colnames.map((colname: string, index: number) => ({ + value: colname, + label: Array.isArray(verboseMap) + ? colname + : (verboseMap[colname] ?? colname), + dataType: coltypes[index], + })), + ] + : []; + const numericColumns = hasColumns + ? colnames + .filter( + (colname: string, index: number) => + coltypes[index] === GenericDataType.Numeric, + ) + .map((colname: string) => ({ + value: colname, + label: Array.isArray(verboseMap) + ? colname + : (verboseMap[colname] ?? colname), + dataType: + colnames && coltypes[colnames?.indexOf(colname)], + })) Review Comment: Confirmed and fixed in 4979813 — every entry in `numericColumns` already passed the `coltypes[index] === GenericDataType.Numeric` filter, so its dataType is always `Numeric`; the `.indexOf(colname)` re-lookup was not only redundant but unsafe on duplicate column names (as you note). Replaced it with the already-known constant and added a regression test with a duplicated column name to lock this in. ########## superset/migrations/shared/migrate_viz/base.py: ########## @@ -148,6 +150,14 @@ def _migrate_temporal_filter(self, rv_data: dict[str, Any]) -> None: def upgrade_slice(cls, slc: Slice) -> None: try: clz = cls(slc.params) + # Some charts don't carry a "datasource" key in params — outside + # of migrations, callers always read it via Slice.form_data, + # which injects "datasource" from the datasource_id/ + # datasource_type columns on every access. _build_query() (and + # anything else touching self.data) needs that same key, so + # synthesize it here the same way for the charts missing it. + if "datasource" not in clz.data and slc.datasource_id is not None: + clz.data["datasource"] = f"{slc.datasource_id}__{slc.datasource_type}" Review Comment: Checked this against the code it's explicitly mirroring: `Slice.form_data` (`superset/models/slice.py:318`) — the property every non-migration caller uses — builds this exact same `f"{self.datasource_id}__{self.datasource_type}"` string unconditionally, with no check on either being `None`. So a slice with `datasource_id` set but `datasource_type` missing already gets a `"123__None"`-shaped token at runtime today, outside of migrations. Adding a stricter guard here would make the migration's synthesized value diverge from what `Slice.form_data` already produces for the exact same row, which seems like it'd be more confusing, not less. Leaving this matching the existing production behavior it's explicitly designed to mirror. ########## superset/migrations/shared/migrate_viz/processors.py: ########## @@ -642,3 +644,254 @@ def process(base_query_object: dict[str, Any]) -> list[dict[str, Any]]: return [result] return build_query_context(self.data, process) + + +def _get_table_chart_time_offsets(form_data: dict[str, Any]) -> list[Any]: + """ + Resolve time_compare into the list of shifts buildQuery.ts sends as + time_offsets. table charts use a single-select time_compare control + whose choices include the special 'custom'/'inherit' shifts, which + resolve to start_date_offset/'inherit' rather than being used verbatim. + """ + time_compare_shifts = ensure_is_array(form_data.get("time_compare")) + non_custom_or_inherit_shifts = [ + shift for shift in time_compare_shifts if shift not in ("custom", "inherit") + ] + custom_or_inherit_shifts = [ + shift for shift in time_compare_shifts if shift in ("custom", "inherit") + ] + + time_offsets: list[Any] = list(non_custom_or_inherit_shifts) + if "custom" in custom_or_inherit_shifts: + time_offsets.append(form_data.get("start_date_offset")) + if "inherit" in custom_or_inherit_shifts: + time_offsets.append("inherit") + + # Dashboard filter override - allows dashboard-level time shifts to + # OVERRIDE chart-level time shift settings, mirroring buildQuery.ts. + extra_form_data_time_compare = (form_data.get("extra_form_data") or {}).get( + "time_compare" + ) + if extra_form_data_time_compare: + time_offsets = [extra_form_data_time_compare] Review Comment: Good catch on the defensive gap, though verified it's not reachable through normal usage: `extra_form_data.time_compare` is typed as a single `string` on the frontend (`QueryObjectExtras.time_compare?: string` in `@superset-ui/core`), so `buildQuery.ts`'s identical `timeOffsets = [extra_form_data.time_compare]` never nests either — TypeScript guarantees the wrapped value is a scalar there. Python has no equivalent runtime guarantee on deserialized JSON, so added defensive normalization via `ensure_is_array` anyway in 4979813 (consistent with this PR's "make migrations more resilient" theme) plus a regression test covering an already-list override value. -- 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]
