Repository navigation
Conversation
zzylol
force-pushed
the
refactor/1207-properties
branch
2 times, most recently
from
November 6, 2026 19:12
60a16ee to
107a77b
Compare
… PR 2) Memo::find_or_add_group hash-conses on a group's expression (operator, arguments, input groups; stages::hash_consing) and stores the schema and result kind the node rules derive; Memo::add_candidate checks a candidate by the same rules, requires the group's output, drops a duplicate (same expression and logical properties) and keeps parents and queries up to date; Memo::add_query and Memo::logical_node_trees are filled in. Stage 0 builds its groups bottom-up through find_or_add_group and adds each query with add_query.
zzylol
force-pushed
the
refactor/1207-memo
branch
from
November 6, 2026 19:14
9e7e404 to
7ea9152
Compare
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
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.
Why
The concepts doc (section 3) says that rules add groups only through
Memo::find_or_add_groupand candidates only throughMemo::add_candidate. Rules match group expressions, never add the same expression/property pair twice, and never copy consumers to make a new producer visible. Pass 1 (PR 3) and Pass 2 (PR 4) are built on these two methods, and PR 0 left both astodo!().Stage 0 had its own node-level hash-consing (
cse.rs). Its equality compared schema, result kind, guarantee and children. A group lookup by expression would have been a second equality, and the two could disagree. #1207 asks for one equality.What
Memo::find_or_add_group(expression)hash-conses on the operator, its arguments and its input group ids. If an equal expression already has a group, it returns that group. Otherwise it adds a group whose logical node is the operator over the input groups' logical nodes, with the derived schema, and lists the new group as a parent of each input group. θ is not an argument of any operator (PR 0 removed it fromFieldDataType::Sketch), so builds of one family over one input that differ only in θ are one group.Memo::add_candidate(group, candidate)appends the candidate toMemoGroup::candidatesand returnsCandidateForMemoGroupId { group, index_in_group }. If the group already has a candidate with the same expression and the same logical properties, it returns that candidate's id and drops the new one. A new candidate makesgroupa parent of each group it reads. The group's queries are added to those groups and to every group below them.Memo::candidate(id)is unchanged (PR 1 filled it in).asap-stage0-memo):memo_of_queriesnow visits each node of the queries' DAGs once, children first, and callsfind_or_add_groupon its operator over its children's groups. It then lists, for each group, the queries that reach it.cse.rs(share_common_sub_dags, its intern table and hash caches) is deleted, together with the crate'stest-supportfeature.share_identicalanddistinct_nodesinintegration-tests/tests/planner_layering_common(#[allow(dead_code)], no caller).Decisions and pending items
PartialEq, confirmed by equal JSON, so signed zeros stay apart and aNaNnever matches) over the same input groups at the same positions. Schema, result kind and guarantee are not compared: the schema is derived from the expression, and the guarantee is a candidate's (from θ), not the group's.LIMIT,Dedupon some columns, row-position window functions;NonASAPOp::is_shareable_when_equal)DISTINCTover two unorderedLIMIT 10s). Stage 0's old node equality compared children by value and made them one node. Both are correct: the new rule shares less, and never gives a wrong answer.==. The duplicate'sinput_requirements,fused_groupsandoriginare not compared, and are dropped with it.parentsholdsfind_or_add_group, Stage 0) or through one of their candidates (add_candidate), each listed once.fused_groupsare not reads.queriesholdsadd_candidateextends it from the candidate's group to the groups the candidate reads and everything below them, through child groups and candidates' inputs.fused_groupsget no queries.find_or_add_grouppanics (# Panicsin its doc). The doc's signature returnsMemoGroupId, not aResult, and no rule may build such an expression. No frontend DAG in the test suites hits it (see "Test status").OperatorNode::new), no guarantee. The frontends build their nodes the same way (none callswith_schema), so Stage 0's nodes are unchanged except that a guarantee is no longer kept. The frontends set none.New abstractions not in #1207 or the doc (pending approval)
Memo::groups_by_expression_hash: HashMap<u64, Vec<MemoGroupId>>andMemo::indexed_group_count: usize, and privateMemo::index_groups_pushed_directlytypes/src/stages/candidates.rsO(1)expected per call, not a scan of every group.groupsstays public as in the doc, so a group pushed onto it directly is indexed at the nextfind_or_add_group. Because of these fields,Memocan no longer be built with a struct literal outside its module; the one test that did (physical/memo_candidates.rs) now starts fromMemo::default().stages::hash_consing:same_expression,expression_hash,same_value,shareable_when_equaltypes/src/stages/hash_consing.rsOperator<MemoGroupId>), not nodes, so it needs no node table or pointer caches.pub(super) fn MemoGroup::expression(&self) -> Operator<MemoGroupId>types/src/stages/memo.rslogical_nodeandchild_groups, for comparing against an indexed group. The doc keeps the group as node + child list (a simplification it does not adopt).add_parent,same_expression_and_logical_properties,Memo::add_queries_belowtypes/src/stages/candidates.rsparentsandqueriesup to date; find a duplicate candidate.GroupOfNode,group_ofstage0-memo/src/lib.rsscan,kll_state,quantile_readout,unordered_limit,candidate,kll_k(memo_tests);node,scan,quantile_agg(_over),compare,memo_of,root,one_group,scalar_of_vector,summary_agg,unordered_limit,literal(Stage 0 tests)candidatebuildsLogicalPropertiesdirectly because the doc example's k = 200 and k = 345 are not in today's KLL list.How
find_or_add_groupfirst indexes any groups pushed directly. It then builds the expression's logical node (needed for the schema and for the legality check, which reads aLIMIT's childSortor aDedup's child schema). If the node is shareable, it hashes the operator's arguments (as JSON, input groups replaced by()) together with the input group ids, and compares the groups in that bucket withsame_expression. On a miss, it pushes the group, adds it to its inputs'parents, and indexes it only if it is shareable.add_candidatescans the group's own candidates for a duplicate; a group has few candidates. On a miss, it pushes the candidate, callsadd_parentfor each input group, and walks downward from the inputs with a worklist, adding the group's queries in sorted position. The walk does not descend from a group that already had every query, because the groups below it have them too.group_ofmaps each originalRc<OperatorNode>by address to its group, so a diamond is visited once. It recurses into the children throughOperator::map_childrenand callsfind_or_add_group. Each query then adds its index to every group reachable from its root groups, in query order, so the lists come out ascending without sorting.Major algorithm changes
Stage 0 hash-consing.
cse::same_node: schema, result kind, guarantee, own fields, children by pointer and then by value) and the group lookup the rules need. A Pass 1 rule addingSummaryAgg[KLL](G1)could fail to find Stage 0's equal group whenever the two equalities disagreed, for example on the guarantee.share_common_sub_dagsinterns nodes bottom-up into anRctable (SipHash of the node's JSON, thensame_node).numberassigns group ids to the interned nodes.pairmaps the original nodes to those groups.group_ofrecursively callsMemo::find_or_add_group(operator over child groups).cse.rsat the memo level.O(N)hashing, made linear by pointer caches, plussame_nodeper bucket hit. After:O(N)expected time forNnodes. Each lookup serializes only the node's own arguments, since its children areu32ids, and needs no caches. Comparing against a bucket entry isO(arguments)and never recurses: the old 24-level diamond guard test has nothing left to guard.Key code interfaces
Bodies are elided as
;. Only the signatures marked// newor// removedchanged; the others are PR 0's with bodies filled in.crates/types/src/stages/candidates.rscrates/types/src/stages/memo.rscrates/stage0-memo/src/lib.rs,crates/stage0-memo/Cargo.tomlUsage
Concepts doc, section 1, G1 and G2. Planning past Stage 0 still stops at PR 3's
todo!(), so this flow is the onememo_tests::kll_builds_with_k_200_and_k_345_share_one_groupand Stage 0's tests run.Before / After
KLL example (doc section 1): G1 =
Scan latency[1m], G2 =SummaryAgg[KLL](G1)with k = 200 and k = 345.find_or_add_grouppanics (todo!("#1207 PR 2"))add_candidatepanicsindex_in_group0 and 1cse::same_node(schema, result kind, guarantee, children), a second equalityfind_or_add_group's, the only oneTest status
New tests, each stating the behavior it checks in its doc line:
asap-types,stages::candidates::memo_tests(8):find_or_add_group_returns_the_group_of_an_equal_expression,…_separates_equal_operators_over_different_inputs,…_never_returns_an_unshareable_group,…_lists_the_new_group_among_its_inputs_parents,…_finds_a_group_pushed_directly,kll_builds_with_k_200_and_k_345_share_one_group,add_candidate_returns_the_existing_candidate_for_a_duplicate,add_candidate_maintains_parents_and_queries.asap-stage0-memo(12):two_queries_sharing_a_sub_expression_get_one_group(also checks parents and queries),a_query_s_repeated_sub_expression_is_one_group,an_already_shared_node_is_one_group,aggregates_that_differ_in_an_argument_are_different_groups,equal_keyless_nodes_that_return_the_same_rows_are_one_group,differing_accuracy_targets_are_two_groups_over_one_scan_group,a_scalar_referenced_vector_is_one_group_across_queries,readouts_share_their_state_group_but_not_each_other,dedup_is_one_group_only_on_every_column,unordered_limits_and_their_parents_are_different_groups,limits_over_a_sort_with_their_partitions_are_one_group,signed_zero_and_non_finite_literals_are_different_groups.cse.rs's 24 tests are not carried over one for one:structural_hashcache tests are dropped, because the caches are gone.shared_root_value_is_unchanged,asap_nodes_share_without_a_unique_keyandasap_nodes_with_distinct_parameters_are_shared_but_distinct_guarantees_are_notare dropped, because guarantees are no longer part of a group. θ sharing is now tested inkll_builds_with_k_200_and_k_345_share_one_group.shared_diamond_does_not_expand_during_comparisonis dropped, because comparison no longer recurses.None of these tests depends on another stage crate;
asap-stage0-memohas no dev-dependencies.asap-typesmemo_tests)asap-accuracyasap-stage0-memo--features asap-types/test-support;cargo test -p asap-stage0-memoalone did not compile (fixtures calledSchema::has_unique_key)crate_graph(asap-integration-tests --test integration crate_graph)asap-planner,asap-executor,asap-devtools,asap-integration-tests(all targets)todo!()s: PR 3 (Pass 1, 81), PR 4 (45 + 13), PR 6 (36 + 13 + 7), … No failure is in Stage 0 orfind_or_add_group.Checks:
cargo check --workspace --all-targets,cargo clippy --workspace --all-targets -D warningsandcargo fmt --all --checkare clean.