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.

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

Wrong query results when lambda scope shadows column name #9025

Closed
findepi opened this issue Sep 21, 2017 · 4 comments
Closed

Wrong query results when lambda scope shadows column name #9025

findepi opened this issue Sep 21, 2017 · 4 comments

Comments

@findepi
Copy link
Contributor

findepi commented Sep 21, 2017

It's not possible to use dereference expressions in lambda context (#9023) except when dereferencing lambda argument's field (789e477).

However, even in the "supported" case, result is different when lambda arguement shadows a column reference:

SELECT transform(array[CAST(ROW(10) AS ROW(x INTEGER))], r -> r.x) FROM (VALUES 1) u(x);
 _col0 
-------
 [10]  
(1 row)

SELECT transform(array[CAST(ROW(10) AS ROW(x INTEGER))], r -> r.x) FROM (VALUES 1) r(x);
 _col0 
-------
 [1]   
(1 row)

IMO both queries should produce 10, as column is being shadowed by lambda's formal argument.

@findepi
Copy link
Contributor Author

findepi commented Sep 21, 2017

Because of this resolution mismatch, the following query fails:

presto> SELECT transform(array[CAST(ROW(10) AS ROW(x INTEGER))], r -> r.x) FROM (VALUES 'a') r(x);

Query 20170921_143253_00000_nbg6x failed: type of symbol 'transform' is expected to be array(integer), but the actual type is array(varchar(1))
java.lang.IllegalArgumentException: type of symbol 'transform' is expected to be array(integer), but the actual type is array(varchar(1))
	at com.google.common.base.Preconditions.checkArgument(Preconditions.java:399)
	at com.facebook.presto.sql.planner.sanity.TypeValidator$Visitor.verifyTypeSignature(TypeValidator.java:188)
	at com.facebook.presto.sql.planner.sanity.TypeValidator$Visitor.visitProject(TypeValidator.java:120)
	at com.facebook.presto.sql.planner.sanity.TypeValidator$Visitor.visitProject(TypeValidator.java:60)
	at com.facebook.presto.sql.planner.plan.ProjectNode.accept(ProjectNode.java:92)
	at com.facebook.presto.sql.planner.SimplePlanVisitor.visitPlan(SimplePlanVisitor.java:26)
	at com.facebook.presto.sql.planner.sanity.TypeValidator$Visitor.visitProject(TypeValidator.java:109)
	at com.facebook.presto.sql.planner.sanity.TypeValidator$Visitor.visitProject(TypeValidator.java:60)
	at com.facebook.presto.sql.planner.plan.ProjectNode.accept(ProjectNode.java:92)
	at com.facebook.presto.sql.planner.SimplePlanVisitor.visitPlan(SimplePlanVisitor.java:26)
	at com.facebook.presto.sql.planner.sanity.TypeValidator$Visitor.visitProject(TypeValidator.java:109)
	at com.facebook.presto.sql.planner.sanity.TypeValidator$Visitor.visitProject(TypeValidator.java:60)
	at com.facebook.presto.sql.planner.plan.ProjectNode.accept(ProjectNode.java:92)
	at com.facebook.presto.sql.planner.SimplePlanVisitor.visitPlan(SimplePlanVisitor.java:26)
	at com.facebook.presto.sql.planner.sanity.TypeValidator$Visitor.visitProject(TypeValidator.java:109)
	at com.facebook.presto.sql.planner.sanity.TypeValidator$Visitor.visitProject(TypeValidator.java:60)
	at com.facebook.presto.sql.planner.plan.ProjectNode.accept(ProjectNode.java:92)
	at com.facebook.presto.sql.planner.SimplePlanVisitor.visitPlan(SimplePlanVisitor.java:26)
	at com.facebook.presto.sql.planner.SimplePlanVisitor.visitPlan(SimplePlanVisitor.java:19)
	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.sanity.TypeValidator.validate(TypeValidator.java:57)
	at com.facebook.presto.sql.planner.sanity.PlanSanityChecker.lambda$validateIntermediatePlan$1(PlanSanityChecker.java:59)
	at com.google.common.collect.ImmutableList.forEach(ImmutableList.java:408)
	at com.facebook.presto.sql.planner.sanity.PlanSanityChecker.validateIntermediatePlan(PlanSanityChecker.java:59)

cc @martint

@findepi
Copy link
Contributor Author

findepi commented Oct 3, 2017

i suspect bug to hide in/near TranslationMap, so #7398 could be a solution.

@findepi
Copy link
Contributor Author

findepi commented Oct 3, 2017

Probable bug location: com.facebook.presto.sql.tree.ExpressionRewriter#rewriteDereferenceExpression:

public Expression rewriteDereferenceExpression(DereferenceExpression node, Void context, ExpressionTreeRewriter<Void> treeRewriter)
{
    Optional<ResolvedField> resolvedField = rewriteBase.getScope().tryResolveField(node);
    ....

here we use some scope to resolve a DereferenceExpression but this is the same fixed scope even if we enter a lambda. Thus lambda doesn't effectively shadow outer scope, but it should.

@sopel39
Copy link
Contributor

sopel39 commented Nov 2, 2017

waits for release to complete before merging #9099 again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants