aminghadersohi commented on PR #42144:
URL: https://github.com/apache/superset/pull/42144#issuecomment-5037871605
Thanks for the deep dive — this caught a real gap the automated reviewers
missed. Addressed in 68b72b5e81:
**1. `'this week'` (merge-blocking):** Confirmed — `'this week'` has no `' :
'` separator and matches none of `get_since_until()`'s recognized prefixes, so
it silently returns `(None, today)` (unbounded start → full table), exactly as
you found. Replaced it with `'Current week'` in the field description, verified
it resolves to a bounded range, and added a regression test that parses every
example named in the description so this can't silently regress again. Also
independently confirmed the `last week` vs `Last week` case-sensitivity issue
you flagged — that's the same `get_since_until()` behavior
(`startswith("Last")`), not something this PR's normalization touches, so
leaving it out of scope here.
**2. Root-cause description was inaccurate:** Confirmed via
`get_since_until_from_query_object` → `get_since_until_from_time_range` →
`get_since_until(time_range=...)` — the `TEMPORAL_RANGE` filter's `val` never
reaches `parse_human_datetime` directly through this path, so an unnormalized
`[year]` doesn't raise `TimeRangeParseFailError`, it silently returns an
unbounded range. That's arguably worse (silent wrong answer vs. a loud
failure). Corrected both test docstrings and the class docstring to describe
the actual failure mode, and dropped the internal `SC-113648` tracker reference
per your note.
**3. `get_table` / `manage_native_filters` unguarded input:** Agreed this is
pre-existing on master and out of scope for this PR. Not fixing here.
**Shared validator suggestion:** Good idea for a follow-up — rejecting
separator-less values that don't match the recognized prefix grammar (with a
`ValidationError` listing accepted forms) would close the whole open class
instead of allow-listing tokens one at a time, and would benefit `get_table`
too. Deferring to a separate PR since it touches parsing behavior shared well
beyond this tool.
--
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]
