expr: fix non-monotone annotations on timestamp/date/interval functions#36702
expr: fix non-monotone annotations on timestamp/date/interval functions#36702antiguru wants to merge 4 commits into
Conversation
`Interval` is lex-ordered by (months, days, micros), but adding an
interval to a timestamp or date adds *calendar* months with day-clamping
and then days as 24-hour periods. That arithmetic does not respect the
lex order:
t = 2024-01-31
i1 = {0 months, 31 days} → t + i1 = 2024-03-02
i2 = {1 month, 0 days} → t + i2 = 2024-02-29
In lex order `i1 < i2`, yet `t + i1 > t + i2`. For timestamps, the
day-clamping also collapses near-boundary inputs into the same date
while preserving sub-day time, so the first argument is non-monotone
too:
t1 = 2024-01-30 23:59:59, i = {1 month} → 2024-02-29 23:59:59
t2 = 2024-01-31 00:00:00, i = {1 month} → 2024-02-29 00:00:00
For dates the first argument *is* monotone (no sub-day precision means
clamping only collapses, never reverses), but the interval argument has
the same problem.
These annotations are consumed by the abstract interpreter that drives
persist filter pushdown. Marking these functions monotone meant the
interpreter computed the output range by evaluating the function only
at the endpoints of the input interval range — and a stats range like
`[{0m,31d}, {1m,0d}]` would yield the narrow output range
`[2024-02-29, 2024-03-02]` even though interior intervals
(e.g. `{0m, 60d}`) actually produce timestamps far outside that window.
Filter pushdown could then incorrectly conclude a part had no matching
rows, tripping the `persist filter pushdown correctness violation!`
audit in `persist_source.rs`.
Companion fix to b607993, which corrected the analogous annotations
for `add_time_interval`, `sub_time_interval`, `mul_interval`, and
`div_interval`.
Fixes database-issues#9656.
The override was added to suppress the audit panic from database-issues#9656; with the monotonicity annotations on timestamp/date + interval corrected, filter pushdown should be sound again. Removing this lets CI re-exercise the audit and surface any remaining latent causes.
- Drop useless .into() in the new interpret.rs regression test (clippy). - Update filter-pushdown.slt: sub_timestamp_interval is no longer pushdownable now that it's marked non-monotone, so the expected pushdown= lines for 'timestamp - INTERVAL day' queries are gone. This is the known tradeoff for soundness — even day-only intervals share the non-monotonic annotation since the abstract interpreter can't distinguish them statically.
There was a problem hiding this comment.
Amazing! I'll rebase my workload-replay change on top of this and verify: #36686
Also triggered a nightly run: https://buildkite.com/materialize/nightly/builds/16533
| /// the unix epoch depends on the stride magnitude rather than on lex order. | ||
| #[mz_ore::test] | ||
| #[cfg_attr(miri, ignore)] | ||
| fn test_date_bin_timestamp_non_monotone() { |
There was a problem hiding this comment.
Does it actually fail without the PR? The QA LLM review claims not, but I didn't verify:
MEDIUM — test_date_bin_timestamp_non_monotone is a tautology and does not actually regression-test the fix
File: src/expr/src/interpret.rs:1844-1903
The test sets up date_bin(stride_col, 2024-01-01 12:00:00) >= 2024-01-01 00:00:00 with stride_col ∈ [{0,1,0}, {0,2,0}] and asserts that the interpreter admits both True and False. The endpoint evaluations are:
date_bin({0,1,0}, 2024-01-01 12:00) = 2024-01-01 00:00→>=is True.date_bin({0,2,0}, 2024-01-01 12:00) = 2023-12-31 00:00→>=is False.
Because the endpoints already produce opposite boolean answers under the (buggy) (true, true) annotation, the interpreter's union of {False} and {True} already admits both outcomes. The test therefore passes regardless of whether date_bin_timestamp is marked (true, true) or (false, true).
Verified empirically: I reverted src/expr/src/scalar/func.rs:2057 to is_monotone = "(true, true)" while leaving the rest of the PR in place, and ran cargo test --lib -p mz-expr test_date_bin_timestamp_non_monotone — the test still passes.
In contrast, the companion test test_add_timestamp_interval_non_monotone is genuinely diagnostic — reverting the add_timestamp_interval annotation makes it fail with interpreter incorrectly ruled out matching rows.
The in-test comment is also self-contradictory: it asserts "both endpoint evaluations give the same boolean answer" while the lines just above explicitly list one endpoint satisfying >= and the other not.
Impact: The fix to date_bin_timestamp / date_bin_timestamp_tz is correct, but the test that's supposed to lock it in provides false assurance. A later refactor that re-promotes the annotation would not be caught by cargo test -p mz-expr --lib (the PR description points to this command as the verification gate). Given that filter-pushdown correctness bugs are P1/test-blocker (see #9656), losing the regression coverage is a real durability concern.
Suggested fix: Compare against a timestamp strictly between the two endpoint outputs so that the buggy lex-mapping rules out the matching-rows case. For example:
// Endpoint outputs are 2023-12-31 00:00 and 2024-01-01 00:00. An interior
// stride of `{0, 1 day, 12h-worth-of-micros}` bins source to 2024-01-01 12:00,
// which is outside the lex-endpoint box. Comparing against 2024-01-01 12:00:00
// forces the interpreter to (wrongly, under the buggy annotation) rule out
// True.
let expr = MirScalarExpr::column(0)
.call_binary(ts_lit("2024-01-01T12:00:00"), DateBinTimestamp)
.call_binary(ts_lit("2024-01-01T12:00:00"), Gte);
// ... same column setup ...
assert!(
range_out.may_contain(Datum::True),
"date_bin is not monotone in the stride argument; \
interpreter must not rule out matching rows",
);Under (true, true), the lex range [2023-12-31, 2024-01-01] is entirely < 2024-01-01 12:00:00, so the interpreter would only admit False and the may_contain(True) assertion would fail — making it a real regression test. Under the fix's (false, true), the stride flat-map yields anything(), both outcomes are reachable, and the assertion passes.
(Concretely, an interior stride such as {0, 1, 43200000000} — 1 day + 12 hours — does bin 2024-01-01 12:00:00 to itself, so the predicate is genuinely achievable. The test doesn't need to construct that value; it just needs to assert the interpreter doesn't rule out True.)
There was a problem hiding this comment.
Great find! Fixed in the latest commit.
51e86ed to
fa54894
Compare
date_bin(stride, source) = origin + floor((source - origin) / stride) * stride. For a fixed source like 2024-01-01 12:00:00, a 1-day stride bins to 2024-01-01 00:00:00 but a 2-day stride bins to 2023-12-31 00:00:00 — i.e. the lex-larger stride produces an earlier output. Same class of bug as the timestamp/date + interval monotonicity issues. Demotes date_bin_timestamp and date_bin_timestamp_tz from (true, true) to (false, true). Still monotone in source.
fa54894 to
eb53ce5
Compare
|
With this PR I'm still seeing the correctness panic: https://buildkite.com/materialize/release-qualification/builds/1250 I did find another non-monotone function though: #36708 Trying now: https://buildkite.com/materialize/release-qualification/builds/1251 Edit: Still the same panic. |
|
Claude found another class of problem, which is error handling: #36721 |
|
Nice, next try with all 3 PRs included: https://buildkite.com/materialize/release-qualification/builds/1252 Edit: Failed, but only because we run a benchmark against main, and of course it crashes on main! Now trying without main: https://buildkite.com/materialize/release-qualification/builds/1256 Looking much better so far |
Motivation
Fixes database-issues#9656 — the
persist filter pushdown correctness violation!audit panic that has been reproducing underparallel-workload --scenario regression --complexity ddl.Description
Intervalis lex-ordered by(months, days, micros)(derivedOrd), but several scalar functions that consume an interval do calendar-aware arithmetic (variable-length months, day-clamping, bin alignment to the unix epoch) which does not respect that ordering. The interpreter behind persist filter pushdown was marking these as monotone, so it mapped only the endpoints of an interval range — and missed interior values whose function results fall outside the endpoint-bounded box. Filter pushdown then incorrectly concluded the part had no matching rows, tripping the audit panic inpersist_source.rs:625.Bug 1:
add/sub_{timestamp,timestamp_tz,date}_intervalFor
t = 2024-01-31:t + i{0 months, 31 days}2024-03-02{1 month, 0 days}2024-02-29So the smaller interval produces the larger output. For timestamps, day-clamping also collapses near-boundary inputs into the same date while preserving sub-day time, breaking monotonicity in the first argument too:
For dates the first argument is monotone (no sub-day precision means clamping only collapses, never reverses), but the interval argument has the same problem.
Demotions:
add_timestamp_interval,add_timestamp_tz_interval,sub_timestamp_interval,sub_timestamp_tz_interval→(false, false)add_date_interval,sub_date_interval→(true, false)Bug 2:
date_bin_{timestamp,timestamp_tz}date_bin(stride, source) = origin + floor((source - origin) / stride) * stride. For a fixed source like2024-01-01 12:00:00, a 1-day stride bins to2024-01-01 00:00:00but a 2-day stride bins to2023-12-31 00:00:00— the lex-larger stride produces an earlier output, because alignment to the unix epoch depends on stride magnitude, not lex order. Demoted from(true, true)to(false, true)(still monotone in source).Context
This is the companion fix to b607993 ("Mark some interval-related functions as non-monotone"), which corrected the analogous annotations for the
*_time_intervaland*_interval(interval × scalar) variants but left the calendar-aware variants untouched. Together with the cases already fixed in #9301, this completes the sweep of interval-consuming functions whose result doesn't respect interval lex order.The second commit removes the
persist_stats_filter_enabled: "false"override inparallel_workload/settings.pythat was added to suppress this audit panic, so CI re-exercises the audit and surfaces any remaining latent causes.On the "numeric trim" suspicion in the issue thread
I traced every
is_monotoneannotation on numeric functions (arithmetic, casts, rounding, etc.) and they all look correct. I think the audit failures previously attributed to numeric trimming were actually the timestamp/date+interval bug class showing up in mixed expressions — parallel-workload going green with only this fix supports that read.Verification
interpret.rs:test_add_timestamp_interval_non_monotone— constructs(2024-01-31 + interval_col) >= 2024-03-15withinterval_colranging over[{0m,31d}, {1m,0d}]. Under the buggy annotation the interpreter ruled outDatum::True(and so pushdown would have skipped the part); with the fix it correctly admitsTrue.test_date_bin_timestamp_non_monotone— constructsdate_bin(stride_col, 2024-01-01 12:00:00) >= 2024-01-01withstride_colranging over[1 day, 2 days]. Asserts the interpreter admits bothTrue(1-day stride) andFalse(2-day stride).cargo test -p mz-expr --lib— all 50 tests pass locally, including the proptestscalar::func::test::test_is_monotone.Tradeoff and follow-up
The non-monotone marking on
sub_timestamp_intervaland friends is conservative — even a day-only interval shares the annotation with month-bearing intervals because the static interpreter can't distinguish them. As a result,EXPLAIN ... WITH (filter pushdown)no longer reports apushdown=line for predicates liketimestamp_col - INTERVAL '1' day < literal(the slt test in this PR reflects that). That class of predicate was a motivating use case for filter pushdown, so the regression is addressed in #36706, which stacks on this PR and recovers the pushdown via aSpecialBinarydynamic-monotonicity check on the interval value.