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.

Sign up for GitHub

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

Move DaliOperatorTable to Coral-Common and rename it #276

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Move DaliOperatorTable to Coral-Common and rename it #276

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
06147f6
Move DaliOperatorTable to Coral-Common and add abstract class Functio…
ljfgem May 2, 2022
0e6eccf
Modify resolve to be an abstract method
ljfgem May 8, 2022
13ccdec
Move CoralOperatorTable
ljfgem Jun 5, 2022
2ed61ee
Create specific TrinoFunctionResolver
ljfgem Jun 6, 2022
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
16 changes: 8 additions & 8 deletions ...oral/hive/hive2rel/DaliOperatorTable.java → ...edin/coral/common/CoralOperatorTable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* Licensed under the BSD-2 Clause license.
* See LICENSE in the project root for license information.
*/
package com.linkedin.coral.hive.hive2rel;
package com.linkedin.coral.common;

import java.util.Collection;
import java.util.List;
Expand All @@ -20,25 +20,25 @@
import org.apache.calcite.util.Util;

import com.linkedin.coral.common.functions.Function;
import com.linkedin.coral.hive.hive2rel.functions.HiveFunctionResolver;
import com.linkedin.coral.common.functions.FunctionResolver;


/**
* Class to resolve Dali function names in SQL definition based on
* Class to resolve Coral operator names in SQL definition based on
* the mapping stored in table parameters in the metastore.
*/
public class DaliOperatorTable implements SqlOperatorTable {
public class CoralOperatorTable implements SqlOperatorTable {
// TODO: support injection framework to inject same function resolver here and ParseTreeBuilder.
// For now, we create another instance since the function registry is simple.
private final HiveFunctionResolver funcResolver;
private final FunctionResolver funcResolver;

public DaliOperatorTable(HiveFunctionResolver funcResolver) {
public CoralOperatorTable(FunctionResolver funcResolver) {
this.funcResolver = funcResolver;
}

/**
* Resolves functions names to corresponding Calcite UDF. HiveFunctionResolver ensures that
* {@code sqlIdentifier} has function name or corresponding class name for Dali functions. All function registry
* Resolves operator names to corresponding Calcite operator. FunctionResolver ensures that
* {@code sqlIdentifier} has operator name or corresponding class name for Coral operator. All registry
* lookups performed by this class are case-insensitive.
*
* Calcite invokes this function multiple times during analysis phase to validate SqlCall operators. This is
Expand Down
28 changes: 28 additions & 0 deletions coral-common/src/main/java/com/linkedin/coral/common/functions/FunctionResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/**
* Copyright 2018-2022 LinkedIn Corporation. All rights reserved.
* Licensed under the BSD-2 Clause license.
* See LICENSE in the project root for license information.
*/
package com.linkedin.coral.common.functions;

import java.util.Collection;


/**
* Class to resolve function names in SQL to Function.
*/
public abstract class FunctionResolver {

protected final FunctionRegistry registry;

protected FunctionResolver(FunctionRegistry registry) {
this.registry = registry;
}

/**
* Resolves function to concrete operator case-insensitively.
* @param functionName function name to resolve
* @return list of matching Functions or empty list if there is no match
*/
public abstract Collection<Function> resolve(String functionName);
}
3 changes: 2 additions & 1 deletion coral-hive/src/main/java/com/linkedin/coral/hive/hive2rel/HiveToRelConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.apache.calcite.sql2rel.SqlToRelConverter;
import org.apache.hadoop.hive.metastore.api.Table;

import com.linkedin.coral.common.CoralOperatorTable;
import com.linkedin.coral.common.HiveMetastoreClient;
import com.linkedin.coral.common.ToRelConverter;
import com.linkedin.coral.hive.hive2rel.functions.HiveFunctionResolver;
Expand Down Expand Up @@ -75,7 +76,7 @@ protected SqlValidator getSqlValidator() {

@Override
protected SqlOperatorTable getOperatorTable() {
return ChainedSqlOperatorTable.of(SqlStdOperatorTable.instance(), new DaliOperatorTable(functionResolver));
return ChainedSqlOperatorTable.of(SqlStdOperatorTable.instance(), new CoralOperatorTable(functionResolver));
}

@Override
Expand Down
7 changes: 4 additions & 3 deletions ...l-hive/src/main/java/com/linkedin/coral/hive/hive2rel/functions/HiveFunctionResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import com.linkedin.coral.common.HiveTable;
import com.linkedin.coral.common.functions.Function;
import com.linkedin.coral.common.functions.FunctionRegistry;
import com.linkedin.coral.common.functions.FunctionResolver;
import com.linkedin.coral.common.functions.UnknownSqlFunctionException;

import static com.google.common.base.Preconditions.*;
Expand All @@ -43,14 +44,13 @@
/**
* Class to resolve hive function names in SQL to Function.
*/
public class HiveFunctionResolver {
public class HiveFunctionResolver extends FunctionResolver {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why could not not this be moved to coral-common?

Copy link
Collaborator Author

@ljfgem ljfgem Jun 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@wmoustafa do you have a preference between common function resolver vs language-specific function resolvers?

Copy link
Collaborator Author

@ljfgem ljfgem Jun 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think aside from the dynamic resolution, rest should be language independent and go to coral-common?

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.

I think those are ultimately going to be Coral IR specific classes, and not Hive?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@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.


public final FunctionRegistry registry;
private final ConcurrentHashMap<String, Function> dynamicFunctionRegistry;
private final List<SqlOperator> operators;

public HiveFunctionResolver(FunctionRegistry registry, ConcurrentHashMap<String, Function> dynamicRegistry) {
this.registry = registry;
super(registry);
this.dynamicFunctionRegistry = dynamicRegistry;
this.operators = new ArrayList<>(SqlStdOperatorTable.instance().getOperatorList());
operators.add(HiveRLikeOperator.REGEXP);
Expand Down Expand Up @@ -142,6 +142,7 @@ public Function tryResolve(@Nonnull String functionName, @Nullable Table hiveTab
* @param functionName function name to resolve
* @return list of matching Functions or empty list if there is no match
*/
@Override
public Collection<Function> resolve(String functionName) {
Collection<Function> staticLookup = registry.lookup(functionName);
if (!staticLookup.isEmpty()) {
Expand Down
17 changes: 11 additions & 6 deletions coral-trino/src/main/java/com/linkedin/coral/trino/trino2rel/TrinoToRelConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@
*/
package com.linkedin.coral.trino.trino2rel;

import java.util.Collection;
import java.util.List;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;

import org.apache.calcite.adapter.java.JavaTypeFactory;
import org.apache.calcite.plan.RelOptCluster;
Expand All @@ -22,13 +22,14 @@
import org.apache.calcite.sql2rel.SqlToRelConverter;
import org.apache.hadoop.hive.metastore.api.Table;

import com.linkedin.coral.common.CoralOperatorTable;
import com.linkedin.coral.common.HiveMetastoreClient;
import com.linkedin.coral.common.ToRelConverter;
import com.linkedin.coral.hive.hive2rel.DaliOperatorTable;
import com.linkedin.coral.common.functions.Function;
import com.linkedin.coral.common.functions.FunctionResolver;
import com.linkedin.coral.hive.hive2rel.HiveConvertletTable;
import com.linkedin.coral.hive.hive2rel.HiveRelBuilder;
import com.linkedin.coral.hive.hive2rel.HiveSqlValidator;
import com.linkedin.coral.hive.hive2rel.functions.HiveFunctionResolver;
import com.linkedin.coral.hive.hive2rel.functions.StaticHiveFunctionRegistry;
import com.linkedin.coral.trino.trino2rel.parsetree.ParseTreeBuilder;
import com.linkedin.coral.trino.trino2rel.parsetree.ParserVisitorContext;
Expand All @@ -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()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR @ljfgem

Can we write a TrinoFunctionResolver similar to hiveFunctionResolver explicitly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for suggestion, added.

@Override
public Collection<Function> resolve(String functionName) {
return registry.lookup(functionName);
}
};
private final
// The validator must be reused
SqlValidator sqlValidator = new HiveSqlValidator(getOperatorTable(), getCalciteCatalogReader(),
Expand All @@ -74,7 +79,7 @@ protected SqlValidator getSqlValidator() {

@Override
protected SqlOperatorTable getOperatorTable() {
return ChainedSqlOperatorTable.of(SqlStdOperatorTable.instance(), new DaliOperatorTable(functionResolver));
return ChainedSqlOperatorTable.of(SqlStdOperatorTable.instance(), new CoralOperatorTable(functionResolver));
}

@Override
Expand Down