BE-500, BE-501: HashQL: Unify mixed parameter resolution in data dependency analysis #8607
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -181,15 +181,31 @@ fn traverse<'heap, A: Allocator + Clone>( | |
| } | ||
| } | ||
|
|
||
| /// Attempts to resolve a block parameter by checking all predecessor values. | ||
| /// | ||
| /// A block parameter may receive values from multiple predecessor blocks, either as | ||
| /// graph edges (when arguments are places) or constant bindings (when arguments are | ||
| /// constants). This function checks whether all non-cyclic predecessors, from both | ||
| /// sources, resolve to the same value. | ||
| /// | ||
| /// # Projection-aware consensus | ||
| /// | ||
| /// When the queried place has a projection suffix (e.g., resolving `x.0` where `x` is a | ||
| /// block parameter), consensus is checked on the *fully resolved* result per predecessor, | ||
| /// not on the partially resolved predecessor bases. This is necessary because different | ||
| /// predecessor locals can still agree on a projected field. | ||
| /// | ||
| /// For example, if predecessor A passes `(42, u)` and predecessor B passes `(42, v)`, | ||
| /// the bases disagree but `A.0 == B.0 == 42`. The algorithm resolves each predecessor | ||
| /// through the full projection suffix before comparing, so this case correctly yields | ||
| /// `Resolved(42)` rather than `Incomplete(x.0)`. | ||
| /// | ||
| /// # Cycle handling | ||
| /// | ||
| /// Cyclic predecessors ([`Backtrack`]) are filtered out before consensus checking. | ||
| /// Since [`Param`] edges are identity transfers, the value is fully determined by | ||
| /// the non-cyclic init edges. If only cyclic predecessors exist (no external source), | ||
| /// the cycle root returns [`Incomplete`] and non-root nodes propagate [`Backtrack`]. | ||
| /// | ||
| /// [`Param`]: EdgeKind::Param | ||
| /// [`Backtrack`]: ResolutionResult::Backtrack | ||
|
|
@@ -198,62 +214,128 @@ fn resolve_params<'heap, A: Allocator + Clone>( | |
| mut state: ResolutionState<'_, '_, 'heap, A>, | ||
| place: PlaceRef<'_, 'heap>, | ||
| ) -> ControlFlow<ResolutionResult<'heap, A>, Local> { | ||
| let graph = state.graph; | ||
|
|
||
| // Check whether graph Param edges exist (cycle detection is only relevant for graph edges, | ||
| // which are the only source of back-edges). | ||
| let has_graph_edges = graph.outgoing_edges(place.local).next().is_some(); | ||
|
|
||
| // Cycle detection: if we've already visited this local, backtrack. | ||
| if has_graph_edges | ||
| && let Some(visited) = &mut state.visited | ||
| && !visited.insert(place.local) | ||
| { | ||
| return ControlFlow::Break(ResolutionResult::Backtrack); | ||
| } | ||
|
|
||
| // Initialize cycle tracking if this is the first Param traversal with graph edges. | ||
| let mut owned_visited = None; | ||
| let visited_ref = if has_graph_edges { | ||
| state.visited.as_deref_mut().or_else(|| { | ||
| let mut set = DenseBitSet::new_empty(graph.graph.node_count()); | ||
| set.insert(place.local); | ||
|
|
||
| owned_visited = Some(set); | ||
| owned_visited.as_mut() | ||
| }) | ||
| } else { | ||
| state.visited.as_deref_mut() | ||
| }; | ||
|
|
||
| let mut rec_state = ResolutionState { | ||
| graph, | ||
| interner: state.interner, | ||
| alloc: state.alloc.clone(), | ||
| visited: visited_ref, | ||
| }; | ||
|
|
||
| // Resolve all predecessor candidates and check consensus. | ||
| // | ||
| // When the queried place has projections (e.g., `x.field`), each predecessor is resolved | ||
| // through the full projection suffix before consensus comparison. If `traverse` returns | ||
| // `Continue(local)` (predecessor base resolved to a bare local), we call `resolve` on | ||
| // `local.projections` to complete the resolution. This ensures consensus is checked on | ||
| // the final value, not intermediate bases that may differ structurally but agree on the | ||
| // projected component. | ||
| // | ||
| // Cyclic predecessors (Backtrack) are skipped: since Param edges are identity transfers, | ||
| // the value is fully determined by the non-cyclic init edges. If only cyclic predecessors | ||
| // exist, we cannot resolve (the value has no external source). | ||
| let graph_edges = graph.outgoing_edges(place.local).map(|edge| { | ||
| let result = traverse(rec_state.cloned(), place, edge); | ||
|
|
||
| match result { | ||
| // Predecessor resolved to a bare local, but the query has remaining projections. | ||
| // Finish resolving through the projection suffix so consensus compares final values. | ||
| ControlFlow::Continue(local) if !place.projections.is_empty() => { | ||
| ControlFlow::Break(resolve( | ||
| rec_state.cloned(), | ||
| PlaceRef { | ||
| local, | ||
| projections: place.projections, | ||
| }, | ||
| )) | ||
| } | ||
| ControlFlow::Continue(_) | ControlFlow::Break(_) => result, | ||
| } | ||
| }); | ||
| let constant_edges = graph | ||
| .constant_bindings | ||
| .iter_by_kind(place.local, EdgeKind::Param) | ||
| .map(|constant| { | ||
| ControlFlow::Break(ResolutionResult::Resolved(Operand::Constant(constant))) | ||
| }); | ||
|
cursor[bot] marked this conversation as resolved.
|
||
|
|
||
| // `try_reduce` returns: | ||
| // `Some(Some(v))` when all predecessors agree on `v` | ||
| // `Some(None)` when the iterator is empty (unreachable: caller guarantees predecessors) | ||
| // `None` when the closure short-circuits (predecessors disagree) | ||
| let mut backtrack_occurred = false; | ||
| let consensus = graph_edges | ||
| .chain(constant_edges) | ||
|
There was a problem hiding this comment. Non-Param edges in consensusMedium Severity Unified   Reviewed by Cursor Bugbot for commit a308b78. Configure here.
Member
Author
There was a problem hiding this comment. Can't happen because of invariance violation. Well typed program (still) |
||
| .filter(|candidate| { | ||
| if matches!(candidate, ControlFlow::Break(ResolutionResult::Backtrack)) { | ||
| backtrack_occurred = true; | ||
| return false; | ||
| } | ||
|
|
||
| true | ||
| }) | ||
| .try_reduce(|lhs, rhs| (lhs == rhs).then_some(lhs)); | ||
|
|
||
| match consensus { | ||
| // Predecessors agree on a value. | ||
| Some(Some(consensus)) => { | ||
| // Clean up visited state before returning. | ||
| if let Some(visited) = state.visited { | ||
| visited.remove(place.local); | ||
| } | ||
|
|
||
| return consensus; | ||
| } | ||
|
|
||
| // All candidates were cyclic (no non-cyclic predecessors to determine the value). | ||
| // If we're not the cycle root, propagate Backtrack so the root can handle it. | ||
| Some(None) if backtrack_occurred && owned_visited.is_none() => { | ||
| if let Some(visited) = &mut state.visited { | ||
| visited.remove(place.local); | ||
| } | ||
|
|
||
| return ControlFlow::Break(ResolutionResult::Backtrack); | ||
| } | ||
| // Pure cycle at root: fall through to Incomplete. | ||
| Some(None) if backtrack_occurred => {} | ||
| Some(None) => unreachable!("caller must guarantee at least one Param predecessor exists"), | ||
| // Predecessors disagree. | ||
| None => {} | ||
| } | ||
|
|
||
| // Clean up visited state before returning incomplete. | ||
| if let Some(visited) = &mut state.visited { | ||
| visited.remove(place.local); | ||
| } | ||
|
|
||
| // Non-cyclic predecessors diverge, or pure cycle at root. | ||
| let mut projections = VecDeque::new_in(state.alloc.clone()); | ||
| projections.extend(place.projections); | ||
|
|
||
|
|
@@ -263,53 +345,17 @@ fn resolve_params<'heap, A: Allocator + Clone>( | |
| })) | ||
| } | ||
|
|
||
| /// Resolves a place to its ultimate data source by traversing the dependency graph. | ||
| /// | ||
| /// Starting from `place`, this function follows edges in the dependency graph to find where | ||
| /// the data ultimately originates. The algorithm handles three types of edges: | ||
| /// | ||
| /// - **[`Load`]**: Always followed transitively (a load has exactly one source) | ||
| /// - **[`Param`]**: Followed only if all predecessors agree on the same source (consensus). | ||
| /// Consensus is checked on fully resolved results: when the queried place has projections, each | ||
| /// predecessor is resolved through the complete projection suffix before comparison. This allows | ||
| /// resolution through Ο-nodes where predecessor bases differ but the projected component agrees | ||
| /// (e.g., `(42, a)` and `(42, b)` agree on field `.0`). | ||
| /// - **[`Index`]/[`Field`]**: Matched against projections to trace through aggregates | ||
| /// | ||
| /// Resolution terminates with: | ||
|
|
@@ -332,13 +378,10 @@ pub(crate) fn resolve<'heap, A: Allocator + Clone>( | |
| mut place: PlaceRef<'_, 'heap>, | ||
| ) -> ResolutionResult<'heap, A> { | ||
| // Scan outgoing edges to find Load and count Param edges. | ||
| let mut params = 0_usize; | ||
| let mut load_edge = None; | ||
|
|
||
| for edge in state.graph.outgoing_edges(place.local) { | ||
| match edge.data.kind { | ||
| EdgeKind::Load => load_edge = Some(edge), | ||
| EdgeKind::Param => params += 1, | ||
|
|
@@ -355,26 +398,15 @@ pub(crate) fn resolve<'heap, A: Allocator + Clone>( | |
| } | ||
|
|
||
| // Attempt to resolve through Param edges, if all predecessors agree. | ||
| // Predecessors may arrive as graph edges (place arguments), constant bindings | ||
| // (constant arguments), or a mix of both. All sources are checked for consensus. | ||
| let has_param_constants = state | ||
| .graph | ||
| .constant_bindings | ||
| .find_by_kind(place.local, EdgeKind::Param) | ||
| .is_some(); | ||
|
|
||
| if params > 0 || has_param_constants { | ||
| place.local = tri!(resolve_params(state.cloned(), place)); | ||
| } | ||
|
|
||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feature flag placed out of alphabetical order
Low Severity
The
iterator_try_reducefeature flag is placed at the end of the library features list, breaking the otherwise alphabetical ordering. It belongs betweeniter_collect_intoandlikely_unlikely. Notably, this same PR correctly movedmaybe_uninit_uninit_array_transposeinto its proper alphabetical position, so the out-of-order placement of the new feature appears to be an oversight.Reviewed by Cursor Bugbot for commit d93d11b. Configure here.