fix: reproduce function registry deterministically in SessionStateBuilder::new_from_existing#23850
fix: reproduce function registry deterministically in SessionStateBuilder::new_from_existing#23850tohuya6 wants to merge 1 commit into
Conversation
…lder::new_from_existing `SessionState` registers every UDF under its canonical name and each of its aliases, so a single `Arc` is reachable from multiple keys in the scalar, aggregate, window, and higher-order registries. `new_from_existing` flattened those registries with `HashMap::into_values()`, which yields the shared `Arc`s duplicated and in non-deterministic iteration order. On `build()` the functions are re-registered last-writer-wins, so which one ends up owning a contested alias depended on HashMap ordering: a user's alias override of a builtin (for example a custom `to_char`) could silently revert, and do so differently from run to run. Reconstruct a faithful registration order instead. Because the registry was produced by some real registration sequence, an order that reproduces it always exists. `deterministic_registration_order` dedups entries by `Arc` identity and topologically sorts them under a single rule -- a key's owner must register after every other function that also claims that key by name or alias -- breaking ties by canonical name. Any valid order reproduces the exact map, so the roundtrip is now both deterministic and override-preserving. - add the `AliasedRegistryEntry` trait and `deterministic_registration_order` helper, and use them for the scalar / aggregate / window / higher-order registries in `new_from_existing` - add regression tests: an alias override survives the roundtrip on every run, the alias-chain counterexample from apache#21262 roundtrips exactly, and a unit test that the helper's reconstructed order replays to the input map Closes apache#23697.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23850 +/- ##
==========================================
- Coverage 80.77% 80.75% -0.02%
==========================================
Files 1089 1089
Lines 369082 369309 +227
Branches 369082 369309 +227
==========================================
+ Hits 298134 298250 +116
- Misses 53196 53297 +101
- Partials 17752 17762 +10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
I wonder if it would be net simpler to split the registry data into two fields. For example:
Then |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks @tohuya6 -- I agree with @neilconway that this feels overly complicated for the problem it is trying to solve. I have some thoughts -- let us know
| /// order dependence surfaces. | ||
| #[test] | ||
| fn test_from_existing_preserves_alias_override() -> Result<()> { | ||
| for _ in 0..64 { |
There was a problem hiding this comment.
why do we need to do this 64 times?
| query_planner: Some(existing.query_planner), | ||
| catalog_list: Some(existing.catalog_list), | ||
| table_functions: Some(existing.table_functions), | ||
| scalar_functions: Some(existing.scalar_functions.into_values().collect_vec()), |
There was a problem hiding this comment.
How about potentially aligning builder to use a HashMap too (rather than a Vec) so that we could just copy the existing state to the builder?
| } | ||
| } | ||
|
|
||
| /// Flatten a function registry into its distinct functions, ordered so that |
There was a problem hiding this comment.
If the problem is the non determinism of reregistration, perhaps we could do something simpler like sort the values by alias name so the result is deterministic?
I haven't read this implementation carefully but it seems pretty complicated semantically ti try and reverse engineer the aliasing
4 participants
Which issue does this PR close?
Rationale for this change
SessionStateregisters every UDF under its canonical name and each of its aliases, so a singleArcis reachable from multiple keys in the scalar, aggregate, window, and higher-order registries.SessionStateBuilder::new_from_existingflattened those registries withHashMap::into_values(), which yields the sharedArcs duplicated and in non-deterministic iteration order.On
build()the functions are re-registered last-writer-wins, so which function ends up owning a contested alias depended on HashMap ordering. A user's alias override of a builtin (for example a customto_char) could silently revert — and do so differently from run to run.What changes are included in this PR?
Reconstruct a faithful registration order instead of relying on HashMap iteration order. Because the registry was produced by some real registration sequence, an order that reproduces it always exists.
AliasedRegistryEntrytrait and adeterministic_registration_orderhelper. The helper dedups entries byArcidentity and topologically sorts them under a single rule — a key's owner must register after every other function that also claims that key by name or alias — breaking ties by canonical name.new_from_existing. Any valid order reproduces the exact map, so the roundtrip is now both deterministic and override-preserving.Are these changes tested?
Yes. New regression tests cover:
Are there any user-facing changes?
No API changes. This fixes a correctness bug: alias overrides now survive
SessionStateBuilder::new_from_existingdeterministically.