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: [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]
