Fix non-deterministic iteration in SessionStateBuilder

[shehab-ali]

Which issue does this PR close?

Problem: SessionStateBuilder::new_from_existing() previously used HashMap::into_values().collect_vec() for scalar, aggregate, and window functions. Since the HashMap stores entries for both the canonical name and each alias (all pointing to the same Arc<UDF>), into_values() produced duplicate entries in arbitrary order. When build() re-registered them, the last-writer-wins behavior on shared alias keys was nondeterministic.

Fix: Introduced dedup_function_registry_by_canonical_name() which:

Iterates HashMap keys in sorted order (deterministic)
Keeps only one Arc<UDF> per unique canonical name (no duplicates)
This ensures build() re-registers each function exactly once, making the round-trip through new_from_existing() → build() deterministic and alias-preserving.

Say you register a UDF named "postgres_to_char" with alias "to_char". The SessionState HashMap stores:
"postgres_to_char" → Arc<UDF(postgres_to_char)>
"to_char"          → Arc<UDF(postgres_to_char)>   // same arc, alias entry

Before the fix — into_values().collect_vec() produces:

// Two copies of the same UDF, in arbitrary HashMap iteration order
[Arc<UDF(postgres_to_char)>, Arc<UDF(postgres_to_char)>]

build() then calls register_udf() for each entry. Each register_udf call re-inserts both the canonical name and all aliases:

// First registration (say the "to_char" entry came first):
"postgres_to_char" → Arc_1
"to_char"          → Arc_1

// Second registration (the "postgres_to_char" entry):
"postgres_to_char" → Arc_2   // overwrites Arc_1
"to_char"          → Arc_2   // overwrites Arc_1

If `with_updated_config()` returns `Some` (which creates a new Arc each call), 
Arc_1 and Arc_2 are different objects for the same logical function. 
Which one ends up stored depends on HashMap iteration order — which is random per run. 
Scale this to many UDFs with aliases and the rebuilt state becomes non-deterministic.

After the fix — filtering to key == canonical_name produces:

// Only the canonical entry, no duplicates
[Arc<UDF(postgres_to_char)>]

Bug Example

Problem: When SessionState is rebuilt via SessionStateBuilder::new_from_existing(), a user's UDF override can be silently reverted, causing the wrong function to execute at query time.

Concrete example: a user registers postgres_to_char with alias "to_char" to override the built-in to_char. Queries using to_char(...) correctly call the custom implementation. But when any code path rebuilds the session state, HashMap::into_values() emits both the custom UDF and the built-in (which survived under its "date_format" alias key). build() re-registers both, and last writer wins. If the built-in happens to register last, "to_char" now points back to the built-in, and subsequent queries silently call the wrong function. This happens nondeterministically depending on HashMap iteration order.

What changed: the filter key.as_str() == udf.name() only keeps entries where the HashMap key is the UDF's canonical name. This drops the built-in because its canonical key ("to_char") was overwritten by the custom UDF; it only exists under "date_format", which doesn't match its canonical name "to_char". So the built-in is never passed to build(), and the custom UDF's override is preserved.

Are these changes tested?

Added unit test

Are there any user-facing changes?

No

[alexanderbianchi]

Do we need canonical_name as it's own function if all the uses are the same function?

[shehab-ali]

those function have different types though: WindowUDF, ScalarUDF, AggregateUDF

[alexanderbianchi]

ah I see now, my bad.

[ahmed-mez]

Thank you for fixing this! I hope the maintainers review this soon.

[ahmed-mez]

minor: I think the function could consume the map instead of borrowing it, to avoid some arc clones

  fn dedup_function_registry_by_canonical_name<T>(
      map: HashMap<String, Arc<T>>,
      canonical_name: impl Fn(&T) -> &str,
  ) -> Vec<Arc<T>> {
      map.into_iter()
          .filter(|(key, udf)| key.as_str() == canonical_name(udf.as_ref()))
          .map(|(_, udf)| udf)  // no Arc::clone needed
          .collect()
  }

[ahmed-mez]

Alternatively, it might be more robust to dedup by identity

  fn dedup_by_identity<T>(map: HashMap<String, Arc<T>>) -> Vec<Arc<T>> {
      let mut seen = HashSet::new();
      map.into_values()
          .filter(|arc| seen.insert(Arc::as_ptr(arc)))
          .collect()
  }

[Jefffrey]

Thanks for the description of the bug that's being encountered here. However, I'm having trouble understanding the implications. Is it a potential performance issue, if we have a massive number of UDFs? There's mention of it not being deterministic but what knock-on effects would this have? I tried out the test introduced here but it passes on main, so I want to understand what exactly this is fixing.

[shehab-ali]

The bug showed up when SessionState is rebuilt via SessionStateBuilder::new_from_existing(), a user's UDF override can be silently reverted, causing the wrong function to execute at query time.

Example: a user registers postgres_to_char with alias "to_char" to override the built-in to_char. Queries using to_char() correctly call the custom implementation. But when any code path rebuilds the session state, HashMap::into_values() emits both the custom UDF and the built-in (which survived under its "date_format" alias key). build() re-registers both, and last writer wins. If the built-in happens to register last, "to_char" now points back to the built-in, and subsequent queries silently call the wrong function. This happens nondeterministically depending on HashMap iteration order.

With these changes, the filter key.as_str() == udf.name() only keeps entries where the HashMap key is the UDF's canonical name. This drops the built-in because its canonical key ("to_char") was overwritten by the custom UDF; it only exists under "date_format", which doesn't match its canonical name "to_char". So the built-in is never passed to build(), and the custom UDF's override is preserved.

[Jefffrey]

Thanks for the example. I was able to reproduce it via modifying the test included here to have two registered UDFs as described. It would be good to enhance the test introduced here to clearly reproduce the error (even if it happens randomly).

With these changes, the filter key.as_str() == udf.name() only keeps entries where the HashMap key is the UDF's canonical name. This drops the built-in because its canonical key ("to_char") was overwritten by the custom UDF; it only exists under "date_format", which doesn't match its canonical name "to_char". So the built-in is never passed to build(), and the custom UDF's override is preserved.

Is this dropping of date_format intentional? It doesn't seem ideal since the roundtripped session state will now be different from the original.