alamb commented on code in PR #23850:
URL: https://github.com/apache/datafusion/pull/23850#discussion_r3647425433
##########
datafusion/core/src/execution/session_state.rs:
##########
@@ -2525,6 +2668,96 @@ mod tests {
Ok(())
}
+ /// A no-op scalar UDF registered under `name` and `aliases`, for
exercising
+ /// alias overrides in the roundtrip tests below.
+ fn simple_udf(
+ name: &str,
+ aliases: impl IntoIterator<Item = &'static str>,
+ ) -> Arc<ScalarUDF> {
+ let udf = datafusion_expr::create_udf(
+ name,
+ vec![DataType::Utf8],
+ DataType::Utf8,
+ datafusion_expr::Volatility::Immutable,
+ Arc::new(|_| {
+ Ok(datafusion_expr::ColumnarValue::Scalar(
+ datafusion_common::ScalarValue::Utf8(None),
+ ))
+ }),
+ )
+ .with_aliases(aliases);
+ Arc::new(udf)
+ }
+
+ /// A UDF whose alias overrides another function must survive a
+ /// `new_from_existing` roundtrip on every run. Before #23697 the override
+ /// reverted whenever HashMap iteration re-registered the displaced
function
+ /// last; each iteration rebuilds the source state for a fresh map seed so
any
+ /// order dependence surfaces.
+ #[test]
+ fn test_from_existing_preserves_alias_override() -> Result<()> {
+ for _ in 0..64 {
Review Comment:
why do we need to do this 64 times?
##########
datafusion/core/src/execution/session_state.rs:
##########
@@ -1067,6 +1068,141 @@ pub struct SessionStateBuilder {
physical_optimizer_rules: Option<Vec<Arc<dyn PhysicalOptimizerRule + Send
+ Sync>>>,
}
+/// A function-registry entry (`ScalarUDF`, `AggregateUDF`, `WindowUDF`,
+/// `HigherOrderUDF`), exposing the canonical name and aliases it is registered
+/// under so [`deterministic_registration_order`] can rebuild a faithful order.
+trait AliasedRegistryEntry {
+ fn entry_name(&self) -> &str;
+ fn entry_aliases(&self) -> &[String];
+}
+
+impl AliasedRegistryEntry for ScalarUDF {
+ fn entry_name(&self) -> &str {
+ self.name()
+ }
+ fn entry_aliases(&self) -> &[String] {
+ self.aliases()
+ }
+}
+
+impl AliasedRegistryEntry for AggregateUDF {
+ fn entry_name(&self) -> &str {
+ self.name()
+ }
+ fn entry_aliases(&self) -> &[String] {
+ self.aliases()
+ }
+}
+
+impl AliasedRegistryEntry for WindowUDF {
+ fn entry_name(&self) -> &str {
+ self.name()
+ }
+ fn entry_aliases(&self) -> &[String] {
+ self.aliases()
+ }
+}
+
+impl AliasedRegistryEntry for HigherOrderUDF {
+ fn entry_name(&self) -> &str {
+ self.name()
+ }
+ fn entry_aliases(&self) -> &[String] {
+ self.aliases()
+ }
+}
+
+/// Flatten a function registry into its distinct functions, ordered so that
Review 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
##########
datafusion/core/src/execution/session_state.rs:
##########
@@ -1145,14 +1281,20 @@ impl SessionStateBuilder {
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()),
Review 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?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
