Easier scalar func support #23642
Easier scalar func support #23642
Conversation
auden-woolfson
force-pushed
the
easier_scalar_func_support
branch
from
3d8ce2b
to
9d62141
Compare
|
Can you add tests ? Take a look at |
| @@ -33,8 +35,8 @@ private OperatorTranslators() | |||
| } | |||
|
|
|||
| @ScalarOperator(ADD) | |||
| @SqlType(StandardTypes.BIGINT) | |||
There was a problem hiding this comment.
I think the file we want to change is in presto-base-jdbc
There was a problem hiding this comment.
Ok, I can update all the cases in the JDBC file, should we ultimately change all operator translator classes?
There was a problem hiding this comment.
Yes, we should support all translators eventually. I am not familiar with the SQL dialect that Clickhouse supports, but I think the basic operators + types should translate and work with it. Feel free to take it up in another PR if this reduces the testing overhead
There was a problem hiding this comment.
Ok, will do once this one gets merged!
| @@ -33,8 +35,8 @@ private OperatorTranslators() | |||
| } | |||
|
|
|||
| @ScalarOperator(ADD) | |||
| @SqlType(StandardTypes.BIGINT) | |||
| public static ClickHouseExpression add(@SqlType(StandardTypes.BIGINT) ClickHouseExpression left, @SqlType(StandardTypes.BIGINT) ClickHouseExpression right) | |||
| @SupportedSignatures(@SqlSignature({StandardTypes.BIGINT, StandardTypes.BIGINT})) | |||
There was a problem hiding this comment.
Should both of these be BIGINT ?
There was a problem hiding this comment.
Yes, one of these values represents the argument types, and one represents the return type
|
|
||
| @Retention(RUNTIME) | ||
| @Target(METHOD) | ||
| public @interface SqlSignature { |
There was a problem hiding this comment.
Should we call this SqlTypes instead ?
There was a problem hiding this comment.
I made some updates that I think clarify the purpose of the annotations, I think SqlSignature might be a bit more descriptive
auden-woolfson
requested review from
jaystarshot,
feilong-liu and
ClarenceThreepwood
as code owners
presto-main/src/test/java/com/facebook/presto/sql/relational/TestRowExpressionTranslator.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
@tdcmeehan Can you validate the approach being proposed here to support multiple type translations. Is there a better approach you can think of ?
| { | ||
| return new JdbcExpression(infixOperation("-", left, right), forwardBindVariables(left, right)); | ||
| } | ||
|
|
||
| @ScalarOperator(EQUAL) | ||
| @SqlType(StandardTypes.BOOLEAN) | ||
| public static JdbcExpression equal(@SqlType(StandardTypes.BIGINT) JdbcExpression left, @SqlType(StandardTypes.BIGINT) JdbcExpression right) | ||
| @SupportedSignatures(@SqlSignature(argumentType = StandardTypes.BIGINT, returnType = StandardTypes.BOOLEAN)) |
There was a problem hiding this comment.
Since most JDBC support EQUALS (=), NOT_EQUALS (<>) , comparison functions (greater than, less than) out-of-the-box, lets add support for these standard type variants - bigint, integer, smallint, tinyint, boolean, date, decimal, real, double, varchar, timestamp
| TypeSignature typeSignature = parseTypeSignature(type.value(), ImmutableSet.of()); | ||
| argumentTypes.add(typeSignature); | ||
| } | ||
| SupportedSignatures supportedSignatures = method.getAnnotation(SupportedSignatures.class); |
There was a problem hiding this comment.
I just saw this if block, I think migrating the Clickhouse OperatorTranslators right away makes sense to avoid the need for this if block
Description
Fix for issue #23605
Currently in order to add new operator/function mappings a new function needs to be defined for each possible mapping. By introducing some new annotations for functions and handling the case we can re use function definitions with different possible type signatures.