You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "Copilot (via GitHub)" <gi...@apache.org> on 2026/03/11 21:05:43 UTC
Re: [PR] fix: Drill-to-detail in TableChart not providing the correct dates b… [superset]
Copilot commented on code in PR #36318:
URL: https://github.com/apache/superset/pull/36318#discussion_r2920998011
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -306,6 +307,61 @@ function SelectPageSize({
const getNoResultsMessage = (filter: string) =>
filter ? t('No matching records found') : t('No records found');
+//Calculates the end time based on the granularity
+function getEndTimeFromGranularity(
+ startTime: Date,
+ granularity?: TimeGranularity,
+): Date {
Review Comment:
`getEndTimeFromGranularity` reimplements time-bucket boundary logic that already exists in `@superset-ui/core` (`createTimeRangeFromGranularity`). Reusing the shared helper (and converting its inclusive end to an exclusive boundary if needed) would avoid drift and ensure consistent handling for edge cases like week-ending grains and DST transitions.
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -556,15 +612,38 @@ export default function TableChart<D extends DataRecord = DataRecord>(
const drillToDetailFilters: BinaryQueryObjectFilterClause[] = [];
filteredColumnsMeta.forEach(col => {
if (!col.isMetric) {
- let dataRecordValue = value[col.key];
- dataRecordValue = extractTextFromHTML(dataRecordValue);
-
- drillToDetailFilters.push({
- col: col.key,
- op: '==',
- val: dataRecordValue as string | number | boolean,
- formattedVal: formatColumnValue(col, dataRecordValue)[1],
- });
+ const dataRecordValue = value[col.key];
+
+ // Handle temporal columns differently to support time ranges
+ if (col.dataType === GenericDataType.Temporal && timeGrain) {
+ // Make sure the value is a Date
+ const startTime =
+ dataRecordValue instanceof Date
+ ? dataRecordValue
+ : new Date(dataRecordValue as string | number);
+
+ // Calculate the end time based on the granularity
+ const endTime = getEndTimeFromGranularity(startTime, timeGrain);
+
+ const timeRangeValue = `${startTime.toISOString()} : ${endTime.toISOString()}`;
+
+ drillToDetailFilters.push({
+ col: col.key,
+ op: 'TEMPORAL_RANGE',
+ val: timeRangeValue,
+ grain: timeGrain,
+ formattedVal: formatColumnValue(col, dataRecordValue)[1],
+ });
Review Comment:
This change introduces new drill-to-detail behavior for temporal columns (switching from `==` to `TEMPORAL_RANGE` with computed bounds). There doesn’t appear to be coverage for the context-menu filter payload in the existing table chart tests; adding a unit test that asserts the generated filter(s) for at least `MONTH` and a `WEEK_ENDING_*` grain would prevent regressions and validate the range computation.
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -556,15 +612,38 @@ export default function TableChart<D extends DataRecord = DataRecord>(
const drillToDetailFilters: BinaryQueryObjectFilterClause[] = [];
filteredColumnsMeta.forEach(col => {
if (!col.isMetric) {
- let dataRecordValue = value[col.key];
- dataRecordValue = extractTextFromHTML(dataRecordValue);
-
- drillToDetailFilters.push({
- col: col.key,
- op: '==',
- val: dataRecordValue as string | number | boolean,
- formattedVal: formatColumnValue(col, dataRecordValue)[1],
- });
+ const dataRecordValue = value[col.key];
+
+ // Handle temporal columns differently to support time ranges
+ if (col.dataType === GenericDataType.Temporal && timeGrain) {
+ // Make sure the value is a Date
+ const startTime =
+ dataRecordValue instanceof Date
+ ? dataRecordValue
+ : new Date(dataRecordValue as string | number);
+
+ // Calculate the end time based on the granularity
+ const endTime = getEndTimeFromGranularity(startTime, timeGrain);
+
+ const timeRangeValue = `${startTime.toISOString()} : ${endTime.toISOString()}`;
+
+ drillToDetailFilters.push({
+ col: col.key,
+ op: 'TEMPORAL_RANGE',
+ val: timeRangeValue,
+ grain: timeGrain,
+ formattedVal: formatColumnValue(col, dataRecordValue)[1],
+ });
Review Comment:
`dataRecordValue` can be `null` for temporal columns (see `DataRecordValue` type). In that case `new Date(dataRecordValue as string | number)` becomes `1970-01-01...`, producing an incorrect `TEMPORAL_RANGE` filter instead of filtering for NULL timestamps. Consider handling `null` explicitly (eg emit an `IS NULL` filter, or fall back to `== null`).
```suggestion
if (dataRecordValue == null) {
// For null temporal values, filter explicitly for NULL instead of a fake 1970-01-01 range
drillToDetailFilters.push({
col: col.key,
op: 'IS NULL',
val: null,
});
} else {
// Make sure the value is a Date
const startTime =
dataRecordValue instanceof Date
? dataRecordValue
: new Date(dataRecordValue as string | number);
// Calculate the end time based on the granularity
const endTime = getEndTimeFromGranularity(startTime, timeGrain);
const timeRangeValue = `${startTime.toISOString()} : ${endTime.toISOString()}`;
drillToDetailFilters.push({
col: col.key,
op: 'TEMPORAL_RANGE',
val: timeRangeValue,
grain: timeGrain,
formattedVal: formatColumnValue(col, dataRecordValue)[1],
});
}
```
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -306,6 +307,61 @@ function SelectPageSize({
const getNoResultsMessage = (filter: string) =>
filter ? t('No matching records found') : t('No records found');
+//Calculates the end time based on the granularity
+function getEndTimeFromGranularity(
+ startTime: Date,
+ granularity?: TimeGranularity,
+): Date {
+ if (!granularity) {
+ return startTime;
+ }
+
+ const time = startTime.getTime();
+ const date = startTime.getUTCDate();
+ const month = startTime.getUTCMonth();
+ const year = startTime.getUTCFullYear();
+
+ // Constants for time calculations
+ 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 new Date(time + MS_IN_SECOND);
+ case TimeGranularity.MINUTE:
+ return new Date(time + MS_IN_MINUTE);
+ case TimeGranularity.FIVE_MINUTES:
+ return new Date(time + MS_IN_MINUTE * 5);
+ case TimeGranularity.TEN_MINUTES:
+ return new Date(time + MS_IN_MINUTE * 10);
+ case TimeGranularity.FIFTEEN_MINUTES:
+ return new Date(time + MS_IN_MINUTE * 15);
+ case TimeGranularity.THIRTY_MINUTES:
+ return new Date(time + MS_IN_MINUTE * 30);
+ case TimeGranularity.HOUR:
+ return new Date(time + MS_IN_HOUR);
+ case TimeGranularity.DAY:
+ case TimeGranularity.DATE:
+ return new Date(Date.UTC(year, month, date + 1));
+ case TimeGranularity.WEEK:
+ case TimeGranularity.WEEK_STARTING_SUNDAY:
+ case TimeGranularity.WEEK_STARTING_MONDAY:
+ return new Date(Date.UTC(year, month, date + 7));
+ case TimeGranularity.WEEK_ENDING_SATURDAY:
+ case TimeGranularity.WEEK_ENDING_SUNDAY:
+ return new Date(Date.UTC(year, month, date + 1));
Review Comment:
For `WEEK_ENDING_SATURDAY`/`WEEK_ENDING_SUNDAY` the generated temporal range is incorrect: `startTime` is treated as the range start, but for these granularities the timestamp returned by the DB represents the *end* of the week bucket. This currently produces only a 1-day range (end date -> end date+1), so drill-to-detail will still miss most rows for those weeks. Adjust the start/end calculation for week-ending grains (start should be 6 days earlier; end should be the next day boundary), or derive both bounds from a shared time-range utility.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org