Skip to content

fix(frontend): support drop function without arguments #9561

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

Merged
merged 3 commits into from
May 3, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 11 additions & 9 deletions e2e_test/udf/python.slt
Original file line number Diff line number Diff line change
Expand Up @@ -165,20 +165,22 @@ drop materialized view mv;
statement ok
drop table t;

# TODO: drop function without arguments

# # Drop a function but ambiguous.
# statement error is not unique
# drop function gcd;

# Drop a function
statement ok
drop function int_42();

# Drop a function
# Drop a function without arguments, but the function name is not unique.
statement error is not unique
drop function gcd;

# Drop a function with arguments.
statement ok
drop function gcd(int, int);

# Drop a function
# foo() is different from foo.
statement error function not found
drop function gcd();

# Drop a function without arguments. Now the function name is unique.
statement ok
drop function gcd(int, int, int);
drop function gcd;
25 changes: 25 additions & 0 deletions src/frontend/src/catalog/root_catalog.rs
Original file line number Diff line number Diff line change
Expand Up @@ -568,6 +568,31 @@ impl Catalog {
.get_schema_by_name(db_name, schema_name)?
.get_function_by_name_args(function_name, args))
})?
.ok_or_else(|| {
CatalogError::NotFound(
"function",
format!(
"{}({})",
function_name,
args.iter().map(|a| a.to_string()).join(", ")
),
)
})
}

/// Gets all functions with the given name.
pub fn get_functions_by_name<'a>(
&self,
db_name: &str,
schema_path: SchemaPath<'a>,
function_name: &str,
) -> CatalogResult<(Vec<&Arc<FunctionCatalog>>, &'a str)> {
schema_path
.try_find(|schema_name| {
Ok(self
.get_schema_by_name(db_name, schema_name)?
.get_functions_by_name(function_name))
})?
.ok_or_else(|| CatalogError::NotFound("function", function_name.to_string()))
}

Expand Down
8 changes: 8 additions & 0 deletions src/frontend/src/catalog/schema_catalog.rs
Original file line number Diff line number Diff line change
Expand Up @@ -483,6 +483,14 @@ impl SchemaCatalog {
self.function_by_name.get(name)?.get(args)
}

pub fn get_functions_by_name(&self, name: &str) -> Option<Vec<&Arc<FunctionCatalog>>> {
let functions = self.function_by_name.get(name)?;
if functions.is_empty() {
return None;
}
Comment on lines +488 to +490
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this option unnecessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it might panic when this function returns a Some(vec![]). So I added these lines to prevent it.

Some(functions.values().collect())
}

pub fn get_connection_by_name(&self, connection_name: &str) -> Option<&Arc<ConnectionCatalog>> {
self.connection_by_name.get(connection_name)
}
Expand Down
35 changes: 29 additions & 6 deletions src/frontend/src/handler/drop_function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,15 +43,38 @@ pub async fn handle_drop_function(
let user_name = &session.auth_context().user_name;
let schema_path = SchemaPath::new(schema_name.as_deref(), &search_path, user_name);

// TODO: argument is not specified, drop the only function with the name
let mut arg_types = vec![];
for arg in func_desc.args.unwrap_or_default() {
arg_types.push(bind_data_type(&arg.data_type)?);
}
let arg_types = match func_desc.args {
Some(args) => {
let mut arg_types = vec![];
for arg in args {
arg_types.push(bind_data_type(&arg.data_type)?);
}
Some(arg_types)
}
None => None,
};

let function_id = {
let reader = session.env().catalog_reader().read_guard();
match reader.get_function_by_name_args(db_name, schema_path, &function_name, &arg_types) {
let res = match arg_types {
Some(arg_types) => {
reader.get_function_by_name_args(db_name, schema_path, &function_name, &arg_types)
}
// check if there is only one function if arguments are not specified
None => match reader.get_functions_by_name(db_name, schema_path, &function_name) {
Ok((functions, schema_name)) => {
if functions.len() > 1 {
return Err(ErrorCode::CatalogError(format!("function name {function_name:?} is not unique\nHINT: Specify the argument list to select the function unambiguously.").into()).into());
}
Ok((
functions.into_iter().next().expect("no functions"),
schema_name,
))
}
Err(e) => Err(e),
},
};
match res {
Comment on lines +64 to +77
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks a little spaghetti, but I don't have an immediate idea to improve it 🤪

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. it's quick-and-dirty. 🤪

Ok((function, schema_name)) => {
session.check_privilege_for_drop_alter(schema_name, &**function)?;
function.id
Expand Down
2 changes: 1 addition & 1 deletion src/sqlparser/src/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2108,7 +2108,7 @@ impl Parser {

let args = if self.consume_token(&Token::LParen) {
if self.consume_token(&Token::RParen) {
None
Some(vec![])
} else {
let args = self.parse_comma_separated(Parser::parse_function_arg)?;
self.expect_token(&Token::RParen)?;
Expand Down