Repository navigation
Conversation
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #45680 +/- ##
==========================================
+ Coverage 84.35% 84.37% +0.01%
==========================================
Files 3059 3060 +1
Lines 202239 202462 +223
Branches 47377 47487 +110
==========================================
+ Hits 170599 170819 +220
- Misses 28382 28384 +2
- Partials 3258 3259 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
EnxDev
left a comment
EnxDev
left a comment
There was a problem hiding this comment.
Read through this one and it holds up. sadpandajoe's three threads are covered in the diff with tests: a grained temporal bucket goes out as TEMPORAL_RANGE plus time_range (same pattern as Timeseries and Sunburst), the context-menu test pins crossFilter for both the unselected and the selected bar, and the breakdown dimming case is asserted.
One small test gap inline. Separately, the description still says a time grain does not expand the value into a TEMPORAL_RANGE and that the filter matches the raw cell value, which is the opposite of what the code does after the last round. Worth updating it, and adding a temporal X axis step to the testing instructions so whoever checks it by hand knows to expect a whole-month filter.
EnxDev
left a comment
EnxDev
left a comment
There was a problem hiding this comment.
Looked at the drill-by change and it holds up. Drill by now reuses the grained == clause from getWaterfallDrillFilters, so the modal's saved time_range can't widen the January bucket into the whole range. That's the same clause Sunburst sends, and DrillByTemporalFilters.test.tsx on master already pins that the grain survives into the modal query. The updated EchartsWaterfall assertion would fail against the old TEMPORAL_RANGE clause, and the new default cursor test covers what I asked about last time. The Waterfall suites pass locally at this head.
No new inline notes. One thing left on the description: it still says a time grain does not expand the value into a TEMPORAL_RANGE. That's still true for drill to detail and drill by, but the cross-filter goes out as a TEMPORAL_RANGE plus time_range on a grained temporal axis. A sentence splitting the two would save the next reader some confusion.
EnxDev
left a comment
EnxDev
left a comment
There was a problem hiding this comment.
The Custom SQL fixes check out. temporalBucketFilter skips adhoc columns so nothing sends TEMPORAL_RANGE on an expression, Drill to detail and Drill by both truncate through adhocBucketColumn (the backend does truncate an adhoc BASE_AXIS column that carries a timeGrain), and the value goes out as a naive datetime, with the test feeding the epoch shape. CI is green.
Two notes inline, both on the Custom SQL path. The description could also use a line on it: an adhoc X axis cross-filters with grained == and no time_range, unlike a dataset column.
| if (!subject || typeof subject !== 'object' || subject.column_name) { | ||
| return undefined; | ||
| } | ||
| return typeof subject.label === 'string' ? subject.label : undefined; |
There was a problem hiding this comment.
The pill label is right, but opening it lands on the Simple tab with a blank column picker. AdhocFilterEditPopoverSimpleTabContent only resolves a string subject or one with column_name, so the Custom SQL bucket shows up as just = 2021-01-01T00:00:00.
Value edits still go through, so a follow-up is fine. Could the picker fall back to the adhoc label the same way?
EnxDev
left a comment
EnxDev
left a comment
There was a problem hiding this comment.
The picker fix checks out. A Custom SQL bucket now opens on the Simple tab with its label selected, and picking that entry returns early instead of swapping the subject for a bare string, so the expression and its grain stay on the filter. The new test covers the display side.
Nothing else from me on this one.
|
|
||
| const crossFilterHandlers: EventHandlers = useMemo(() => { | ||
| const selectedKeys = getSelectedWaterfallKeys(filterState?.selectedValues); | ||
| const categoryAt = (event: WaterfallBarEvent) => { |
There was a problem hiding this comment.
categoryAt maps every series hit to a category by dataIndex, including the transparent ASSIST_MARK spacer series. Those spacers are drawn with a transparent fill but are still hit-testable and carry the pointer cursor, so with values 10 then 3 the second visible bar spans 10–13, and clicking the empty area under it (0–10) sets or clears that category's cross-filter; right-clicking it opens the drill menu. Should the handlers ignore events from the assist series (e.g. check event.seriesName) while still accepting the colored sign-crossing segments?
| result = { | ||
| expressionType: 'SQL', | ||
| clause, | ||
| sqlExpression: translateToSql({ |
There was a problem hiding this comment.
For a Custom SQL breakdown such as COALESCE(name, ''), Drill by on a bar whose value is '' reaches this branch with val: ''. translateToSql goes through getSimpleSQLExpression, which formats comparators with optionLabel, so the empty string becomes the display marker <empty string> and (since Number('') is not NaN) it is left unquoted, giving (COALESCE(name, '')) = <empty string>. That is invalid SQL, so the drilled chart errors instead of showing the matching rows. Could this branch keep a structured filter, or serialize the real SQL literal for empty strings?
| isCurrentValueSelected: false, | ||
| }; | ||
|
|
||
| test('a context menu carries the drill filters for the clicked bar', () => { |
There was a problem hiding this comment.
Every context-menu test here renders with emitCrossFilters: true, so nothing covers Drill to detail / Drill by when cross-filtering is off, which the PR says should still work. If the contextmenu handler were gated on emitCrossFilters, these tests would still pass. Could you add a case with emitCrossFilters: false that right-clicks a breakdown bar and asserts onContextMenu still receives the drillToDetail and drillBy filters while setDataMask is not called?
SUMMARY
Adds dashboard cross-filtering to the Waterfall chart, plus Drill to detail and Drill by.
A click sets one filter and a second click on the same bar clears it. The grand total is not a category: clicking it or opening its context menu does nothing and does not clear an existing filter.
NULLusesIS NULL. An empty string staysIN ['']. Chips use<NULL>and<empty string>.bigintdoes not share a selection key with the same decimal string (42nvs'42'). The filter value is still the decimal string, so the data mask stays JSON-serializable.A temporal X axis with a time grain filters the whole bucket. The bar value is the bucket start (1 January for a month), so an exact
INor==would miss rows later in that bucket. Cross-filter emitsTEMPORAL_RANGEfor[start, next bucket)and setsextraFormData.time_rangeto that range. The backend copies the query time range onto everyTEMPORAL_RANGEfilter, so the clicked bucket has to be that time range. A dashboard time grain overrides the chart grain. A non-temporal X axis, and a temporal axis with no grain, stay an exact match.Drill to detail and Drill by on the X axis use
==plusgraininstead ofTEMPORAL_RANGE. The drill-by modal still sends the chart's saved time range, and the backend would rewrite aTEMPORAL_RANGEto that whole range. Grained equality is applied to the truncated column, so January stays January. Breakdown filters stay exact.Bars outside the selection are drawn at 30% opacity, including when cross-filtering is off. The pointer cursor is used only when cross-filtering is on.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
TEMPORAL_RANGEcovering January, not an exact match on midnight 1 January, and the other chart shows both rows. Repeat with a dashboard time-grain override and confirm that grain wins.2021-01-01 : 2022-01-01. Drill to detail should use==with the month grain.ADDITIONAL INFORMATION