fix(chart): cross-filter emits dimension value instead of metric label for stacked bars#38120
fix(chart): cross-filter emits dimension value instead of metric label for stacked bars#38120nitishagar wants to merge 1 commit into
Conversation
Code Review Agent Run #0bb261Actionable 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 |
|
@nitishagar sorry if this is a half-baked question but how does this interact with #37407 which was merged recently and is cherry-picked into 6.0.1rc1? Does your PR interact with that one or are they touching different aspects of Superset? Just on my mind since I tested that other PR out this week in 6.0.1rc1. |
|
Superset uses Git pre-commit hooks courtesy of pre-commit. To install run the following: A series of checks will now run when you make a git commit. Alternatively it is possible to run pre-commit by running pre-commit manually: |
There was a problem hiding this comment.
Pull request overview
Fixes cross-filtering for stacked/multi-metric ECharts bar/timeseries (and related charts using shared handlers) so that emitted filters use the actual groupby dimension value(s) rather than the metric label.
Changes:
- Update
getCrossFilterDataMaskimplementations to skip metric label entries inlabelMapand extract the correct groupby dimension values. - Apply the same offset logic across Timeseries, MixedTimeseries, and shared
eventHandlers. - Add unit tests for the shared
eventHandlerscross-filter behavior (single- and multi-metric cases).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| superset-frontend/plugins/plugin-chart-echarts/test/utils/eventHandlers.test.ts | Adds unit tests verifying cross-filter emits dimension values (including multi-metric + multi-groupby scenarios). |
| superset-frontend/plugins/plugin-chart-echarts/src/utils/eventHandlers.ts | Adjusts cross-filter value extraction to skip metric label(s) in labelMap entries. |
| superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx | Adjusts cross-filter value extraction for timeseries/bar charts to skip metric label(s). |
| superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/EchartsMixedTimeseries.tsx | Adjusts cross-filter value extraction for mixed timeseries charts to skip metric label(s). |
Comments suppressed due to low confidence (3)
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:141
valis computed as an array viagroupbyValues.map(...), soval === null || val === undefinedcan’t be true. If a dimension value can be null/undefined, this will incorrectly emit anINfilter containing null instead ofIS NULL. Consider checking the selected element(s) insidevalinstead of the array itself.
const val = groupbyValues.map(v => {
const metricsCount = v.length - groupby.length;
return v[metricsCount + idx];
});
if (val === null || val === undefined)
return {
col,
op: 'IS NULL' as const,
};
return {
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/EchartsMixedTimeseries.tsx:94
valis an array (fromgroupbyValues.map(...)), soval === null || val === undefinedwill never be true. This makes theIS NULLbranch unreachable and can produceIN [null]when a groupby value is null/undefined. Consider checking the extracted value(s) insidevalinstead.
const val: DataRecordValue[] = groupbyValues.map(
v => {
const metricsCount =
v.length - currentGroupBy.length;
return v[metricsCount + idx];
},
);
if (val === null || val === undefined)
return {
col,
op: 'IS NULL' as const,
};
return {
superset-frontend/plugins/plugin-chart-echarts/src/utils/eventHandlers.ts:76
valis always an array (fromgroupbyValues.map(...)), so theval === null || val === undefinedcheck will never be true. This prevents emitting anIS NULLfilter when the selected dimension value is actually null/undefined (you’ll end up withIN [null], which won’t match in SQL). Consider checking the extracted element(s) instead (e.g.,val[0]for single-select behavior, orval.every(...)/split clauses if multi-select is supported).
const val = groupbyValues.map(v => {
const metricsCount = v.length - groupby.length;
return v[metricsCount + idx];
});
if (val === null || val === undefined)
return {
col,
op: 'IS NULL' as const,
};
| const val = groupbyValues.map(v => { | ||
| const metricsCount = v.length - groupby.length; | ||
| return v[metricsCount + idx]; | ||
| }); |
There was a problem hiding this comment.
This change fixes stacked/multi-metric cross-filtering in getCrossFilterDataMask, but there isn’t a unit test covering the multi-metric + groupby click path in EchartsTimeseries (the reported regression case). Adding a test that simulates a click on a stacked-bar seriesName like 'Intent, cancellations' and asserts the emitted filter uses the dimension value would prevent regressions.
sadpandajoe
requested review from
Copilot,
kgabryje,
michael-s-molina and
villebro
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (3)
superset-frontend/plugins/plugin-chart-echarts/src/utils/eventHandlers.ts:76
valis always an array here (result ofgroupbyValues.map(...)), so theval === null || val === undefinedcheck will never be true. This makes theIS NULLbranch unreachable and will emit anINfilter even when the dimension value isnull/undefined. Consider checking the mapped values (e.g.,val[0]when single-select, orval.some(v => v == null)/splitting intoIN+IS NULLlogic) instead of checking the array itself.
const val = groupbyValues.map(v => {
const metricsCount = v.length - groupby.length;
return v[metricsCount + idx];
});
if (val === null || val === undefined)
return {
col,
op: 'IS NULL' as const,
};
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:141
valis an array (fromgroupbyValues.map(...)), soval === null || val === undefinedwill never be true and theIS NULLfilter path is unreachable. If the intention is to support null dimension values, check the contents ofval(typicallyval[0]since cross-filtering is single-select) and emitIS NULLwhen that value is nullish.
const val = groupbyValues.map(v => {
const metricsCount = v.length - groupby.length;
return v[metricsCount + idx];
});
if (val === null || val === undefined)
return {
col,
op: 'IS NULL' as const,
};
return {
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/EchartsMixedTimeseries.tsx:93
valis declared asDataRecordValue[]and assigned viagroupbyValues.map(...), so the subsequentval === null || val === undefinedcheck can never succeed. If null groupby values are expected, check the mapped entries (e.g.,val[0]for single-select) and emit anIS NULLclause based on the element value(s), not the array reference.
const val: DataRecordValue[] = groupbyValues.map(
v => {
const metricsCount =
v.length - currentGroupBy.length;
return v[metricsCount + idx];
},
);
if (val === null || val === undefined)
return {
col,
op: 'IS NULL' as const,
};
| v => { | ||
| const metricsCount = | ||
| v.length - currentGroupBy.length; | ||
| return v[metricsCount + idx]; | ||
| }, | ||
| ); |
There was a problem hiding this comment.
The cross-filter labelMap indexing logic changed here, but there are no tests covering EchartsMixedTimeseries click-to-cross-filter behavior for multi-metric stacked series (including multiple groupby columns). Adding a small unit test (or factoring the indexing into a shared, tested helper) would help ensure this fix doesn’t regress.
| const metricsCount = v.length - groupby.length; | ||
| return v[metricsCount + idx]; | ||
| }); | ||
| if (val === null || val === undefined) |
There was a problem hiding this comment.
The offset logic for multi-metric labelMap entries is updated here, but there doesn’t appear to be any unit test coverage exercising EchartsTimeseries cross-filter click behavior (especially for stacked multi-metric series). Consider adding a focused test (or extracting this indexing into a shared helper that can be unit-tested) to prevent regressions for the original bug scenario.
| if (val === null || val === undefined) | |
| if (val.every(vv => vv == null)) |
codeant-ai-for-open-source
Bot
added
the
size:L
Sequence DiagramThis PR fixes cross-filtering for multi-metric stacked charts so that clicking a segment emits filters based on groupby dimension values (with metric fields skipped) and updates dependent charts correctly. sequenceDiagram
participant User
participant EchartsChart
participant CrossFilterHandler
participant DashboardFilters
participant DependentChart
User->>EchartsChart: Click stacked bar segment
EchartsChart->>CrossFilterHandler: Handle click with labelMap and groupby
CrossFilterHandler->>CrossFilterHandler: Skip metric labels and derive dimension values
CrossFilterHandler->>DashboardFilters: setDataMask with dimension filters
DashboardFilters->>DependentChart: Apply filters and refresh
DependentChart-->>User: Updated data for selected dimension
Generated by CodeAnt AI |
Code Review Agent Run #8f3a5aActionable 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 |
…l for stacked bars When clicking a stacked bar segment in a multi-metric chart, the cross-filter incorrectly used the metric label as the filter value instead of the groupby dimension value. This happened because labelMap arrays contain both metric labels and dimension values (e.g., ["Intent", "cancellations"]), but the getCrossFilterDataMask function indexed from position 0 (the metric label) instead of skipping to the dimension values. The fix calculates a metricsCount offset (labelMap entry length minus groupby count) and indexes from there, consistent with how the drillBy handler already works in EchartsTimeseries.tsx. Applied to all three getCrossFilterDataMask implementations: - EchartsTimeseries.tsx (Timeseries/Bar charts) - EchartsMixedTimeseries.tsx (Mixed Timeseries charts) - eventHandlers.ts (shared handler for Pie, Funnel, Gauge, Radar, BoxPlot) Closes apache#37882
codeant-ai-for-open-source
Bot
added
size:L
Code Review Agent Run #928805Actionable Suggestions - 0Additional Suggestions - 1
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 |
User description
SUMMARY
Fixes the cross-filter behavior for stacked bar charts with multiple metrics. When clicking a stacked bar segment, the cross-filter incorrectly emitted the metric label (e.g.,
"Intent") as the filter value instead of the actual groupby dimension value (e.g.,"cancellations"). This caused downstream charts to receive invalid filters and return no rows.Root cause: The
labelMaparrays for multi-metric series contain both metric labels and dimension values (e.g.,["Intent", "cancellations"]), butgetCrossFilterDataMaskindexed from position0(the metric label) instead of skipping the metric entries to reach the dimension values.Fix: Calculate a
metricsCountoffset (labelMap entry length - groupby column count) and index from there. This is the same approach already used by thedrillByhandler inEchartsTimeseries.tsx(lines 240-241), which correctly skips metric values.Applied consistently across all three
getCrossFilterDataMaskimplementations:EchartsTimeseries.tsx— Timeseries/Bar chartsEchartsMixedTimeseries.tsx— Mixed Timeseries chartseventHandlers.ts— shared handler used by Pie, Funnel, Gauge, Radar, BoxPlotFor single-metric charts, the offset is
0, so behavior is unchanged.TESTING INSTRUCTIONS
topics) and two metrics (e.g.,Intent,Volume)topicsas the groupby dimensiontopics = 'cancellations'instead of using the metric labelADDITIONAL INFORMATION
CodeAnt-AI Description
Use the clicked dimension value when cross-filtering stacked and multi-metric charts
What Changed
Impact
✅ Correct filters from stacked chart clicks✅ Fewer empty or wrong dashboard results✅ Reliable cross-filtering across chart types💡 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.