[flink] Validate database existence when resolving catalog functions in FlinkCatalog - #3906
[flink] Validate database existence when resolving catalog functions in FlinkCatalog#3906pbanakar wants to merge 5 commits into
Conversation
| public CatalogFunction getFunction(ObjectPath functionPath) | ||
| throws FunctionNotExistException, CatalogException { | ||
| if (!databaseExists(functionPath.getDatabaseName())) { | ||
| throw new FunctionNotExistException(getName(), functionPath); |
There was a problem hiding this comment.
Masking a missing database as a missing function can be misleading to users. I believe we should explicitly inform the user that the database does not exist. We can throw a CatalogException while wrapping it with a clear "database not found" message.
There was a problem hiding this comment.
Updated getFunction to throw CatalogException with an explicit "Database X does not exist in catalog Y" message when the database is not found,
| @Override | ||
| public boolean functionExists(ObjectPath objectPath) throws CatalogException { | ||
| if (!databaseExists(objectPath.getDatabaseName())) { | ||
| return false; |
There was a problem hiding this comment.
Same here, we should throw CatalogException with database not found message rather than telling users the function not exist.
There was a problem hiding this comment.
Same, updated this and also the unit test assertions accordingly
|
Thanks @wuchong! Quick heads up, throwing CatalogException in functionExists broke FlinkProcedureITCase#testCallNonExistProcedure in CI. Turns out Flink internally uses functionExists() as a boolean probe during function/procedure resolution, so throwing there short-circuits the lookup chain before it reaches Calcite's validator users get "Database ... does not exist" instead of the expected "No match found for function signature ...". Can we revert just functionExists back to returning false and keep getFunction throwing CatalogException as you suggested, Does that split sound right to you, or would you prefer a different approach for functionExists? |
|
Thank you @pbanakar , I think you are right. I'm fine to revert the changes on |
|
thanks jark!, reverted it and i'll ping here once again once the ci passes |
|
Hi @wuchong , Hit the same issue with getFunction as functionExists CatalogException here also breaks Flink's internal probe chain, need to revert getFunction also, you suggestions valuable here! if you know of a cleaner way to handel this instead of reverting? , happy to dig further, Please let me know! |
Purpose
Linked issue: close #3901
FlinkCatalog#listFunctions,#functionExists, and#getFunctionresolved the built-in RoaringBitmap SQL functions (introduced in FIP-37) purely by function name, ignoring the database component of the request entirely. As a result, a fully qualified reference to a built-in function resolved successfully even when its database did not exist, e.g.:This broke the database-scoped semantics Flink catalogs are expected to guarantee, and made it impossible to rely on the database component of
<catalog>.<database>.<function>identifying an existing database.Brief change log
FlinkCatalog.java:listFunctions(String dbName)now throwsDatabaseNotExistExceptionifdbNamedoes not exist, before returning the built-in bitmap function names.functionExists(ObjectPath)now returnsfalseif the object path's database does not exist, before checking the built-in function map.getFunction(ObjectPath)now throwsFunctionNotExistExceptionif the object path's database does not exist, before resolving the function class.databaseExists(String)(already backed byadmin.databaseExists()), so no new dependency or RPC path was introduced.Tests
./mvnw test -pl fluss-flink/fluss-flink-common -Dtest="FlinkCatalogTest"— BUILD SUCCESS./mvnw verify -pl fluss-flink/fluss-flink-common -Dit.test="Flink118CatalogITCase,Flink119CatalogITCase,Flink120CatalogITCase,Flink22CatalogITCase,RbFunctionsCatalogITCase"— BUILD SUCCESSAPI and Format
no new public API is introduced.
Documentation
No user-facing documentation changes needed