Gate pre-aggregation routing on reported freshness (opt-in) - #2381
Closed
shangyian wants to merge 2 commits into
Closed
Gate pre-aggregation routing on reported freshness (opt-in)#2381shangyian wants to merge 2 commits into
shangyian wants to merge 2 commits into
Conversation
DJ stored an external pipeline's `valid_through_ts` for each pre-aggregation and exposed it over the API, but nothing consulted it when deciding whether that pre-agg could answer a query. A pre-agg whose pipeline died kept serving silently truncated numbers. The gate compares the watermark against the time range the query itself asks for rather than against wall clock, so a report bounded at last March still reads an aggregate that stopped updating months ago, while a query reaching past the watermark falls back to source. Both sides of the comparison stay in the partition format, so `valid_through_ts` is never reinterpreted as a point in time. A query with no discoverable upper bound implicitly asks for data through the present; those are allowed unless a wall-clock staleness budget is also configured, in which case "now minus the budget" is rendered into the partition format and compared. Off by default behind `preagg_freshness_gating`, since turning it on can route queries away from pre-aggs that serve them today. The extra eager loading the gate needs is only added to the pre-agg query when it is enabled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for thriving-cassata-78ae72 canceled.
|
Merged
3 tasks
The gate was 271 lines for what is one comparison. Most of the excess came from re-implementing machinery that already existed and from supporting a case that cannot arise. Shares the traversal. temporal_grain_refs was a second copy of the temporal column to grain matching in get_temporal_partitions, and had already drifted: it implemented two of that function's three strategies, so a pre-agg whose temporal axis reaches the grain only through the column's own dimension reference was never gated at all. Both now go through match_temporal_columns_to_grain, which also collapses three copies of the output-name derivation in get_temporal_partitions into one. Collapses the multi-axis support. A pre-agg reports one watermark, so bounds read off two axes could be in different partition formats and compared against a watermark in only one of them -- and the wall-clock branch already gave up and used the first format. Now judged only when exactly one temporal column is in the grain, matching the collapse combiner construction already makes. Reads the filters once. Bound extraction moves to filters.py as upper_bounds_by_ref, memoized per node revision on the build context: it was re-parsing every filter once per temporal ref per candidate pre-agg. It also reads reversed operands now, so `20250101 >= date_id` is a bound rather than silently unbounded, and it uses the public get_column_full_name instead of filters.py's private _extract_full_column_ref. Drops the eager-load branch in load_available_preaggs. Every relationship it added is already loaded on the same objects by batch_load_nodes_with_dependencies, and Column.partition is lazy="joined", so it bought two discarded round trips and re-selected the large query column that the main loader deliberately restricts away. Says when it cannot judge. valid_through_ts has no agreed encoding in this repo -- partition format, epoch seconds and epoch milliseconds all appear -- so a deployment following the documented epoch contract would compare 20250101 against 1.7e12, pass everything forever, and report nothing. An order-of-magnitude mismatch now logs and declines. Also: render_partition_value moves to models/partition.py beside the SQL renderer and rejects formats whose token order is not chronological (ddMMyyyy); the wall clock is read once per build from ctx.build_timestamp rather than per pre-agg; and the tests use a real Settings instance, so renaming a setting fails loudly instead of leaving them green while the feature stops working. Net: 271 -> 43 statements in the gate, and Strategy 3 is covered by tests rather than pragma'd. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
DJ stores the
valid_through_tsan external pipeline reports for a pre-aggregation and exposes it over the API and GraphQL, but nothing ever consulted it when deciding whether that pre-aggregation may answer a query. A pre-agg whose pipeline died months ago kept serving queries silently. This adds gating, off by default.The rule: compare against the query's own temporal filter, not the wall clock. A query for last March doesn't care that a table stopped updating yesterday, so a wall-clock staleness budget rejects pre-aggs that are perfectly valid for the range requested.
There's a second, stronger reason to avoid wall-clock as the primary rule:
valid_through_ts's encoding is not agreed on inside this repo. The docs describe it as the table's partition format (20260721),models/preaggregation.pydocuments it as "Timestamp (epoch)", the GraphQL scalar comments mention millisecond timestamps, and test fixtures use all three. A wall-clock budget has to interpret that integer as a point in time. Comparing against a filter literal never interprets it — it only compares two values written in the same encoding, whatever that encoding is.For a query with no upper bound (the actual dead-pipeline case), a wall-clock budget does apply, but still without parsing the watermark: it renders
now - budgetinto the partition format and compares. Formats it can't render decline to judge.Where it gates
In
load_available_preaggs, alongside the existing availability filter — not inAvailabilityState.is_available(), which has no access to the build context and so can't see the query's temporal range. That TODO stays open.Configuration
Both settings preserve today's behavior by default:
preagg_freshness_gating: bool = False— master switch.preagg_max_staleness_seconds: Optional[int] = None— budget for open-ended queries only;Nonemeans they're never rejected on staleness.Deliberately conservative elsewhere: filters containing
ORor a negatedBETWEENare ignored rather than trusted,<lowers the bound by one, a pre-agg with no temporal grain is never judged, and if any temporal axis is unbounded the whole query is treated as open-ended. The extra eager loading is added to the pre-agg query only when the gate is on, so the default path's query cost is unchanged.Behavior on rejection
A rejected pre-agg falls back to source, exactly like any other non-match — results stay correct, just slower. That's the right default; failing the query would be worse than what we have today.
The signal that matters is operational rather than per-caller: a pre-agg rejected for staleness on every query means a broken pipeline, and that belongs in monitoring. The
logger.infoinpreagg_is_freshis the hook. Not included in this PR — flagging it as the natural follow-up rather than building a new surfacing mechanism here.Test Plan
Both touched modules at 100% statement and branch coverage.
pre-commit run --from-ref main --to-ref HEADclean including mypy.make checkpassesmake testshows 100% unit test coverageDeployment Plan
No migration. Off by default, so deploying changes nothing until
preagg_freshness_gatingis enabled.Note for reviewers
Touches
docs/content/0.1.0/docs/dj-concepts/aggregate-awareness.md, which conflicts with #2380. Merge #2380 first and I'll rebase this. #2380's "freshness doesn't gate routing yet" bullet stays accurate for the default configuration and needs amending rather than removing.The
valid_through_tsencoding disagreement described above is worth fixing separately — the design here works around it rather than resolving it, and the field descriptions are wrong per the docs.