array_sort function expecting lambda comparator with 2 parameters when sidecar functions enabled

[kevintang2022]

The velox exposes a signature function(T,T,bigint) but the coordinator version has function(T,T,int)

With native sidecar enabled and using native.default. as the default function namespace, Q1 fails.

With presto.default function namespace and native sidecar functions disabled, Q2 fails.

With presto.default function namespace and built in worker functions enabled, Q1 and Q2 fail.

Q1 (array_sort using int)

SELECT 
  ARRAY_SORT(
    ARRAY[
      ARRAY['a', 'b', 'c']
    ], 
    (x, y) -> IF(
      COALESCE(
        x[1], 
        ''
      ) > COALESCE(
        y[1], 
        ''
      ), 
      1, 
      IF(
        COALESCE(
          x[1], 
          ''
        ) < COALESCE(
          y[1], 
          ''
        ), 
        -1, 
        0
      )
    )
  )

Q2 (array_sort using bigint)

SELECT 
  ARRAY_SORT(
    ARRAY[
      ARRAY['a', 'b', 'c']
    ], 
    (x, y) -> IF(
      COALESCE(
        x[1], 
        ''
      ) > COALESCE(
        y[1], 
        ''
      ), 
      cast(1 as bigint), 
      IF(
        COALESCE(
          x[1], 
          ''
        ) < COALESCE(
          y[1], 
          ''
        ), 
        cast(-1 as bigint), 
        cast(0 as bigint)
      )
    )
  )

Error stack trace:

com.facebook.presto.sql.analyzer.SemanticException: line 1:51: Expected a lambda that takes 1 argument(s) but got 2
	at com.facebook.presto.sql.analyzer.ExpressionAnalyzer$Visitor.visitLambdaExpression(ExpressionAnalyzer.java:1524)
	at com.facebook.presto.sql.analyzer.ExpressionAnalyzer$Visitor.visitLambdaExpression(ExpressionAnalyzer.java:390)
	at com.facebook.presto.sql.tree.LambdaExpression.accept(LambdaExpression.java:60)
	at com.facebook.presto.sql.tree.StackableAstVisitor.process(StackableAstVisitor.java:26)
	at com.facebook.presto.sql.analyzer.ExpressionAnalyzer$Visitor.process(ExpressionAnalyzer.java:413)
	at com.facebook.presto.sql.analyzer.ExpressionAnalyzer.analyze(ExpressionAnalyzer.java:357)
	at com.facebook.presto.sql.analyzer.ExpressionAnalyzer.access$2800(ExpressionAnalyzer.java:208)
	at com.facebook.presto.sql.analyzer.ExpressionAnalyzer$Visitor.lambda$visitFunctionCall$3(ExpressionAnalyzer.java:1089)
	at com.facebook.presto.sql.analyzer.TypeSignatureProvider.getTypeSignature(TypeSignatureProvider.java:60)
	at com.facebook.presto.metadata.SignatureBinder$FunctionSolver.update(SignatureBinder.java:764)
	at com.facebook.presto.metadata.SignatureBinder.iterativeSolve(SignatureBinder.java:434)
	at com.facebook.presto.metadata.SignatureBinder.bindVariables(SignatureBinder.java:118)
	at com.facebook.presto.metadata.SignatureBinder.bind(SignatureBinder.java:95)
	at com.facebook.presto.metadata.FunctionSignatureMatcher.identifyApplicableFunctions(FunctionSignatureMatcher.java:162)
	at com.facebook.presto.metadata.FunctionSignatureMatcher.matchFunctionGeneric(FunctionSignatureMatcher.java:96)
	at com.facebook.presto.metadata.FunctionSignatureMatcher.match(FunctionSignatureMatcher.java:74)
	at com.facebook.presto.metadata.FunctionAndTypeManager.getMatchingFunction(FunctionAndTypeManager.java:1092)
	at com.facebook.presto.metadata.FunctionAndTypeManager.getMatchingFunctionHandle(FunctionAndTypeManager.java:1043)
	at com.facebook.presto.metadata.FunctionAndTypeManager.resolveFunctionInternal(FunctionAndTypeManager.java:874)
	at com.facebook.presto.metadata.FunctionAndTypeManager.resolveFunction(FunctionAndTypeManager.java:626)
	at com.facebook.presto.metadata.FunctionAndTypeManager$1.resolveFunction(FunctionAndTypeManager.java:256)
	at com.facebook.presto.sql.analyzer.ExpressionAnalyzer.resolveFunction(ExpressionAnalyzer.java:1859)
	at com.facebook.presto.sql.analyzer.ExpressionAnalyzer$Visitor.visitFunctionCall(ExpressionAnalyzer.java:1111)
	at com.facebook.presto.sql.analyzer.ExpressionAnalyzer$Visitor.visitFunctionCall(ExpressionAnalyzer.java:390)
	at com.facebook.presto.sql.tree.FunctionCall.accept(FunctionCall.java:136)
	at com.facebook.presto.sql.tree.StackableAstVisitor.process(StackableAstVisitor.java:26)
	at com.facebook.presto.sql.analyzer.ExpressionAnalyzer$Visitor.process(ExpressionAnalyzer.java:413)
	at com.facebook.presto.sql.analyzer.ExpressionAnalyzer.analyze(ExpressionAnalyzer.java:351)
	at com.facebook.presto.sql.analyzer.ExpressionAnalyzer.analyzeExpression(ExpressionAnalyzer.java:1971)
	at com.facebook.presto.sql.analyzer.ExpressionAnalyzer.analyzeExpression(ExpressionAnalyzer.java:1955)
	at com.facebook.presto.sql.analyzer.StatementAnalyzer$Visitor.analyzeExpression(StatementAnalyzer.java:2988)
	at com.facebook.presto.sql.analyzer.StatementAnalyzer$Visitor.analyzeSelect(StatementAnalyzer.java:2806)
	at com.facebook.presto.sql.analyzer.StatementAnalyzer$Visitor.visitQuerySpecification(StatementAnalyzer.java:1798)
	at com.facebook.presto.sql.analyzer.StatementAnalyzer$Visitor.visitQuerySpecification(StatementAnalyzer.java:366)
	at com.facebook.presto.sql.tree.QuerySpecification.accept(QuerySpecification.java:138)
	at com.facebook.presto.sql.tree.AstVisitor.process(AstVisitor.java:27)
	at com.facebook.presto.sql.analyzer.StatementAnalyzer$Visitor.process(StatementAnalyzer.java:380)
	at com.facebook.presto.sql.analyzer.StatementAnalyzer$Visitor.process(StatementAnalyzer.java:388)
	at com.facebook.presto.sql.analyzer.StatementAnalyzer$Visitor.visitQuery(StatementAnalyzer.java:1202)
	at com.facebook.presto.sql.analyzer.StatementAnalyzer$Visitor.visitQuery(StatementAnalyzer.java:366)
	at com.facebook.presto.sql.tree.Query.accept(Query.java:105)
	at com.facebook.presto.sql.tree.AstVisitor.process(AstVisitor.java:27)
	at com.facebook.presto.sql.analyzer.StatementAnalyzer$Visitor.process(StatementAnalyzer.java:380)
	at com.facebook.presto.sql.analyzer.StatementAnalyzer.analyze(StatementAnalyzer.java:358)
	at com.facebook.presto.sql.analyzer.Analyzer.analyzeSemantic(Analyzer.java:121)
	at com.facebook.presto.sql.analyzer.BuiltInQueryAnalyzer.analyze(BuiltInQueryAnalyzer.java:95)
	at com.facebook.presto.execution.SqlQueryExecution.lambda$new$0(SqlQueryExecution.java:219)
	at com.facebook.presto.common.RuntimeStats.recordWallAndCpuTime(RuntimeStats.java:158)
	at com.facebook.presto.execution.SqlQueryExecution.<init>(SqlQueryExecution.java:219)
	at com.facebook.presto.execution.SqlQueryExecution.<init>(SqlQueryExecution.java:112)
	at com.facebook.presto.execution.SqlQueryExecution$SqlQueryExecutionFactory.createQueryExecution(SqlQueryExecution.java:1019)
	at com.facebook.presto.dispatcher.LocalDispatchQueryFactory.lambda$createDispatchQuery$0(LocalDispatchQueryFactory.java:169)
	at com.google.common.util.concurrent.TrustedListenableFutureTask$TrustedFutureInterruptibleTask.runInterruptibly(TrustedListenableFutureTask.java:131)
	at com.google.common.util.concurrent.InterruptibleTask.run(InterruptibleTask.java:75)
	at com.google.common.util.concurrent.TrustedListenableFutureTask.run(TrustedListenableFutureTask.java:82)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)
	at java.base/java.lang.Thread.run(Thread.java:1583)

Your Environment

• Presto version used:
• Storage (HDFS/S3/GCS..):
• Data source and connector used:
• Deployment (Cloud or On-prem):
• Pastebin link to the complete debug logs:

Expected Behavior

Expected behavior should be that Q1 and Q2 queries will succeed in all cases.

Current Behavior

array_sort requires specific casting of types.

Possible Solution

1. Relax the constraint that a FunctionSignatureMatcher must return at least one matching bound signature. Instead, just return an empty list when a semantic exception occurs.
2. Fix array_sort signature on Java coordinator to accept bigint instead of int
3. Have the function sidecar expose extra function signature

Steps to Reproduce

Run the queries in the above scenarios

Screenshots (if appropriate)

Context

[tdcmeehan]

The simplest solution is to just fix the signature, these were intended to be identical.

[kevintang2022]

@tdcmeehan Can you suggest some changes on the array_sort function signature? I want to test it out to see if it fixes this

[kevintang2022]

Also, it might be worth thinking about the FunctionSignatureMatcher error. It feels unexpected that it would throw an error when it's not able to find a match. I would think that if it can't find a match, it can just return an empty list

[tdcmeehan]

@kevintang2022 can you try changing the signatures in this file to bigint? I think because of type coercion, there should be no change in any queries as a result of this change.

presto/presto-main-base/src/main/java/com/facebook/presto/operator/scalar/ArraySortComparatorFunction.java

Line 51 in 47a7080

@SqlType("function(T, T, int)") ComparatorLongLambda function)

[Dilli-Babu-Godari]

@kevintang2022 can you try changing the signatures in this file to bigint? I think because of type coercion, there should be no change in any queries as a result of this change.

presto/presto-main-base/src/main/java/com/facebook/presto/operator/scalar/ArraySortComparatorFunction.java

Line 51 in 47a7080

@SqlType("function(T, T, int)") ComparatorLongLambda function)

I already made a PR for this but it requires a lot of tests to fix I guess.

[rschlussel]

I'm confused why it matters how the java function is registered. shouldn't it only matter that the sidecar is registered with the right signature?

[tdcmeehan]

@rschlussel for the pure sidecar, as evidenced from @kevintang2022's stack trace, it simply doesn't support the 2-arg comparator (since Velox doesn't directly support it, at least without some sort of internal rewriting that sometimes fails), and that's working as designed. This is just to make things work when we mix Java-defined functions for constant folding and C++-defined functions for function validation.

[kevintang2022]

@Dilli-Babu-Godari @tdcmeehan I just tested it out by updating signature from int -> bigint. It seems that for a regular java cluster without sidecar, it will allow Q2 to run successfully, but it will now break Q1

[tdcmeehan]

@kevintang2022 can you share the error? I'd like to understand why the type isn't being promoted.

[kevintang2022]

Q1 after changing siguature to bigint
https://pastebin.com/1Qbmjmid