codeant-ai-for-open-source[bot] commented on code in PR #40180:
URL: https://github.com/apache/superset/pull/40180#discussion_r3525372636
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/PivotTableChart.tsx:
##########
@@ -419,10 +461,14 @@ export default function PivotTableChart(props:
PivotTableProps) {
col,
op: 'IS NULL',
};
+ const formatter =
+ typeof col === 'string' ? dateFormatters[col] : undefined;
return {
col,
op: 'IN',
- val: val as (string | number | boolean)[],
+ val: (val as DataRecordValue[]).map(value =>
+ getCrossFilterValue(value, formatter),
+ ) as (string | number | boolean)[],
};
Review Comment:
**Suggestion:** Temporal coercion is skipped for adhoc groupby columns in
click-based cross-filters because the formatter lookup is gated on `typeof col
=== 'string'`. For adhoc columns, `col` is an object, so epoch-millisecond
strings remain strings and can still compile as invalid temporal literals
downstream. Resolve the formatter from the selected key/label (or adhoc label)
instead of only from string-typed `col`. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Critical π¨</summary>
```mdx
- β Pivot table header clicks break cross-filter for adhoc temporals.
- β BigQuery temporal cross-filter queries use string literal timestamps.
- β οΈ Inconsistent behavior between physical and adhoc temporal dimensions.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. In `transformProps`
(`superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/transformProps.ts:124-154`),
a `dateFormatters` map is built for temporal result columns, keyed by the
temporal column
names from `colnames`, and wired into `PivotTableChart` props.
2. Configure a Pivot Table whose `groupbyRows` or `groupbyColumns` (passed
via `formData`,
read in `transformProps` at lines 98-100) include an adhoc temporal
`QueryFormColumn`
(non-string) that appears as a temporal column in `colnames`, so
`dateFormatters` has an
entry for this column.
3. Render the chart and click a column or row header for that adhoc temporal
groupby;
`TableRenderers` builds filters keyed by the header label
(`react-pivottable/TableRenderers.tsx:46-75` and `340-77`), and invokes
`clickColumnHeaderCallback`/`clickRowHeaderCallback`, which are
`toggleFilter` from
`PivotTableChart` (`PivotTableChart.tsx:74-137`).
4. Inside `toggleFilter`, `handleChange` is called with `SelectedFiltersType`
(`PivotTableChart.tsx:438-485`), which resolves `col` by finding the matching
`groupbyRowsRaw`/`groupbyColumnsRaw` entry; for adhoc columns
`isAdhocColumn(item)` is
true and `col` becomes the adhoc `QueryFormColumn` object (lines 450-457).
Because `typeof
col !== 'string'`, the formatter is forced to `undefined` (`const formatter
= typeof col
=== 'string' ? dateFormatters[col] : undefined;` at lines 464-465), so
`getCrossFilterValue` (`lines 233-245`) returns the original string value
(for example
"1778630400000") without coercion. The resulting `extraFormData.filters`
entries sent via
`setDataMask` contain string epoch values for temporal adhoc groupbys,
leaving temporal
coercion incomplete and reintroducing the backend issue this PR is trying to
fix for those
columns.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=78fa9f3f98ae48a08dd56d95df3acd80&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=78fa9f3f98ae48a08dd56d95df3acd80&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-pivot-table/src/PivotTableChart.tsx
**Line:** 464:472
**Comment:**
*Incomplete Implementation: Temporal coercion is skipped for adhoc
groupby columns in click-based cross-filters because the formatter lookup is
gated on `typeof col === 'string'`. For adhoc columns, `col` is an object, so
epoch-millisecond strings remain strings and can still compile as invalid
temporal literals downstream. Resolve the formatter from the selected key/label
(or adhoc label) instead of only from string-typed `col`.
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%2F40180&comment_hash=1b1a470676d4ff89bdfe687ed30fcdd35941b8af94ecf8be5d86bd474edb2262&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40180&comment_hash=1b1a470676d4ff89bdfe687ed30fcdd35941b8af94ecf8be5d86bd474edb2262&reaction=dislike'>π</a>
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/PivotTableChart.tsx:
##########
@@ -492,10 +538,14 @@ export default function PivotTableChart(props:
PivotTableProps) {
col,
op: 'IS NULL' as const,
};
+ const formatter =
+ typeof col === 'string' ? dateFormatters[col] :
undefined;
return {
col,
op: 'IN' as const,
- val: val as (string | number | boolean)[],
+ val: (val as DataRecordValue[]).map(value =>
+ getCrossFilterValue(value, formatter),
+ ) as (string | number | boolean)[],
};
Review Comment:
**Suggestion:** The same formatter-resolution gap exists in context-menu
cross-filter generation, so adhoc temporal headers still produce string epoch
values in `IN` filters. This leaves drill/cross-filter behavior inconsistent
and can still trigger engine-specific temporal compilation failures for adhoc
temporal fields. Use key/label-based formatter lookup here as well. [incomplete
implementation]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
- β Context menu cross-filters mis-handle adhoc temporal headers.
- β Drill menus send string epochs to temporal filter pipeline.
- β οΈ Inconsistent coercion between click and context-menu filters.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. As in the click-path, `transformProps` builds `dateFormatters` keyed by
temporal result
column names
(`superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/transformProps.ts:124-154`)
and passes them, along with `groupbyRows`/`groupbyColumns`, into
`PivotTableChart`.
2. In `TableRenderers`, column header rendering wires a context menu handler
that calls
the chartβs `onContextMenu` hook with a `dataPoint` map containing the
header attr name
and value (`react-pivottable/TableRenderers.tsx:930-1030`, specifically
lines 16-19 and
95-101).
3. `PivotTableChart`βs `handleContextMenu` (`PivotTableChart.tsx:185-251`)
receives this
`dataPoint` and constructs a context menu payload where `crossFilter` is
computed by
`getCrossFilterDataMask(dataPoint)` (`PivotTableChart.tsx:500-71`). For each
entry in
`values`, it resolves `col` from `groupbyRowsRaw`/`groupbyColumnsRaw`
exactly as in
`handleChange`, so for an adhoc temporal groupby `col` is again a
`QueryFormColumn`
object.
4. In `getCrossFilterDataMask`, when `val` is non-null, it computes
`formatter` as `typeof
col === 'string' ? dateFormatters[col] : undefined;`
(`PivotTableChart.tsx:536-543). For
adhoc columns `typeof col !== 'string'`, so `formatter` is `undefined` and
`getCrossFilterValue` (`lines 233-245`) returns the original string epoch
(for example
"1778630400000") instead of coercing to a number. The
`dataMask.dataMask.extraFormData.filters` emitted through the context menu
therefore carry
string epoch values for adhoc temporal headers, so context-menu-triggered
cross-filters
continue to send string temporal literals to downstream engines while
physical columns
benefit from numeric coercion.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e60c957e92ba4b328328391ad645750b&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=e60c957e92ba4b328328391ad645750b&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-pivot-table/src/PivotTableChart.tsx
**Line:** 541:549
**Comment:**
*Incomplete Implementation: The same formatter-resolution gap exists in
context-menu cross-filter generation, so adhoc temporal headers still produce
string epoch values in `IN` filters. This leaves drill/cross-filter behavior
inconsistent and can still trigger engine-specific temporal compilation
failures for adhoc temporal fields. Use key/label-based formatter lookup here
as well.
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%2F40180&comment_hash=4180e9e1b8b8b68e9658a0c10ac0ad41f949ba56c892eb2b731edd58fa090b69&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40180&comment_hash=4180e9e1b8b8b68e9658a0c10ac0ad41f949ba56c892eb2b731edd58fa090b69&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]
