Copilot commented on code in PR #36901:
URL: https://github.com/apache/superset/pull/36901#discussion_r2673622544
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:
##########
@@ -25,7 +25,7 @@ import {
getColumnLabel,
getNumberFormatter,
LegendState,
- ensureIsArray,
+ ensureIsArray, TimeGranularity,
Review Comment:
The TimeGranularity import is added to the existing import line without
proper formatting. While functional, this makes the import less readable.
Consider adding TimeGranularity on a new line or ensuring consistent spacing
around the comma separator.
##########
superset-frontend/src/components/Chart/DrillBy/DrillByModal.tsx:
##########
@@ -366,12 +366,35 @@ export default function DrillByModal({
updatedFormData.slice_id = 0;
delete updatedFormData.slice_name;
delete updatedFormData.dashboards;
+
+ if (
+ updatedFormData.viz_type === 'echarts_timeseries_bar' &&
+ drillByConfigs?.length
+ ) {
+ const column = drillByConfigs[drillByConfigs.length - 1]?.column;
+ if (column?.column_name) {
+ updatedFormData.x_axis = column.column_name;
+ updatedFormData.groupby = [
+ {
+ sqlExpression: column.column_name,
+ expressionType: 'SQL',
+ optionName: column.column_name + ' (label)',
+ hasCustomLabel: true,
+ label: column.column_name + ' (label)',
+ isColumnReference: true,
+ },
+ ];
Review Comment:
Setting both x_axis and groupby to the same column may lead to duplication
in the query. Based on the buildQuery logic in Timeseries/buildQuery.ts (lines
76-79), columns are constructed from both getXAxisColumn(formData) and groupby
arrays. If the same column appears in both x_axis and groupby, it may result in
duplicate columns in the query, potentially causing incorrect data aggregation
or visualization issues. Consider removing the column from groupby when it's
set as x_axis, or verify that this duplication is intentional and handled
correctly downstream.
```suggestion
// Ensure the x_axis column is not also present in groupby to avoid
// duplicated query columns and potential aggregation issues.
if (updatedFormData.groupby) {
const currentGroupby = ensureIsArray(updatedFormData.groupby);
updatedFormData.groupby = currentGroupby.filter(group =>
typeof group === 'string'
? group !== column.column_name
: group?.column_name !== column.column_name &&
group?.label !== column.column_name &&
group?.sqlExpression !== column.column_name,
);
}
```
##########
superset-frontend/src/components/Chart/DrillBy/DrillByModal.tsx:
##########
@@ -366,12 +366,35 @@ export default function DrillByModal({
updatedFormData.slice_id = 0;
delete updatedFormData.slice_name;
delete updatedFormData.dashboards;
+
+ if (
+ updatedFormData.viz_type === 'echarts_timeseries_bar' &&
+ drillByConfigs?.length
+ ) {
+ const column = drillByConfigs[drillByConfigs.length - 1]?.column;
+ if (column?.column_name) {
+ updatedFormData.x_axis = column.column_name;
+ updatedFormData.groupby = [
+ {
+ sqlExpression: column.column_name,
+ expressionType: 'SQL',
+ optionName: column.column_name + ' (label)',
+ hasCustomLabel: true,
+ label: column.column_name + ' (label)',
+ isColumnReference: true,
+ },
+ ];
+ updatedFormData.xAxisForceCategorical = true;
+ }
+ }
Review Comment:
The new drill-by functionality for echarts_timeseries_bar lacks test
coverage. The changes modify the drilledFormData calculation and add complex
logic for x_axis manipulation, groupby configuration, and xAxisForceCategorical
setting, but no tests verify this behavior. Tests should cover scenarios like
drilling into a bar chart with different column types, ensuring the x_axis is
correctly set, and verifying that the drilled chart renders correctly.
##########
superset-frontend/src/components/Chart/DrillBy/DrillByModal.tsx:
##########
@@ -366,12 +366,35 @@ export default function DrillByModal({
updatedFormData.slice_id = 0;
delete updatedFormData.slice_name;
delete updatedFormData.dashboards;
+
+ if (
+ updatedFormData.viz_type === 'echarts_timeseries_bar' &&
+ drillByConfigs?.length
+ ) {
+ const column = drillByConfigs[drillByConfigs.length - 1]?.column;
+ if (column?.column_name) {
+ updatedFormData.x_axis = column.column_name;
+ updatedFormData.groupby = [
+ {
+ sqlExpression: column.column_name,
+ expressionType: 'SQL',
+ optionName: column.column_name + ' (label)',
+ hasCustomLabel: true,
+ label: column.column_name + ' (label)',
Review Comment:
The label being constructed using string concatenation with " (label)"
suffix appears arbitrary and potentially confusing. The label should either use
the column's verbose_name if available, or just the column_name without
appending " (label)". Additionally, the optionName field should not include the
" (label)" suffix as it represents the actual option identifier.
```suggestion
optionName: column.column_name,
hasCustomLabel: true,
label: column.verbose_name || column.column_name,
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:
##########
@@ -208,18 +209,49 @@ export default function EchartsTimeseries({
...(labelMap[seriesName] ?? []),
];
const groupBy = ensureIsArray(formData.groupby);
+ let xAxisDrillByFilter: BinaryQueryObjectFilterClause | undefined;
+
+ const xAxisColumn =
+ xAxis.label === DTTM_ALIAS
+ ? formData.granularitySqla
+ : xAxis.label;
+
if (data && xAxis.type === AxisType.Time) {
+
+ const timeValue = data[0];
+ const timeValueMs = typeof timeValue === 'number'
+ ? timeValue
+ : new Date(timeValue).getTime();
+ const grainMs =
+ TIMEGRAIN_TO_TIMESTAMP[formData.timeGrainSqla as keyof typeof
TIMEGRAIN_TO_TIMESTAMP] ??
+ TIMEGRAIN_TO_TIMESTAMP[TimeGranularity.DAY];
+
drillToDetailFilters.push({
- col:
- // if the xAxis is '__timestamp', granularity_sqla will be the
column of filter
- xAxis.label === DTTM_ALIAS
- ? formData.granularitySqla
- : xAxis.label,
+ col: xAxisColumn,
grain: formData.timeGrainSqla,
op: '==',
- val: data[0],
- formattedVal: xValueFormatter(data[0]),
+ val: timeValue,
+ formattedVal: xValueFormatter(timeValue),
});
+ if (!Number.isNaN(timeValueMs)) {
+ const startTime = new Date(timeValueMs).toISOString();
+ const endTime = new Date(timeValueMs + grainMs).toISOString();
+ xAxisDrillByFilter = {
+ col: xAxisColumn,
+ op: 'TEMPORAL_RANGE',
+ val: `${startTime} : ${endTime}`,
+ formattedVal: xValueFormatter(timeValue),
+ };
+ }
+ } else if (data && data[0] !== undefined && data[0] !== null) {
+ xAxisDrillByFilter = {
+ col: xAxis.label,
+ op: '==',
+ val: data[0],
+ formattedVal: String(data[0]),
+ }
Review Comment:
Missing semicolon at the end of the object literal. While JavaScript allows
this, it's inconsistent with the codebase style and could lead to unintended
behavior in certain edge cases.
```suggestion
};
```
##########
superset-frontend/src/components/Chart/DrillBy/DrillByModal.tsx:
##########
@@ -366,12 +366,35 @@ export default function DrillByModal({
updatedFormData.slice_id = 0;
delete updatedFormData.slice_name;
delete updatedFormData.dashboards;
+
+ if (
+ updatedFormData.viz_type === 'echarts_timeseries_bar' &&
+ drillByConfigs?.length
+ ) {
+ const column = drillByConfigs[drillByConfigs.length - 1]?.column;
+ if (column?.column_name) {
+ updatedFormData.x_axis = column.column_name;
+ updatedFormData.groupby = [
+ {
+ sqlExpression: column.column_name,
+ expressionType: 'SQL',
+ optionName: column.column_name + ' (label)',
+ hasCustomLabel: true,
+ label: column.column_name + ' (label)',
+ isColumnReference: true,
+ },
+ ];
+ updatedFormData.xAxisForceCategorical = true;
+ }
+ }
Review Comment:
The complex drill-by behavior modification for echarts_timeseries_bar lacks
inline documentation. The code modifies x_axis, groupby, and
xAxisForceCategorical fields without explaining why these specific changes are
necessary or how they work together to achieve the desired drill-by behavior.
Adding a comment block explaining the purpose and interaction of these field
changes would improve maintainability and help future developers understand the
implementation.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:
##########
@@ -35,6 +35,7 @@ import Echart from '../components/Echart';
import { TimeseriesChartTransformedProps } from './types';
import { formatSeriesName } from '../utils/series';
import { ExtraControls } from '../components/ExtraControls';
+import {TIMEGRAIN_TO_TIMESTAMP} from "../constants";
Review Comment:
The import statement violates the project's style conventions by using
double quotes instead of single quotes. All other imports in the file use
single quotes, maintaining consistency is important for code maintainability.
```suggestion
import { TIMEGRAIN_TO_TIMESTAMP } from '../constants';
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:
##########
@@ -208,18 +209,49 @@ export default function EchartsTimeseries({
...(labelMap[seriesName] ?? []),
];
const groupBy = ensureIsArray(formData.groupby);
+ let xAxisDrillByFilter: BinaryQueryObjectFilterClause | undefined;
+
+ const xAxisColumn =
+ xAxis.label === DTTM_ALIAS
+ ? formData.granularitySqla
+ : xAxis.label;
+
if (data && xAxis.type === AxisType.Time) {
+
+ const timeValue = data[0];
+ const timeValueMs = typeof timeValue === 'number'
+ ? timeValue
+ : new Date(timeValue).getTime();
+ const grainMs =
+ TIMEGRAIN_TO_TIMESTAMP[formData.timeGrainSqla as keyof typeof
TIMEGRAIN_TO_TIMESTAMP] ??
+ TIMEGRAIN_TO_TIMESTAMP[TimeGranularity.DAY];
Review Comment:
The TIMEGRAIN_TO_TIMESTAMP values for MONTH, QUARTER, and YEAR use fixed day
counts (31 days per month, 31*3 days per quarter, 31*12 days per year), which
is mathematically incorrect and will produce inaccurate temporal ranges. Months
vary between 28-31 days, quarters vary between 90-92 days, and years vary
between 365-366 days. This simplified calculation will result in incorrect end
times for temporal range filters, potentially excluding or including incorrect
data points.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:
##########
@@ -208,18 +209,49 @@ export default function EchartsTimeseries({
...(labelMap[seriesName] ?? []),
];
const groupBy = ensureIsArray(formData.groupby);
+ let xAxisDrillByFilter: BinaryQueryObjectFilterClause | undefined;
+
+ const xAxisColumn =
+ xAxis.label === DTTM_ALIAS
+ ? formData.granularitySqla
+ : xAxis.label;
+
if (data && xAxis.type === AxisType.Time) {
+
+ const timeValue = data[0];
+ const timeValueMs = typeof timeValue === 'number'
+ ? timeValue
+ : new Date(timeValue).getTime();
+ const grainMs =
+ TIMEGRAIN_TO_TIMESTAMP[formData.timeGrainSqla as keyof typeof
TIMEGRAIN_TO_TIMESTAMP] ??
+ TIMEGRAIN_TO_TIMESTAMP[TimeGranularity.DAY];
Review Comment:
The TIMEGRAIN_TO_TIMESTAMP mapping is incomplete and does not cover all time
granularities supported by Superset. The TimeGranularity type includes DATE,
SECOND, MINUTE, FIVE_MINUTES, TEN_MINUTES, FIFTEEN_MINUTES, THIRTY_MINUTES,
WEEK, WEEK_STARTING_SUNDAY, WEEK_STARTING_MONDAY, WEEK_ENDING_SATURDAY, and
WEEK_ENDING_SUNDAY, but the mapping only includes HOUR, DAY, MONTH, QUARTER,
and YEAR. When users drill by on bars with these missing granularities, the
fallback to TimeGranularity.DAY will produce incorrect temporal ranges.
--
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]
