Skip to content

feat(chart): drill-by drills into bar on bar chart #36901

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
JayHealth wants to merge 2 commits into
base: master
Choose a base branch
from
+66 −8
Open

feat(chart): drill-by drills into bar on bar chart #36901

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c50ad65
feat(chart): drill-by drills into bar on bar chart
Jan 5, 2026
ba4e9ba
feat(chart): implemented changes suggested in review
Jan 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 43 additions & 8 deletions superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import {
getColumnLabel,
getNumberFormatter,
LegendState,
ensureIsArray,
ensureIsArray, TimeGranularity,
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
} from '@superset-ui/core';
import type { ViewRootGroup } from 'echarts/types/src/util/types';
import type GlobalModel from 'echarts/types/src/model/Global';
Expand All @@ -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";
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
import {TIMEGRAIN_TO_TIMESTAMP} from "../constants";
import { TIMEGRAIN_TO_TIMESTAMP } from '../constants';

Copilot uses AI. Check for mistakes.

const TIMER_DURATION = 300;

Expand Down Expand Up @@ -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];
Comment on lines +225 to +227
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment on lines +225 to +227
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TIMEGRAIN_TO_TIMESTAMP values for MONTH, QUARTER, and YEAR use fixed day counts (31 days per month, 313 days per quarter, 3112 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.

Copilot uses AI. Check for mistakes.

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]),
}
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
}
};

Copilot uses AI. Check for mistakes.
} else {
xAxisDrillByFilter = undefined;
}
[
...(xAxis.type === AxisType.Category && data ? [xAxis.label] : []),
Expand All @@ -232,6 +264,9 @@ export default function EchartsTimeseries({
formattedVal: String(values[i]),
}),
);
if (xAxisDrillByFilter) {
drillByFilters.push(xAxisDrillByFilter);
}
groupBy.forEach((dimension, i) => {
const dimensionValues = labelMap[seriesName] ?? [];

Expand Down
23 changes: 23 additions & 0 deletions superset-frontend/src/components/Chart/DrillBy/DrillByModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)',
Comment on lines +381 to +383
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
optionName: column.column_name + ' (label)',
hasCustomLabel: true,
label: column.column_name + ' (label)',
optionName: column.column_name,
hasCustomLabel: true,
label: column.verbose_name || column.column_name,

Copilot uses AI. Check for mistakes.
isColumnReference: true,
},
];
Comment on lines +377 to +386
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
updatedFormData.groupby = [
{
sqlExpression: column.column_name,
expressionType: 'SQL',
optionName: column.column_name + ' (label)',
hasCustomLabel: true,
label: column.column_name + ' (label)',
isColumnReference: true,
},
];
// 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,
);
}

Copilot uses AI. Check for mistakes.
updatedFormData.xAxisForceCategorical = true;
}
}
Comment on lines +370 to +389
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment on lines +370 to +389
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

return updatedFormData;
}, [
currentFormData,
currentColumn,
groupbyFieldName,
getFiltersFromConfigsByFieldName,
drillByConfigs,
getNewGroupby,
formData,
]);
Expand Down
Loading