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 parameters order in EXECUTE USING when using with clause #1191

Closed
archongum opened this issue Jul 26, 2019 · 5 comments · Fixed by #1529
Closed

Wrong parameters order in EXECUTE USING when using with clause #1191

archongum opened this issue Jul 26, 2019 · 5 comments · Fixed by #1529
Assignees
Labels
bug Something isn't working correctness
Milestone

Comments

@archongum
Copy link
Member

env

  • Presto JDBC: prestosql-315
  • Mybatis: 3.5.1

If using with clause

PREPARE statement1 FROM 
with src as (
	select
		*
	from tpch.sf1.customer
	where mktsegment = ?
)
select
	*
from src
where
	custkey between ? and ?

Mybatis handle with left-to-right as they appear

EXECUTE statement1 using 'HOUSEHOLD', BIGINT '0', BIGINT '10'

But Presto is expecting

EXECUTE statement1 USING BIGINT '0', BIGINT '10', 'HOUSEHOLD'

If without with clause

PREPARE statement1 FROM
select
	*
from (
	select
		*
	from tpch.sf1.customer
	where mktsegment = ?
)src
where
	custkey between ? and ?

Presto is expecting the same order as mybatis is generated

EXECUTE statement1 using 'HOUSEHOLD', BIGINT '0', BIGINT '10'
@sopel39
Copy link
Member

sopel39 commented Jul 26, 2019

I would expect parameter order to be the same independently whether with is used. This seems like a bug. Do we know what spec says @martint ?

@findepi findepi added bug Something isn't working correctness labels Jul 26, 2019
@ankitdixit ankitdixit self-assigned this Jul 26, 2019
@ankitdixit
Copy link
Member

The issue seems to be that AstBuilder part calls visitParameter for the query first and the With statement later, hence the id assigned to paramter corresponding to mktsegment = ? is 2.
custkey between ?(id=0) and ?(id=1) and mktsegment = ? (id=2) which is wrong.
While fetching the values ExpressionAnalyzer.visitParameters() calls parameters.get(node.getPosition()) taking the sequential order and for mktsegment = ? picks the value at position 2.

@sopel39 Need help to figure out how to make sure that with part is processed first so that parameter ids are allocated in sequential order

@martint
Copy link
Member

martint commented Jul 26, 2019

What we need to do walk the query tree and collect all the parameters, sort them by their location and record them in the analysis.

Btw, I think @Praveen2112 is also looking into this, so please coordinate with him to avoid duplicating work.

@martint
Copy link
Member

martint commented Jul 26, 2019

So, the position should not be recorded during analysis, not during parsing. The tricky bit is going to be ExpressionFormatter.visitParameter, which currently relies on the AST node to indicate which position it's in in order to substitute with the actual parameter value before printing. On the other hand, ExpressionFormatter should not be doing that kind of substitution -- we should track down what depends on this.

@ankitdixit
Copy link
Member

@martint Had an offline discussion with @Praveen2112, he is taking this forward.

Btw, I think @Praveen2112 is also looking into this, so please coordinate with him to avoid duplicating work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working correctness
Development

Successfully merging a pull request may close this issue.

6 participants