Presto: Unsupported RowExpression type LambdaDefinitionExpression

[ggreg]

Reproducible query:

WITH t AS (
  SELECT * FROM (VALUES (1, 2, 'c', 'd', '2018-11-01')) AS t (a, b, c, d, ds)
)
SELECT label
FROM (
  SELECT IF (p <= q, '1', '0') AS label
  FROM (
      SELECT
        reduce(               
          array_agg(a),
          CAST(Row(0.0, 0.0) AS ROW(field0 DOUBLE, field1 DOUBLE)),
          (s, x) -> CAST(ROW(s.field0, s.field1) AS ROW(field0 DOUBLE, field1 DOUBLE)),
          s -> s.field0 + s.field1
        ) as p,
        reduce(               
          array_agg(a),             
          CAST(Row(0.0, 0.0) AS ROW(field0 DOUBLE, field1 DOUBLE)),
          (s, x) -> CAST(ROW(s.field0, s.field1) AS ROW(field0 DOUBLE, field1 DOUBLE)),
          s -> s.field0 + s.field1
        ) as q
      FROM t
      WHERE ds = '2018-11-01' AND d = 'test' GROUP BY c
   )
WHERE p >= 0.75 AND q >= 0.75)

The query above works if i remove WHERE p >= 0.75 AND q >= 0.75.

I found commit 92b8a88 by bisecting from 0.213.

[ggreg]

Hold on, confirming by running the query against the commit again.

[sopel39]

which commit is it?

[ggreg]

Confirmed it is 92b8a88

[sopel39]

What is the stacktrace?

[ggreg]

Query 20181112_203438_00005_yzjhw failed: Unsupported RowExpression type LambdaDefinitionExpression
java.lang.IllegalArgumentException: Unsupported RowExpression type LambdaDefinitionExpression
    at com.facebook.presto.sql.planner.optimizations.ExpressionEquivalence$RowExpressionComparator.compare(ExpressionEquivalence.java:295)
    at com.facebook.presto.sql.planner.optimizations.ExpressionEquivalence$RowExpressionComparator.compare(ExpressionEquivalence.java:225)
    at com.facebook.presto.sql.planner.optimizations.ExpressionEquivalence$ListComparator.compare(ExpressionEquivalence.java:314)
    at com.facebook.presto.sql.planner.optimizations.ExpressionEquivalence$ListComparator.compare(ExpressionEquivalence.java:299)
    at com.google.common.collect.ComparisonChain$1.compare(ComparisonChain.java:79)
    at com.facebook.presto.sql.planner.optimizations.ExpressionEquivalence$RowExpressionComparator.compare(ExpressionEquivalence.java:244)
    at com.facebook.presto.sql.planner.optimizations.ExpressionEquivalence$RowExpressionComparator.compare(ExpressionEquivalence.java:225)
    at com.facebook.presto.sql.planner.optimizations.ExpressionEquivalence$ListComparator.compare(ExpressionEquivalence.java:314)
    at com.facebook.presto.sql.planner.optimizations.ExpressionEquivalence$ListComparator.compare(ExpressionEquivalence.java:299)
    at com.google.common.collect.ComparisonChain$1.compare(ComparisonChain.java:79)
    at com.facebook.presto.sql.planner.optimizations.ExpressionEquivalence$RowExpressionComparator.compare(ExpressionEquivalence.java:244)
    at com.facebook.presto.sql.planner.optimizations.ExpressionEquivalence$RowExpressionComparator.compare(ExpressionEquivalence.java:225)
    at com.google.common.collect.ComparatorOrdering.compare(ComparatorOrdering.java:37)
    at java.util.TimSort.countRunAndMakeAscending(TimSort.java:355)
    at java.util.TimSort.sort(TimSort.java:220)
    at java.util.Arrays.sort(Arrays.java:1438)
    at com.google.common.collect.Ordering.sortedCopy(Ordering.java:852)
    at com.facebook.presto.sql.planner.optimizations.ExpressionEquivalence$CanonicalizationVisitor.visitCall(ExpressionEquivalence.java:144)
    at com.facebook.presto.sql.planner.optimizations.ExpressionEquivalence$CanonicalizationVisitor.visitCall(ExpressionEquivalence.java:118)
    at com.facebook.presto.sql.relational.CallExpression.accept(CallExpression.java:88)
    at com.facebook.presto.sql.planner.optimizations.ExpressionEquivalence.areExpressionsEquivalent(ExpressionEquivalence.java:93)
    at com.facebook.presto.sql.planner.optimizations.PredicatePushDown$Rewriter.visitFilter(PredicatePushDown.java:345)
    at com.facebook.presto.sql.planner.optimizations.PredicatePushDown$Rewriter.visitFilter(PredicatePushDown.java:128)
    at com.facebook.presto.sql.planner.plan.FilterNode.accept(FilterNode.java:72)
    at com.facebook.presto.sql.planner.plan.SimplePlanRewriter$RewriteContext.rewrite(SimplePlanRewriter.java:84)
    at com.facebook.presto.sql.planner.plan.SimplePlanRewriter$RewriteContext.lambda$defaultRewrite$0(SimplePlanRewriter.java:73)
    at java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:193)
    at java.util.Collections$2.tryAdvance(Collections.java:4717)
    at java.util.Collections$2.forEachRemaining(Collections.java:4725)
    at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:481)
    at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:471)
    at java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:708)
    at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
    at java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:499)
    at com.facebook.presto.sql.planner.plan.SimplePlanRewriter$RewriteContext.defaultRewrite(SimplePlanRewriter.java:74)
    at com.facebook.presto.sql.planner.optimizations.PredicatePushDown$Rewriter.visitProject(PredicatePushDown.java:235)
    at com.facebook.presto.sql.planner.optimizations.PredicatePushDown$Rewriter.visitProject(PredicatePushDown.java:128)
    at com.facebook.presto.sql.planner.plan.ProjectNode.accept(ProjectNode.java:92)
    at com.facebook.presto.sql.planner.plan.SimplePlanRewriter$RewriteContext.rewrite(SimplePlanRewriter.java:84)
    at com.facebook.presto.sql.planner.plan.SimplePlanRewriter$RewriteContext.lambda$defaultRewrite$0(SimplePlanRewriter.java:73)
    at java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:193)
    at java.util.Collections$2.tryAdvance(Collections.java:4717)
    at java.util.Collections$2.forEachRemaining(Collections.java:4725)
    at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:481)
    at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:471)
    at java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:708)
    at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
    at java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:499)
    at com.facebook.presto.sql.planner.plan.SimplePlanRewriter$RewriteContext.defaultRewrite(SimplePlanRewriter.java:74)
    at com.facebook.presto.sql.planner.optimizations.PredicatePushDown$Rewriter.visitProject(PredicatePushDown.java:235)
    at com.facebook.presto.sql.planner.optimizations.PredicatePushDown$Rewriter.visitProject(PredicatePushDown.java:128)
    at com.facebook.presto.sql.planner.plan.ProjectNode.accept(ProjectNode.java:92)
    at com.facebook.presto.sql.planner.plan.SimplePlanRewriter$RewriteContext.rewrite(SimplePlanRewriter.java:84)
    at com.facebook.presto.sql.planner.plan.SimplePlanRewriter$RewriteContext.lambda$defaultRewrite$0(SimplePlanRewriter.java:73)
    at java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:193)
    at java.util.Collections$2.tryAdvance(Collections.java:4717)
    at java.util.Collections$2.forEachRemaining(Collections.java:4725)
    at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:481)
    at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:471)
    at java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:708)
    at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
    at java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:499)
    at com.facebook.presto.sql.planner.plan.SimplePlanRewriter$RewriteContext.defaultRewrite(SimplePlanRewriter.java:74)
    at com.facebook.presto.sql.planner.optimizations.PredicatePushDown$Rewriter.visitPlan(PredicatePushDown.java:165)
    at com.facebook.presto.sql.planner.optimizations.PredicatePushDown$Rewriter.visitPlan(PredicatePushDown.java:128)
    at com.facebook.presto.sql.planner.plan.PlanVisitor.visitOutput(PlanVisitor.java:49)
    at com.facebook.presto.sql.planner.plan.OutputNode.accept(OutputNode.java:82)
    at com.facebook.presto.sql.planner.plan.SimplePlanRewriter.rewriteWith(SimplePlanRewriter.java:32)
    at com.facebook.presto.sql.planner.optimizations.PredicatePushDown.optimize(PredicatePushDown.java:122)
    at com.facebook.presto.sql.planner.optimizations.StatsRecordingPlanOptimizer.optimize(StatsRecordingPlanOptimizer.java:51)
    at com.facebook.presto.sql.planner.LogicalPlanner.plan(LogicalPlanner.java:163)
    at com.facebook.presto.sql.planner.LogicalPlanner.plan(LogicalPlanner.java:152)
    at com.facebook.presto.execution.SqlQueryExecution.doAnalyzeQuery(SqlQueryExecution.java:397)
    at com.facebook.presto.execution.SqlQueryExecution.analyzeQuery(SqlQueryExecution.java:382)
    at com.facebook.presto.execution.SqlQueryExecution.start(SqlQueryExecution.java:326)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
    at java.lang.Thread.run(Thread.java:748)

[sopel39]

Oh I see. Is this a release blocker?

[ggreg]

Yes, i was planning to revert the commit. What do you think?

[sopel39]

Sure. You can revert it (if you are in a hurry) and I can address that later on. Keep in mind that the problem will still occur if lambdas are part of join filters as expression equivalence is broken for them.

[martint]

@ggreg, unless the commit is fundamentally broken, we should fix the underlying issue, not revert it.

[martint]

Specifically, we don’t yet understand what’s the issue: is that commit bad or did it uncover a latent issue somewhere else?

[sopel39]

The problem is with ExpressionEquivalence.RowExpressionComparator#compare which lacks support for LambdaDefinitionExpression and VariableReferenceExpression
Adding support for them shouldn't be hard though.

[sopel39]

Specifically, we don’t yet understand what’s the issue: is that commit bad or did it uncover a latent issue somewhere else?

@martint I think it's the latter.

[ggreg]

Thanks for the clarification, we'll wait for the fix

[ggreg]

Yes, i was planning to revert the commit. What do you think?

This statement was confusing. My intent was to let @sopel39 (as the author of the commit and someone who has more context about why it was needed) evaluate the impact and discuss if we should revert the commit or fix it. I should have expressed it in a better way :).

[ggreg]

The bug was found when running the verifier. We have a sample of more than 10 queries that triggers it and fail with the same stack trace. During the verifier runs, we used Presto 0.213 in production as the control.

[ggreg]

Let me know if another query would help to debug