Change result type of concat varchar function to bounded varchar #12922

ebyhr · 2022-06-22T02:47:40Z

Description

Change result type of concat varchar function to bounded varchar
Fixes #12827

Documentation

( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

core/trino-main/src/main/java/io/trino/metadata/SignatureBinder.java

core/trino-main/src/test/java/io/trino/operator/scalar/TestStringFunctions.java

findepi · 2022-06-22T10:03:07Z

testing/trino-testing/src/main/java/io/trino/testing/AbstractTestEngineOnlyQueries.java

@@ -1627,7 +1627,7 @@ public void testInvalidTypeInfixOperator()
        // Comment on why error message references varchar(214783647) instead of varchar(2) which seems expected result type for concatenation in expression.


this comment is outdated (apparently since long)

findepi · 2022-06-24T12:31:39Z

core/trino-main/src/main/java/io/trino/operator/scalar/ConcatFunction.java

+                            .longVariable("x", "min(2147483647, TOTAL_LONG_LITERAL_SIZE)")
+                            .returnType(parseTypeSignature("varchar(x)", ImmutableSet.of("x")))


would it be possible to populate the return type in a functional manner?
like

.returnType(argumentTypes -> { .... })

I know this would harm equivalence on Signature objects (Signature defines equals and there is no strict equality for functions). However, Signature seems to be used as a map key only for non-calculated signatures (at least here

trino/core/trino-main/src/main/java/io/trino/operator/ParametricImplementationsGroup.java

Lines 164 to 173 in 93e6bc3

if (implementation.getSignature().getTypeVariableConstraints().isEmpty()

&& implementation.getSignature().getArgumentTypes().stream().noneMatch(TypeSignature::isCalculated)

&& !implementation.getSignature().getReturnType().isCalculated()) {

exactImplementations.put(implementation.getSignature(), implementation);

}

else if (implementation.hasSpecializedTypeParameters()) {

specializedImplementations.add(implementation);

}

else {

genericImplementations.add(implementation);

). So maybe the equivalence-related problem is solvable.

@dain @martint @losipiuk @sopel39 thoughts?

core/trino-main/src/main/java/io/trino/metadata/SignatureBinder.java

ebyhr · 2022-07-19T06:21:40Z

Updated to functional style. A downside seems we have to provide dummy return type because such information is required for SHOW FUNCTIONS result.

findepi · 2022-07-20T13:29:46Z

core/trino-main/src/main/java/io/trino/metadata/Signature.java

+    @JsonProperty
+    public Optional<Function<List<TypeSignature>, TypeSignature>> getFunctionalReturnType()


a Function can unlikely be json-serializable?

@ebyhr i think this is a blocker for me, unless Signature is not actually getting serialized.

I think so too. I'm wondering when json (de)serialization happens for Signature class.

I don't know that, but you should be able to find out easily, by removing the @Json* annotations from the class and running tests with DistributedQueryRunner

findepi · 2022-08-03T14:42:44Z

@martint can you please take a look?

Outdated. Addressed.

findepi · 2022-09-07T15:06:51Z

It looks like this PR conflicted with @dain 's #12588 that was merged recently.

ebyhr · 2022-09-08T01:22:43Z

Just rebased on upstream to resolve conflicts.

findepi · 2022-09-09T09:34:49Z

Looks like the only test failing after Remove json annotations from Signature is TestSignature.testSerializationRoundTrip.
So it looks like it doesn't need to be json-serializable.

cc @dain

martint · 2022-09-09T19:08:02Z

core/trino-spi/src/main/java/io/trino/spi/function/Signature.java

    public TypeSignature getReturnType()
    {
        return returnType;
    }

-    @JsonProperty
+    public Optional<Function<List<TypeSignature>, TypeSignature>> getFunctionalReturnType()


What's a "functional return type"?

It's a method to allow building "return type" (e.g. a + b) from "function arguments" (e.g. concat(a, b)). The name might be confusing. Do you have any suggestion?

getReturnTypeDerivation ?

Thanks for your suggestion, changed to getReturnTypeDerivation.

findepi · 2022-09-13T10:40:02Z

core/trino-spi/src/main/java/io/trino/spi/function/Signature.java

-     */
-    @Deprecated
-    @JsonCreator
-    public static Signature fromJson(


This method is here on purpose to make it clear that this method is only here for Jasckson json parsing because we disable access to private constructors when we use JSON. If the parsing works when this is removed, it implies that the code deserializing the JSON has a misconfigured parser.

findepi · 2022-09-13T10:42:02Z

core/trino-spi/src/main/java/io/trino/spi/function/Signature.java

@@ -241,6 +254,12 @@ public Builder returnType(TypeSignature returnType)
            return this;
        }

+        public Builder returnTypeDerivation(Optional<Function<List<TypeSignature>, TypeSignature>> returnTypeDerivation)


This could accept without Optional.

findepi · 2022-09-13T10:42:22Z

core/trino-main/src/main/java/io/trino/operator/scalar/ConcatFunction.java

+                            // still required for SHOW FUNCTIONS result
+                            .longVariable("x", "1")


what does the SHOW FUNCTIONS print now?

It's varchar(x) after this change

findepi · 2022-09-13T10:43:04Z

core/trino-main/src/main/java/io/trino/operator/scalar/ConcatFunction.java

+
+                            // still required for SHOW FUNCTIONS result
+                            .longVariable("x", "1")
+                            .returnType(new TypeSignature("varchar", typeVariable("x")))


Can we use unbounded varchar as a "fake" return type?

Probably, it requires additional change. Below exception happens after changing to returnType(VARCHAR) without longVariable.

Bound signature concat(varchar, varchar):varchar(3) does not match declared signature concat(varchar):varchar java.lang.IllegalArgumentException: Bound signature concat(varchar, varchar):varchar(3) does not match declared signature concat(varchar):varchar at com.google.common.base.Preconditions.checkArgument(Preconditions.java:435) at io.trino.metadata.SignatureBinder.verifyBoundSignature(SignatureBinder.java:332) at io.trino.metadata.SignatureBinder.extractBoundVariables(SignatureBinder.java:306) at io.trino.metadata.SignatureBinder.bindFunction(SignatureBinder.java:251) at io.trino.metadata.FunctionResolver.toFunctionBinding(FunctionResolver.java:127) at io.trino.metadata.FunctionResolver.matchFunction(FunctionResolver.java:258) at io.trino.metadata.FunctionResolver.matchFunctionWithCoercion(FunctionResolver.java:233) at io.trino.metadata.FunctionResolver.resolveFunction(FunctionResolver.java:174) at io.trino.metadata.MetadataManager.resolvedFunctionInternal(MetadataManager.java:2115) at io.trino.metadata.MetadataManager.lambda$resolvedFunctionInternal$49(MetadataManager.java:2110) at java.base/java.util.Optional.orElseGet(Optional.java:364) at io.trino.metadata.MetadataManager.resolvedFunctionInternal(MetadataManager.java:2110) at io.trino.metadata.MetadataManager.resolveFunction(MetadataManager.java:2081) at io.trino.sql.analyzer.ExpressionAnalyzer$Visitor.visitFunctionCall(ExpressionAnalyzer.java:1258) at io.trino.sql.analyzer.ExpressionAnalyzer$Visitor.visitFunctionCall(ExpressionAnalyzer.java:581) at io.trino.sql.tree.FunctionCall.accept(FunctionCall.java:121) at io.trino.sql.tree.StackableAstVisitor.process(StackableAstVisitor.java:27) at io.trino.sql.analyzer.ExpressionAnalyzer$Visitor.process(ExpressionAnalyzer.java:604) at io.trino.sql.analyzer.ExpressionAnalyzer.analyze(ExpressionAnalyzer.java:485) at io.trino.sql.analyzer.ExpressionAnalyzer.analyzeExpression(ExpressionAnalyzer.java:3441) at io.trino.sql.analyzer.StatementAnalyzer$Visitor.analyzeExpression(StatementAnalyzer.java:4075) at io.trino.sql.analyzer.StatementAnalyzer$Visitor.analyzeSelectSingleColumn(StatementAnalyzer.java:3887) at io.trino.sql.analyzer.StatementAnalyzer$Visitor.analyzeSelect(StatementAnalyzer.java:3673) at io.trino.sql.analyzer.StatementAnalyzer$Visitor.visitQuerySpecification(StatementAnalyzer.java:2436) at io.trino.sql.analyzer.StatementAnalyzer$Visitor.visitQuerySpecification(StatementAnalyzer.java:457) at io.trino.sql.tree.QuerySpecification.accept(QuerySpecification.java:155) at io.trino.sql.tree.AstVisitor.process(AstVisitor.java:27) at io.trino.sql.analyzer.StatementAnalyzer$Visitor.process(StatementAnalyzer.java:474) at io.trino.sql.analyzer.StatementAnalyzer$Visitor.process(StatementAnalyzer.java:482) at io.trino.sql.analyzer.StatementAnalyzer$Visitor.visitQuery(StatementAnalyzer.java:1394) at io.trino.sql.analyzer.StatementAnalyzer$Visitor.visitQuery(StatementAnalyzer.java:457) at io.trino.sql.tree.Query.accept(Query.java:107) at io.trino.sql.tree.AstVisitor.process(AstVisitor.java:27) at io.trino.sql.analyzer.StatementAnalyzer$Visitor.process(StatementAnalyzer.java:474) at io.trino.sql.analyzer.StatementAnalyzer.analyze(StatementAnalyzer.java:436) at io.trino.sql.analyzer.Analyzer.analyze(Analyzer.java:79) at io.trino.sql.analyzer.Analyzer.analyze(Analyzer.java:71) at io.trino.execution.SqlQueryExecution.analyze(SqlQueryExecution.java:261) at io.trino.execution.SqlQueryExecution.<init>(SqlQueryExecution.java:199) at io.trino.execution.SqlQueryExecution$SqlQueryExecutionFactory.createQueryExecution(SqlQueryExecution.java:825) at io.trino.dispatcher.LocalDispatchQueryFactory.lambda$createDispatchQuery$0(LocalDispatchQueryFactory.java:135) at io.trino.$gen.Trino_testversion____20220914_081840_71.call(Unknown Source) at com.google.common.util.concurrent.TrustedListenableFutureTask$TrustedFutureInterruptibleTask.runInterruptibly(TrustedListenableFutureTask.java:131) at com.google.common.util.concurrent.InterruptibleTask.run(InterruptibleTask.java:74) at com.google.common.util.concurrent.TrustedListenableFutureTask.run(TrustedListenableFutureTask.java:82) at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136) at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635) at java.base/java.lang.Thread.run(Thread.java:833)

It doesn't make sense to allow a function definition to declare a return type and a returnTypeDerivation at the same time:

If they are not the same value, which one matters?

If they are the same value, why have the returnTypeDerivation at all?

What should be the return type printed by SHOW FUNCTIONS?

findepi · 2022-09-13T10:43:44Z

core/trino-main/src/main/java/io/trino/operator/scalar/ConcatFunction.java

+        throw new IllegalArgumentException("Unsupported type: " + type);
+    }
+
+    private static TypeSignature calculateReturnType(List<TypeSignature> typeSignatures)


calculateVarcharConcatReturnType

ebyhr · 2022-09-25T22:58:20Z

@dain Gentle reminder.

findepi · 2022-09-26T08:42:23Z

@ebyhr there is a CI failure

ebyhr · 2022-09-27T02:08:18Z

I will merge this PR tomorrow if there's no objection.
cc: @dain @martint

martint · 2022-09-27T02:47:24Z

The concern I have is that this lets functions define how the result type is derived and completely bypass the type system. It's possible that this will make it harder to improve the type inferencer in the future without breaking backward compatibility, especially if anyone uses this feature to derive a return type that is not what the inference engine would naturally produce if it was capable of doing so.

ebyhr · 2022-11-08T06:29:20Z

Just rebased on upstream to resolve conflicts.

ebyhr · 2022-11-08T06:31:48Z

@martint Do you have any alternative solution for improving concat result type?

ebyhr · 2022-12-13T02:32:26Z

@martint Gentle reminder.

ebyhr · 2023-01-10T04:18:14Z

@martint Gentle reminder.

All tests passed except for TestSignature when removed json annotations from Signature class. TestSignature was a unit test verifying json serialization and didn't contain the actual usage. Removing the annotation and test should be fine.

ebyhr · 2023-01-25T23:14:58Z

Rebased on upstream to resolve conflicts.

dain

I don't understand why this change. If we want concat(varchar(3), varchar(5) to return varchar(8) and concat(varchar, varchar(5) to be unbounded, we should work on the type calculation system to system to support that exact calculation instead of giving functions the ability to opt out of the type system.

dain · 2023-02-19T19:19:00Z

core/trino-spi/src/main/java/io/trino/spi/function/Signature.java

-     */
-    @Deprecated
-    @JsonCreator
-    public static Signature fromJson(


This method is here on purpose to make it clear that this method is only here for Jasckson json parsing because we disable access to private constructors when we use JSON. If the parsing works when this is removed, it implies that the code deserializing the JSON has a misconfigured parser.

dain · 2023-02-19T19:19:33Z

core/trino-main/src/test/java/io/trino/metadata/TestSignature.java

-import static io.trino.type.InternalTypeManager.TESTING_TYPE_MANAGER;
-import static org.testng.Assert.assertEquals;
-
-public class TestSignature


Why would we remove the test that verifies the signature round trips?

cla-bot bot added the cla-signed label Jun 22, 2022

ebyhr force-pushed the ebi/concat branch 3 times, most recently from fb4a2de to e15651c Compare June 22, 2022 08:15

github-actions bot added the tests:hive label Jun 22, 2022

findepi reviewed Jun 22, 2022

View reviewed changes

findepi requested a review from martint June 22, 2022 10:16

ebyhr force-pushed the ebi/concat branch from e15651c to 2d57cd4 Compare June 23, 2022 02:02

ebyhr requested a review from kasiafi June 24, 2022 02:34

findepi reviewed Jun 24, 2022

View reviewed changes

martint previously requested changes Jun 24, 2022

View reviewed changes

core/trino-main/src/main/java/io/trino/metadata/SignatureBinder.java Outdated Show resolved Hide resolved

ebyhr force-pushed the ebi/concat branch 2 times, most recently from b8eb947 to 63ed2ef Compare July 6, 2022 02:23

ebyhr marked this pull request as draft July 6, 2022 03:02

ebyhr force-pushed the ebi/concat branch from 63ed2ef to b1b4af4 Compare July 6, 2022 04:05

ebyhr marked this pull request as ready for review July 6, 2022 06:10

ebyhr requested a review from martint July 6, 2022 06:11

findepi reviewed Jul 20, 2022

View reviewed changes

ebyhr force-pushed the ebi/concat branch from b1b4af4 to 54f978f Compare September 1, 2022 01:41

ebyhr force-pushed the ebi/concat branch from 54f978f to b12c6cb Compare September 8, 2022 01:22

martint reviewed Sep 9, 2022

View reviewed changes

ebyhr force-pushed the ebi/concat branch 2 times, most recently from 9976c86 to 2659241 Compare September 10, 2022 00:47

ebyhr requested a review from findepi September 12, 2022 02:11

ebyhr requested a review from martint September 12, 2022 02:11

ebyhr force-pushed the ebi/concat branch from 2659241 to 6ab8b43 Compare September 12, 2022 13:32

findepi reviewed Sep 13, 2022

View reviewed changes

findepi approved these changes Sep 13, 2022

View reviewed changes

findepi requested a review from dain September 13, 2022 10:44

ebyhr force-pushed the ebi/concat branch from 6ab8b43 to f3bc26b Compare September 14, 2022 08:21

ebyhr force-pushed the ebi/concat branch from 9892b9c to a946fce Compare November 8, 2022 06:29

ebyhr assigned martint Dec 15, 2022

ebyhr added 3 commits January 26, 2023 08:12

Remove json annotations from Signature

8e693f1

All tests passed except for TestSignature when removed json annotations from Signature class. TestSignature was a unit test verifying json serialization and didn't contain the actual usage. Removing the annotation and test should be fine.

Support functional return type in Signature class

4d3fc4f

Change result type of concat varchar function to bounded varchar

d8db783

ebyhr force-pushed the ebi/concat branch from a946fce to d8db783 Compare January 25, 2023 23:14

dain requested changes Feb 19, 2023

View reviewed changes

ebyhr closed this Feb 20, 2023

ebyhr deleted the ebi/concat branch February 20, 2023 00:22

		@@ -1627,7 +1627,7 @@ public void testInvalidTypeInfixOperator()
		// Comment on why error message references varchar(214783647) instead of varchar(2) which seems expected result type for concatenation in expression.

		.longVariable("x", "min(2147483647, TOTAL_LONG_LITERAL_SIZE)")
		.returnType(parseTypeSignature("varchar(x)", ImmutableSet.of("x")))

	if (implementation.getSignature().getTypeVariableConstraints().isEmpty()
	&& implementation.getSignature().getArgumentTypes().stream().noneMatch(TypeSignature::isCalculated)
	&& !implementation.getSignature().getReturnType().isCalculated()) {
	exactImplementations.put(implementation.getSignature(), implementation);
	}
	else if (implementation.hasSpecializedTypeParameters()) {
	specializedImplementations.add(implementation);
	}
	else {
	genericImplementations.add(implementation);

		@JsonProperty
		public Optional<Function<List<TypeSignature>, TypeSignature>> getFunctionalReturnType()

		// still required for SHOW FUNCTIONS result
		.longVariable("x", "1")

Change result type of concat varchar function to bounded varchar #12922

Change result type of concat varchar function to bounded varchar #12922

Conversation

ebyhr commented Jun 22, 2022 • edited Loading

Description

Documentation

Release notes

findepi Jun 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebyhr commented Jul 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findepi commented Aug 3, 2022

findepi commented Sep 7, 2022

ebyhr commented Sep 8, 2022

findepi commented Sep 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebyhr commented Sep 25, 2022

findepi commented Sep 26, 2022

ebyhr commented Sep 27, 2022

martint commented Sep 27, 2022

ebyhr commented Nov 8, 2022

ebyhr commented Nov 8, 2022

ebyhr commented Dec 13, 2022

ebyhr commented Jan 10, 2023

ebyhr commented Jan 25, 2023

dain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebyhr commented Jun 22, 2022 •

edited

Loading

findepi Jun 22, 2022 •

edited

Loading