Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix duplicate function entries in the catalog #2368

Closed
scsmithr opened this issue Jan 8, 2024 · 0 comments · Fixed by #2370
Closed

Fix duplicate function entries in the catalog #2368

scsmithr opened this issue Jan 8, 2024 · 0 comments · Fixed by #2370
Labels
bug Something isn't working

Comments

@scsmithr
Copy link
Member

scsmithr commented Jan 8, 2024

Description

Currently we're inserting multiple functions with the same name due to the entry generation not accounting for our namespace rules here:

let udfs = udfs
.into_iter()
.flat_map(|f| {
let entry = (f.name().to_string(), f.clone());
// TODO: This is very weird to do here as this completely
// bypasses our schema resolution. It also results in duplicate
// function entries with the same name as the function's `name`
// argument is not aware of the namespace being added here.
match f.namespace() {
// we register the function under both the namespaced entry and the normal entry
// e.g. select foo.my_function() or select my_function()
FunctionNamespace::Optional(namespace) => {
let namespaced_entry = (format!("{}.{}", namespace, f.name()), f.clone());
vec![entry, namespaced_entry]
}
// we only register the function under the namespaced entry
// e.g. select foo.my_function()
FunctionNamespace::Required(namespace) => {
let namespaced_entry = (format!("{}.{}", namespace, f.name()), f.clone());
vec![namespaced_entry]
}
// we only register the function under the normal entry
// e.g. select my_function()
FunctionNamespace::None => {
vec![entry]
}
}
})
.collect::<HashMap<_, _>>();

Closing this issue requires this test to pass:

#[test]
fn builtin_catalog_no_function_name_duplicates() {
// TODO: Fix this
// Ensures each function is a unique (schema, name) pair.
// let catalog = BuiltinCatalog::new().unwrap();
// let names: Vec<(u32, &String)> = catalog
// .entries
// .values()
// .filter_map(|ent| match ent {
// CatalogEntry::Function(ent) => Some((ent.meta.parent, &ent.meta.name)),
// _ => None,
// })
// .collect();
// let mut deduped: HashSet<_> = names.clone().into_iter().collect();
// let diff: Vec<_> = names
// .into_iter()
// .filter(|name_and_parent| {
// let was_present = deduped.remove(name_and_parent);
// !was_present // We saw this value before, indicates a duplicated name.
// })
// .collect();
// assert_eq!(Vec::<(u32, &String)>::new(), diff);
}

@scsmithr scsmithr added the bug Something isn't working label Jan 8, 2024
scsmithr added a commit that referenced this issue Jan 8, 2024
Closes #2368

Futzing around with function aliases. This replaces `HashMap` with
`AliasMap` to allow multiple keys to point to the same object. Also adds
some tests.

I needed to add a workaround for 'array_to_string' for one of the
iterators, see #2371.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant