You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "shehab-ali (via GitHub)" <gi...@apache.org> on 2026/03/30 19:47:52 UTC
[PR] Fix non-deterministic iteration in SessionStateBuilder [datafusion]
shehab-ali opened a new pull request, #21262:
URL: https://github.com/apache/datafusion/pull/21262
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
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.
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix non-deterministic iteration in SessionStateBuilder [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #21262:
URL: https://github.com/apache/datafusion/pull/21262#issuecomment-4949588775
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix non-deterministic iteration in SessionStateBuilder [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #21262:
URL: https://github.com/apache/datafusion/pull/21262#issuecomment-5014673377
i've raised an issue for tracking this bug since the PR went stale: https://github.com/apache/datafusion/issues/23697
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix non-deterministic iteration in SessionStateBuilder [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #21262:
URL: https://github.com/apache/datafusion/pull/21262#issuecomment-4266411930
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix non-deterministic iteration in SessionStateBuilder [datafusion]
Posted by "shehab-ali (via GitHub)" <gi...@apache.org>.
shehab-ali commented on PR #21262:
URL: https://github.com/apache/datafusion/pull/21262#issuecomment-4408757373
@Jeff
Updated it. Lmk what you think
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix non-deterministic iteration in SessionStateBuilder [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #21262:
URL: https://github.com/apache/datafusion/pull/21262#issuecomment-4273032887
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix non-deterministic iteration in SessionStateBuilder [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #21262:
URL: https://github.com/apache/datafusion/pull/21262#issuecomment-4324527637
That could be worth investigating, if we can achieve that without any knock-on effects or breaking changes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix non-deterministic iteration in SessionStateBuilder [datafusion]
Posted by "alexanderbianchi (via GitHub)" <gi...@apache.org>.
alexanderbianchi commented on code in PR #21262:
URL: https://github.com/apache/datafusion/pull/21262#discussion_r3012018900
##########
datafusion/core/src/execution/session_state.rs:
##########
@@ -978,6 +978,20 @@ impl SessionState {
/// A builder to be used for building [`SessionState`]'s. Defaults will
/// be used for all values unless explicitly provided.
///
+/// Deduplicates function-registry map entries by keeping only entries whose key
Review Comment:
looks like you break up the comment above from the `SessionStateBuilder` it maps to
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix non-deterministic iteration in SessionStateBuilder [datafusion]
Posted by "alexanderbianchi (via GitHub)" <gi...@apache.org>.
alexanderbianchi commented on code in PR #21262:
URL: https://github.com/apache/datafusion/pull/21262#discussion_r3012026432
##########
datafusion/core/src/execution/session_state.rs:
##########
@@ -978,6 +978,20 @@ impl SessionState {
/// A builder to be used for building [`SessionState`]'s. Defaults will
/// be used for all values unless explicitly provided.
///
+/// Deduplicates function-registry map entries by keeping only entries whose key
+/// matches the canonical name. The session stores one hash map entry per alias
+/// plus the canonical name; filtering to canonical-name entries yields exactly
+/// one [`Arc`] per logical function.
+fn dedup_function_registry_by_canonical_name<T>(
+ map: &HashMap<String, Arc<T>>,
+ canonical_name: impl Fn(&T) -> &str,
Review Comment:
Do we need `canonical_name` as it's own function if all the uses are the same function?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix non-deterministic iteration in SessionStateBuilder [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on PR #21262:
URL: https://github.com/apache/datafusion/pull/21262#issuecomment-5041179960
If we need to preserve insertion order why not just use indexmap?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix non-deterministic iteration in SessionStateBuilder [datafusion]
Posted by "shehab-ali (via GitHub)" <gi...@apache.org>.
shehab-ali commented on PR #21262:
URL: https://github.com/apache/datafusion/pull/21262#issuecomment-4307747494
```
scalar_functions_map: Option<HashMap<String, Arc<ScalarUDF>>>,
aggregate_functions_map: Option<HashMap<String, Arc<AggregateUDF>>>,
window_functions_map: Option<HashMap<String, Arc<WindowUDF>>>,
```
What if we add and pass three function HashMaps directly through the builder with those ^ three new private hash maps. In `build()`, these are assigned straight to the session state, bypassing `register_udf()` entirely. Keys are never re-derived, so whatever mapping was in the original `SessionState`, including chains of overrides and surviving alias entries would be preserved exactly
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix non-deterministic iteration in SessionStateBuilder [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #21262: Fix non-deterministic iteration in SessionStateBuilder
URL: https://github.com/apache/datafusion/pull/21262
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix non-deterministic iteration in SessionStateBuilder [datafusion]
Posted by "shehab-ali (via GitHub)" <gi...@apache.org>.
shehab-ali commented on PR #21262:
URL: https://github.com/apache/datafusion/pull/21262#issuecomment-4408760921
@Jefffrey updates it. Lmk what you think
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix non-deterministic iteration in SessionStateBuilder [datafusion]
Posted by "shehab-ali (via GitHub)" <gi...@apache.org>.
shehab-ali commented on code in PR #21262:
URL: https://github.com/apache/datafusion/pull/21262#discussion_r3012109374
##########
datafusion/core/src/execution/session_state.rs:
##########
@@ -978,6 +978,20 @@ impl SessionState {
/// A builder to be used for building [`SessionState`]'s. Defaults will
/// be used for all values unless explicitly provided.
///
+/// Deduplicates function-registry map entries by keeping only entries whose key
+/// matches the canonical name. The session stores one hash map entry per alias
+/// plus the canonical name; filtering to canonical-name entries yields exactly
+/// one [`Arc`] per logical function.
+fn dedup_function_registry_by_canonical_name<T>(
+ map: &HashMap<String, Arc<T>>,
+ canonical_name: impl Fn(&T) -> &str,
Review Comment:
those function have different types though: `WindowUDF, ScalarUDF, AggregateUDF`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix non-deterministic iteration in SessionStateBuilder [datafusion]
Posted by "shehab-ali (via GitHub)" <gi...@apache.org>.
shehab-ali commented on PR #21262:
URL: https://github.com/apache/datafusion/pull/21262#issuecomment-4268903339
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix non-deterministic iteration in SessionStateBuilder [datafusion]
Posted by "shehab-ali (via GitHub)" <gi...@apache.org>.
shehab-ali commented on PR #21262:
URL: https://github.com/apache/datafusion/pull/21262#issuecomment-4300376278
This is not intentional. You are right; this is a concern.
How about we instead of blindly dumping all values, we deduplicate by Arc pointer and split UDFs into two groups:
- those that were displaced from their canonical name key by another UDF (like the built-in that lost "to_char" to the custom)
- those that still own their canonical name key (like the custom).
We emit displaced first, owners last. So this guarantees that when build() re-registers them, owners always write last and their overrides are preserved deterministically, regardless of HashMap iteration order.
`
/// Collects unique UDFs from a function registry map, ordered so that
/// [`SessionStateBuilder::build`] reproduces the original [`HashMap`] state exactly.
///
/// The map stores one entry per canonical name and one per alias, all pointing to the same
/// [`Arc`]. We deduplicate by pointer and emit in two passes:
///
/// - **Displaced** (their canonical name key was taken by another UDF): register first so their
/// entries can be overwritten.
/// - **Owners** (still own their canonical name key): register last so their overrides survive.
///
/// This ensures that if a user registered a custom UDF as an alias override (e.g.
/// `postgres_to_char` with alias `"to_char"`), the override is preserved after a roundtrip
/// through [`SessionStateBuilder::new_from_existing`].
fn udfs_for_roundtrip<T>(
map: &HashMap<String, Arc<T>>,
canonical_name: impl Fn(&T) -> &str,
) -> Vec<Arc<T>> {
let mut seen: HashSet<*const T> = HashSet::new();
let mut displaced: Vec<Arc<T>> = Vec::new();
let mut owners: Vec<Arc<T>> = Vec::new();
for udf in map.values() {
if seen.insert(Arc::as_ptr(udf)) {
let owns_key = map
.get(canonical_name(udf.as_ref()))
.is_some_and(|u| Arc::ptr_eq(u, udf));
if owns_key {
owners.push(Arc::clone(udf));
} else {
displaced.push(Arc::clone(udf));
}
}
}
displaced.extend(owners);
displaced
}
`
then we can set each function to
`
scalar_functions: Some(udfs_for_roundtrip(&existing.scalar_functions, |u| {
u.name()
})),
...
`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix non-deterministic iteration in SessionStateBuilder [datafusion]
Posted by "ahmed-mez (via GitHub)" <gi...@apache.org>.
ahmed-mez commented on code in PR #21262:
URL: https://github.com/apache/datafusion/pull/21262#discussion_r3052789266
##########
datafusion/core/src/execution/session_state.rs:
##########
@@ -975,6 +975,20 @@ impl SessionState {
}
}
+/// Deduplicates function-registry map entries by keeping only entries whose key
+/// matches the canonical name. The session stores one hash map entry per alias
+/// plus the canonical name; filtering to canonical-name entries yields exactly
+/// one [`Arc`] per logical function.
+fn dedup_function_registry_by_canonical_name<T>(
Review Comment:
Alternatively, it might be more robust to dedup by identity
```rust
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()
}
```
##########
datafusion/core/src/execution/session_state.rs:
##########
@@ -975,6 +975,20 @@ impl SessionState {
}
}
+/// Deduplicates function-registry map entries by keeping only entries whose key
+/// matches the canonical name. The session stores one hash map entry per alias
+/// plus the canonical name; filtering to canonical-name entries yields exactly
+/// one [`Arc`] per logical function.
+fn dedup_function_registry_by_canonical_name<T>(
Review Comment:
minor: I think the function could consume the map instead of borrowing it, to avoid some arc clones
```rust
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()
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix non-deterministic iteration in SessionStateBuilder [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #21262:
URL: https://github.com/apache/datafusion/pull/21262#issuecomment-4303398429
That could be a way forward, but I think we could potentially still run into edge cases such as:
1. register `to_char()` (has alias `date_format`)
1. register `to_character()` (has alias `to_char`)
1. register `to_string()` (has alias `to_character`)
This leaves us in a state where only `to_string()` keeps its canonical mapping, and `to_character()` survives via its alias `to_char` and `to_char()` survives via `date_format`. So now we could have a race based on if `to_character()` or `to_char()` gets registered first in the roundtrip.
Perhaps the current data structure of `SessionStateBuilder` is incompatible of doing a pure roundtrip, unless we can somehow record the order in which UDFs were recorded in the original `SessionState`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix non-deterministic iteration in SessionStateBuilder [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #21262:
URL: https://github.com/apache/datafusion/pull/21262#discussion_r3226121281
##########
datafusion/core/src/execution/session_state.rs:
##########
@@ -1054,6 +1054,37 @@ pub struct SessionStateBuilder {
physical_optimizer_rules: Option<Vec<Arc<dyn PhysicalOptimizerRule + Send + Sync>>>,
}
+fn collect_registry_values_by_canonical_name<T: ?Sized, F>(
+ entries: HashMap<String, Arc<T>>,
+ canonical_name: F,
+) -> Vec<Arc<T>>
+where
+ F: Fn(&Arc<T>) -> String,
+{
+ // First, collect canonical key mappings (if present), which represent the
+ // current authoritative binding for a canonical name.
+ let mut by_name = std::collections::BTreeMap::new();
Review Comment:
I think relying on a btreemap like this still has issues, because when we iterate on it later we iterate based on the total key ordering, not based on insertion time. That can lead to issues where based on the names of the UDFs the roundtripped state is still incorrect. For example see this test case:
```rust
#[test]
fn test123() -> Result<()> {
let mut state = SessionStateBuilder::new().build();
let udf1 = datafusion_expr::create_udf(
"z_to_char",
vec![DataType::Utf8],
DataType::Utf8,
datafusion_expr::Volatility::Immutable,
Arc::new(|_args| {
Ok(datafusion_expr::ColumnarValue::Scalar(
datafusion_common::ScalarValue::Utf8(None),
))
}),
)
.with_aliases(["z_date_format"]);
let udf2 = datafusion_expr::create_udf(
"to_character",
vec![DataType::Utf8],
DataType::Utf8,
datafusion_expr::Volatility::Immutable,
Arc::new(|_args| {
Ok(datafusion_expr::ColumnarValue::Scalar(
datafusion_common::ScalarValue::Utf8(None),
))
}),
)
.with_aliases(["z_to_char"]);
state.register_udf(Arc::new(udf1))?;
state.register_udf(Arc::new(udf2))?;
let roundtrip = SessionStateBuilder::new_from_existing(state.clone());
let roundtrip = roundtrip.build();
dbg!(state.scalar_functions(), roundtrip.scalar_functions());
assert_eq!(state.scalar_functions(), roundtrip.scalar_functions());
Ok(())
}
```
The before state has this mapping:
```
[datafusion/core/src/execution/session_state.rs:2668:9] state.scalar_functions() = {
"z_date_format": ScalarUDF {
inner: AliasedScalarUDFImpl {
inner: SimpleScalarUDF {
name: "z_to_char",
signature: Signature {
type_signature: Exact(
[
Utf8,
],
),
volatility: Immutable,
parameter_names: None,
},
return_type: Utf8,
fun: "<FUNC>",
},
aliases: [
"z_date_format",
],
},
},
"to_character": ScalarUDF {
inner: AliasedScalarUDFImpl {
inner: SimpleScalarUDF {
name: "to_character",
signature: Signature {
type_signature: Exact(
[
Utf8,
],
),
volatility: Immutable,
parameter_names: None,
},
return_type: Utf8,
fun: "<FUNC>",
},
aliases: [
"z_to_char",
],
},
},
"z_to_char": ScalarUDF {
inner: AliasedScalarUDFImpl {
inner: SimpleScalarUDF {
name: "to_character",
signature: Signature {
type_signature: Exact(
[
Utf8,
],
),
volatility: Immutable,
parameter_names: None,
},
return_type: Utf8,
fun: "<FUNC>",
},
aliases: [
"z_to_char",
],
},
},
}
```
Meanwhile the roundtrip has this mapping:
```
[datafusion/core/src/execution/session_state.rs:2668:9] roundtrip.scalar_functions() = {
"to_character": ScalarUDF {
inner: AliasedScalarUDFImpl {
inner: SimpleScalarUDF {
name: "to_character",
signature: Signature {
type_signature: Exact(
[
Utf8,
],
),
volatility: Immutable,
parameter_names: None,
},
return_type: Utf8,
fun: "<FUNC>",
},
aliases: [
"z_to_char",
],
},
},
"z_to_char": ScalarUDF {
inner: AliasedScalarUDFImpl {
inner: SimpleScalarUDF {
name: "z_to_char",
signature: Signature {
type_signature: Exact(
[
Utf8,
],
),
volatility: Immutable,
parameter_names: None,
},
return_type: Utf8,
fun: "<FUNC>",
},
aliases: [
"z_date_format",
],
},
},
"z_date_format": ScalarUDF {
inner: AliasedScalarUDFImpl {
inner: SimpleScalarUDF {
name: "z_to_char",
signature: Signature {
type_signature: Exact(
[
Utf8,
],
),
volatility: Immutable,
parameter_names: None,
},
return_type: Utf8,
fun: "<FUNC>",
},
aliases: [
"z_date_format",
],
},
},
}
```
See how `z_to_char` now points to the `z_to_char` UDF instead of the `to_character` UDF.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix non-deterministic iteration in SessionStateBuilder [datafusion]
Posted by "alexanderbianchi (via GitHub)" <gi...@apache.org>.
alexanderbianchi commented on code in PR #21262:
URL: https://github.com/apache/datafusion/pull/21262#discussion_r3012177499
##########
datafusion/core/src/execution/session_state.rs:
##########
@@ -978,6 +978,20 @@ impl SessionState {
/// A builder to be used for building [`SessionState`]'s. Defaults will
/// be used for all values unless explicitly provided.
///
+/// Deduplicates function-registry map entries by keeping only entries whose key
+/// matches the canonical name. The session stores one hash map entry per alias
+/// plus the canonical name; filtering to canonical-name entries yields exactly
+/// one [`Arc`] per logical function.
+fn dedup_function_registry_by_canonical_name<T>(
+ map: &HashMap<String, Arc<T>>,
+ canonical_name: impl Fn(&T) -> &str,
Review Comment:
ah I see now, my bad.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org