codeant-ai-for-open-source[bot] commented on code in PR #42296:
URL: https://github.com/apache/superset/pull/42296#discussion_r3626623823
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:
##########
@@ -343,6 +345,55 @@ export default function EchartsTimeseries({
});
});
+ // Filters for the clicked x-axis value, so Drill By can subset the
+ // drilled data to the clicked bar/point rather than only the series
+ const xAxisFilters: BinaryQueryObjectFilterClause[] = [];
+ const xAxisCol =
+ // if the xAxis is '__timestamp', granularity_sqla will be the
column of filter
+ xAxis.label === DTTM_ALIAS ? formData.granularitySqla : xAxis.label;
+ if (data && xAxis.type === AxisType.Time && xAxisCol) {
+ // For horizontal orientation the [x, value] pair is swapped
+ const xValue = Array.isArray(data)
+ ? data[categoryAxisValueIndex]
+ : data;
+ const xAxisFilter = getTemporalXAxisDrillByFilter(
+ xAxisCol,
+ xValue,
+ formData.timeGrainSqla,
+ String(xValueFormatter(xValue as number)),
+ );
+ if (xAxisFilter) {
+ xAxisFilters.push(xAxisFilter);
+ }
+ } else if (xAxis.type === AxisType.Category && xAxisCol) {
+ const categoryAxisValue = getCategoryAxisValue(
+ data,
+ eventParams.name,
+ );
+ if (categoryAxisValue !== undefined) {
+ // A category axis can still sit on a temporal column when the
+ // axis is forced categorical; filter by time bucket in that case
+ const xAxisFilter =
+ coltypeMapping?.[getColumnLabel(xAxis.label)] ===
+ GenericDataType.Temporal
Review Comment:
**Suggestion:** The temporal-type check for a force-categorical axis uses
`xAxis.label` instead of the already-normalized `xAxisCol`. When the label is
`__timestamp`, this lookup can miss the real temporal column type and
incorrectly build an `==` filter instead of a time-bucket filter, so drill-by
returns wrong rows for temporal axes forced to categorical. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Drill By on forced-categorical temporal x-axes misfilters rows.
- ⚠️ Users see unexpected results when drilling into time buckets.
- ⚠️ Inconsistent behavior versus time-axis drill-by branch.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start Superset with this PR and open any chart using the Timeseries
ECharts
visualization, which renders `EchartsTimeseries` from
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx`
(component exported around line 46).
2. Configure the chart so the x-axis is driven by a temporal column via
`formData.granularitySqla` (used when `xAxis.label === DTTM_ALIAS` at lines
351–353), and
force the axis to categorical so that at click time `xAxis.type ===
AxisType.Category` and
`xAxis.label === DTTM_ALIAS` in the series context-menu handler around lines
348–395.
3. Right-click a bar/point on that chart to open the Drill By context menu;
the handler
builds `xAxisFilters` using the temporal-type check at lines 377–378 and
then passes them
into `onContextMenu` at lines 414–420 via the `drillBy` payload.
4. Because the type check uses
`coltypeMapping?.[getColumnLabel(xAxis.label)]` (lines
377–378) instead of the normalized `xAxisCol` (which is set to
`formData.granularitySqla`
when `xAxis.label === DTTM_ALIAS` at lines 351–353), the lookup can return
`undefined` for
the real temporal column. The code then falls into the non-temporal branch
building a
simple `==` filter on the category label (lines 385–390). Executing Drill By
from the
context menu yields filters that match on the string label instead of a
proper time
bucket, returning incorrect row subsets for temporal axes forced to
categorical.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5aed8df34a2d410dbec4503db30b1694&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=5aed8df34a2d410dbec4503db30b1694&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx
**Line:** 377:378
**Comment:**
*Logic Error: The temporal-type check for a force-categorical axis uses
`xAxis.label` instead of the already-normalized `xAxisCol`. When the label is
`__timestamp`, this lookup can miss the real temporal column type and
incorrectly build an `==` filter instead of a time-bucket filter, so drill-by
returns wrong rows for temporal axes forced to categorical.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42296&comment_hash=31653ab515559c6e642e8ae84a48bb7b1342be10460b78bc747616549bee004f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42296&comment_hash=31653ab515559c6e642e8ae84a48bb7b1342be10460b78bc747616549bee004f&reaction=dislike'>👎</a>
--
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]
