feat(chart): drill-by drills into bar on bar chart #36901
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Code Review Agent Run #9c4206Actionable Suggestions - 0Additional Suggestions - 2
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
codeant-ai-for-open-source
bot
added
the
size:M
Nitpicks 🔍
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx
Show resolved
Hide resolved
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/components/Chart/DrillBy/DrillByModal.tsx
Outdated
Show resolved
Hide resolved
|
CodeAnt AI finished reviewing your PR. |
Code Review Agent Run #6b1076Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Interesting! I can 1000% see the utility of this, but it's also a very different animal than what we have today, which is supported by ALL charts (or at least the "main" ones). If we were able to do this for ALL charts, that'd be a very cool thing... but changing the behavior for one chart type seems like an antipattern from a UX perspective. I wonder if this should be given a new name, and put alongside Drill By, e.g. Zoom By or something... then we can have it BOTH ways, which is nice, and start spreading this option around to other supported charts (I could see it working for Pie, Line, Area, whatever) Curious what @kasiazjc or others like @michael-s-molina @kgabryje @yousoph think. |
|
@rusackas indeed. I like having my pie chart and eating it too. Confusingly though, pie chart already kinda works like this, but the timeseries charts don't. |
|
Agreed with @rusackas on both points - support for extra chart types and a new menu option/name |
|
@yousoph @rusackas Nice, nice. if it's implemented as an extra menu option, how would pie chart handle it? Since pie chart already functions like this, there would be two menu options that do the same thing. Or should pie chart only support the option, in order to work consistently? I'm curious, the old functionality, where does that come from? I can't really imagine a real world scenario where one would like to run drill by and then see ALL the columns become subdivided. But perhaps the behaviour is based on a need I'm not seeing. |
|
Both the current behavior and the one expected by @JayHealth are valid. I think we should fix this by using the same strategy I used in Screen.Recording.2026-01-07.at.09.08.23.mov |
There was a problem hiding this comment.
Pull request overview
This pull request implements drill-by functionality for bar charts in Superset, allowing users to right-click on a specific bar and drill into it by replacing the x-axis with the selected drill-by dimension. The implementation adds temporal range filters for time-based bars and updates the chart configuration to display the drilled data correctly.
Key Changes
- Modified drill-by modal to replace x-axis with drill-by column for echarts_timeseries_bar charts
- Added temporal range filter calculation for time-based bar charts using TIMEGRAIN_TO_TIMESTAMP mapping
- Implemented x-axis filter logic to target specific bars during drill-by operations
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| superset-frontend/src/components/Chart/DrillBy/DrillByModal.tsx | Added logic to update formData for echarts_timeseries_bar charts, setting x_axis, groupby, and xAxisForceCategorical when drilling by |
| superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx | Implemented xAxisDrillByFilter with temporal range calculation for time axes and equality filters for categorical axes to enable precise drill-by targeting |
| const grainMs = | ||
| TIMEGRAIN_TO_TIMESTAMP[formData.timeGrainSqla as keyof typeof TIMEGRAIN_TO_TIMESTAMP] ?? | ||
| TIMEGRAIN_TO_TIMESTAMP[TimeGranularity.DAY]; |
There was a problem hiding this 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.
| import { TimeseriesChartTransformedProps } from './types'; | ||
| import { formatSeriesName } from '../utils/series'; | ||
| import { ExtraControls } from '../components/ExtraControls'; | ||
| import {TIMEGRAIN_TO_TIMESTAMP} from "../constants"; |
There was a problem hiding this 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.
| import {TIMEGRAIN_TO_TIMESTAMP} from "../constants"; | |
| import { TIMEGRAIN_TO_TIMESTAMP } from '../constants'; |
| optionName: column.column_name + ' (label)', | ||
| hasCustomLabel: true, | ||
| label: column.column_name + ' (label)', |
There was a problem hiding this 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.
| 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, |
| 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; | ||
| } | ||
| } |
There was a problem hiding this 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.
| updatedFormData.groupby = [ | ||
| { | ||
| sqlExpression: column.column_name, | ||
| expressionType: 'SQL', | ||
| optionName: column.column_name + ' (label)', | ||
| hasCustomLabel: true, | ||
| label: column.column_name + ' (label)', | ||
| isColumnReference: true, | ||
| }, | ||
| ]; |
There was a problem hiding this 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.
| 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, | |
| ); | |
| } |
| getNumberFormatter, | ||
| LegendState, | ||
| ensureIsArray, | ||
| ensureIsArray, TimeGranularity, |
There was a problem hiding this 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.
| op: '==', | ||
| val: data[0], | ||
| formattedVal: String(data[0]), | ||
| } |
There was a problem hiding this 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.
| } | |
| }; |
| const grainMs = | ||
| TIMEGRAIN_TO_TIMESTAMP[formData.timeGrainSqla as keyof typeof TIMEGRAIN_TO_TIMESTAMP] ?? | ||
| TIMEGRAIN_TO_TIMESTAMP[TimeGranularity.DAY]; |
There was a problem hiding this comment.
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.
| 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; | ||
| } | ||
| } |
There was a problem hiding this 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.
4 participants
User description
SUMMARY
Makes it so that right clicking and using drill by on a bar in the bar chart, actually drills into that particular bar, replacing the x-axis of the bart chart, with what was selected as the drill by value. This is how drill-by functionality works in similar programs.
SCREENSHOTS OF HOW IT WORKS:
TESTING INSTRUCTIONS
On a bar chart, right click a column and select a category. A pop up window will appear where the result is visible.
ADDITIONAL INFORMATION
CodeAnt-AI Description
Drill-by focuses timeseries bar: replace x-axis and apply precise filters
What Changed
Impact
✅ Drill into a specific bar✅ Temporal-range filters for time-based bars✅ X-axis updated to drilled dimension for bar charts💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.