Move DaliOperatorTable to Coral-Common and rename it
#276
Conversation
coral-common/src/main/java/com/linkedin/coral/common/functions/CoralOperatorTable.java
Outdated
Show resolved
Hide resolved
| @@ -43,14 +44,13 @@ | |||
| /** | |||
| * Class to resolve hive function names in SQL to Function. | |||
| */ | |||
| public class HiveFunctionResolver { | |||
| public class HiveFunctionResolver extends FunctionResolver { | |||
There was a problem hiding this comment.
Why could not not this be moved to coral-common?
There was a problem hiding this comment.
HiveFunctionResolver is only used for Coral-Hive and contains many Coral-Hive specific methods like Generic UDF dynamic registration. Therefore, I don't think we need this class for all the other xxx2rel modules, we need to have xxxFunctionResolver extending FunctionResolver for each xxx2rel module instead.
And in practice, given HiveFunctionResolver uses many classes in Coral-Hive like VersionedSqlUserDefinedFunction, HiveRLikeOperator and HiveGenericUDFReturnTypeInference, we need to move all of them for HiveFunctionResolver, which is not suitable in my opinion.
There was a problem hiding this comment.
As we are moving towards sqlNode translations, (coralSqlNode <-> *language*SqlNode), we can repurpose these *language*FunctionResolver classes to translate CoralFunctions to languagefunctions in this de-centralized manner.
Or we could maintain 1 registry and 1 function resolver in coral-common to fo coral <-> language translations in a centralized way.
I'm okay with sticking to the existing pattern.
There was a problem hiding this comment.
@wmoustafa do you have a preference between common function resolver vs language-specific function resolvers?
There was a problem hiding this comment.
Not sure IIUC, I don't think we can use a common function resolver in coral-common to solve all the language2rel functions, given difference input languages may need different methods in their function resolvers, for example, hive2rel needs dynamic Hive generic UDF registration while trino2rel doesn't need.
There was a problem hiding this comment.
I think aside from the dynamic resolution, rest should be language independent and go to coral-common?
And in practice, given
HiveFunctionResolveruses many classes in Coral-Hive likeVersionedSqlUserDefinedFunction,HiveRLikeOperatorandHiveGenericUDFReturnTypeInference, we need to move all of them forHiveFunctionResolver, which is not suitable in my opinion.
I think those are ultimately going to be Coral IR specific classes, and not Hive?
There was a problem hiding this comment.
@wmoustafa Rests are resolveUnaryOperator and resolveBinaryOperator which are only used in Hive's ParseTreeBuilder, not used in trino2rel because trino2rel has its own ParseTreeBuilder. So I would prefer to just keep resolve in FunctionResolver for now, we may add more common methods if there is in the future.
| @@ -47,8 +48,12 @@ | |||
| public class TrinoToRelConverter extends ToRelConverter { | |||
| private final ParseTreeBuilder parseTreeBuilder = new ParseTreeBuilder(); | |||
| private final ParserVisitorContext parserVisitorContext = new ParserVisitorContext(); | |||
| private final HiveFunctionResolver functionResolver = | |||
| new HiveFunctionResolver(new StaticHiveFunctionRegistry(), new ConcurrentHashMap<>()); | |||
| private final FunctionResolver functionResolver = new FunctionResolver(new StaticHiveFunctionRegistry()) { | |||
There was a problem hiding this comment.
Thanks for the PR @ljfgem
Can we write a TrinoFunctionResolver similar to hiveFunctionResolver explicitly?
There was a problem hiding this comment.
Thanks for suggestion, added.
| @@ -43,14 +44,13 @@ | |||
| /** | |||
| * Class to resolve hive function names in SQL to Function. | |||
| */ | |||
| public class HiveFunctionResolver { | |||
| public class HiveFunctionResolver extends FunctionResolver { | |||
There was a problem hiding this comment.
As we are moving towards sqlNode translations, (coralSqlNode <-> *language*SqlNode), we can repurpose these *language*FunctionResolver classes to translate CoralFunctions to languagefunctions in this de-centralized manner.
Or we could maintain 1 registry and 1 function resolver in coral-common to fo coral <-> language translations in a centralized way.
I'm okay with sticking to the existing pattern.
This PR is for #235.
DaliOperatorTableregisters legal operators for Calcite to validate and convert the input SqlNode.Given we convert other types of operators (like Trino operators) to Hive (Coral) operators in SqlNode phase, we use
DaliOperatorTablefor both Coral-Hive hive2rel, Coral-Trino trino2rel and all the future inputs, therefore, it makes more sense to moveDaliOperatorTableto Coral-Common.To move it, we need to create an abstract class
FunctionResolverto be used inCoralOperatorTable.cc: @wmoustafa
Tested by building and i-itest.