rebenitez1802 commented on code in PR #45612:
URL: https://github.com/apache/superset/pull/45612#discussion_r4365588275
##########
superset-frontend/src/explore/components/controls/FilterControl/AdhocFilter/index.ts:
##########
@@ -140,6 +146,11 @@ export default class AdhocFilter {
operator: this.operator,
operatorId: this.operatorId,
comparator: this.comparator,
+ // A grain belongs to its subject's time buckets.
+ timeGrain:
+ nextFields.subject === undefined || nextFields.subject === this.subject
+ ? this.timeGrain
+ : undefined,
Review Comment:
🔴 High — A drilled filter keeps its time grain after the user picks a date
range
`duplicateWith` keeps `timeGrain` whenever the column is unchanged, even
when the operator changes. After Drill by → "Edit chart", the pill reads
`order_date = '2004-01-01T00:00:00'`. Opening it shows the date picker set to
"No filter". Picking `2004-01-15 : 2004-02-15` goes through
`onDatePickerChange` → `duplicateWith({ subject, operator: TEMPORAL_RANGE,
comparator })` and produces:
```json
{"col":"order_date","op":"TEMPORAL_RANGE","val":"2004-01-15 :
2004-02-15","grain":"P1M"}
```
The backend then compares the month-truncated column against the range:
`DATE_TRUNC('month', order_date) >= '2004-01-15' AND ... < '2004-02-15'`. On a
small SQLite fixture this returns Feb 10 and Feb 20, where the correct rows are
Jan 25 and Feb 10. On master the same edit is correct, because the drilled
filter was a plain `TEMPORAL_RANGE`.
The suggestion below also drops the grain when the operator changes. Edits
that only change the value or the clause still keep it, so the existing "keeps
its time grain through Explore edits" test still passes. I checked it locally:
- `oxfmt --check` and oxlint are clean.
- `src/explore/components/controls/FilterControl`, `src/utils` and
`src/components/Chart/DrillBy` pass (1016 tests).
- The date-picker edit above returns Jan 25 and Feb 10.
```suggestion
// A grain belongs to its subject's time buckets under its operator,
so a
// new column or operator (e.g. a picked time range) drops it.
timeGrain:
(nextFields.subject === undefined ||
nextFields.subject === this.subject) &&
(nextFields.operator === undefined ||
nextFields.operator === this.operator)
? this.timeGrain
: undefined,
```
--
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]
