You are viewing a plain text version of this content. The canonical link for it is here.
Posted to codereview@trafodion.apache.org by andyyangcn <gi...@git.apache.org> on 2018/04/27 03:20:58 UTC
[GitHub] trafodion pull request #1543: [TRAFODION 3047] Cannot get right result using...
GitHub user andyyangcn opened a pull request:
https://github.com/apache/trafodion/pull/1543
[TRAFODION 3047] Cannot get right result using prepare statement with…
… dynamic parameters
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/andyyangcn/incubator-trafodion jira3047
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafodion/pull/1543.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1543
----
commit db68b6f5c050874aa8ab86f10eae482f08c6cfc5
Author: Andy Yang <yo...@...>
Date: 2018-04-27T03:19:37Z
[TRAFODION 3047] Cannot get right result using prepare statement with dynamic parameters
----
---
[GitHub] trafodion pull request #1543: [TRAFODION 3047] Cannot get right result using...
Posted by andyyangcn <gi...@git.apache.org>.
Github user andyyangcn commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1543#discussion_r186910912
--- Diff: core/sql/optimizer/RelExpr.cpp ---
@@ -7880,11 +7880,17 @@ NABoolean GroupByAgg::tryToPullUpPredicatesInPreCodeGen(
else
pulledPredicates += tempPulledPreds;
+ // just remove pulled up predicates from char. input
+ ValueIdSet newInputs(getGroupAttr()->getCharacteristicInputs());
+ myLocalExpr += selectionPred();
+ myLocalExpr -= tempPulledPreds;
+ myLocalExpr.weedOutUnreferenced(newInputs);
--- End diff --
Hi Dave, I sent you a mail about this issue.
---
[GitHub] trafodion pull request #1543: [TRAFODION 3047] Cannot get right result using...
Posted by andyyangcn <gi...@git.apache.org>.
Github user andyyangcn commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1543#discussion_r185411453
--- Diff: core/sql/optimizer/RelExpr.cpp ---
@@ -7880,11 +7880,17 @@ NABoolean GroupByAgg::tryToPullUpPredicatesInPreCodeGen(
else
pulledPredicates += tempPulledPreds;
+ // just remove pulled up predicates from char. input
+ ValueIdSet newInputs(getGroupAttr()->getCharacteristicInputs());
+ myLocalExpr += selectionPred();
+ myLocalExpr -= tempPulledPreds;
+ myLocalExpr.weedOutUnreferenced(newInputs);
--- End diff --
For my test case, in the code you mentioned above, selectionPred() and tempPulledPreds is the same, and myLocalExpr doesn't include the dynamic parameter "?", but the dynamic parameter "?" is included in variable newInputs. So after myLocalExpr.weedOutUnreferenced(newInputs), the dynamic parameter "?" is removed from newINputs. And actually the predicate with dynamic parameter in this case is not pulled up. That cause we have a predicate with an expression "?" in selection predicate, but don't have the expression in character input. To my understand, character inputs are needed by selection predicates.
So, for this fix, I recompute the character input and just remove the expressions which are included in pulled up predicates.
---
[GitHub] trafodion pull request #1543: [TRAFODION 3047] Cannot get right result using...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1543#discussion_r186773837
--- Diff: core/sql/optimizer/RelExpr.cpp ---
@@ -7880,11 +7880,17 @@ NABoolean GroupByAgg::tryToPullUpPredicatesInPreCodeGen(
else
pulledPredicates += tempPulledPreds;
+ // just remove pulled up predicates from char. input
+ ValueIdSet newInputs(getGroupAttr()->getCharacteristicInputs());
+ myLocalExpr += selectionPred();
+ myLocalExpr -= tempPulledPreds;
+ myLocalExpr.weedOutUnreferenced(newInputs);
--- End diff --
Hi Andy: I tried this. I get one row back, "Y level code", which looks like the correct result. I tried varying the query, replacing the parameter '?' with the value '12345678' and got the same answer. I got the same query plan for both formulations. So I don't think I successfully reproduced the problem. What query plan do you get in the failing case?
---
[GitHub] trafodion pull request #1543: [TRAFODION 3047] Cannot get right result using...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1543#discussion_r187433575
--- Diff: core/sql/optimizer/RelExpr.cpp ---
@@ -7880,11 +7880,17 @@ NABoolean GroupByAgg::tryToPullUpPredicatesInPreCodeGen(
else
pulledPredicates += tempPulledPreds;
+ // just remove pulled up predicates from char. input
+ ValueIdSet newInputs(getGroupAttr()->getCharacteristicInputs());
+ myLocalExpr += selectionPred();
+ myLocalExpr -= tempPulledPreds;
+ myLocalExpr.weedOutUnreferenced(newInputs);
--- End diff --
I stepped through the code with your test case that reproduces the error. Looks like your fix does the right thing.
---
[GitHub] trafodion pull request #1543: [TRAFODION 3047] Cannot get right result using...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1543#discussion_r185329230
--- Diff: core/sql/optimizer/RelExpr.cpp ---
@@ -7880,11 +7880,17 @@ NABoolean GroupByAgg::tryToPullUpPredicatesInPreCodeGen(
else
pulledPredicates += tempPulledPreds;
+ // just remove pulled up predicates from char. input
+ ValueIdSet newInputs(getGroupAttr()->getCharacteristicInputs());
+ myLocalExpr += selectionPred();
+ myLocalExpr -= tempPulledPreds;
+ myLocalExpr.weedOutUnreferenced(newInputs);
--- End diff --
Could you explain what is going on in this code? I notice that earlier in the method we already added selectionPred() to myLocalExpr, and then removed tempPulledPreds. I'm wondering why we are doing this again. Also, It's not obvious to me why newInputs will have different values than myNewInputs, which went through a similar calculation. An example might help.
---
[GitHub] trafodion pull request #1543: [TRAFODION 3047] Cannot get right result using...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1543#discussion_r186465542
--- Diff: core/sql/optimizer/RelExpr.cpp ---
@@ -7880,11 +7880,17 @@ NABoolean GroupByAgg::tryToPullUpPredicatesInPreCodeGen(
else
pulledPredicates += tempPulledPreds;
+ // just remove pulled up predicates from char. input
+ ValueIdSet newInputs(getGroupAttr()->getCharacteristicInputs());
+ myLocalExpr += selectionPred();
+ myLocalExpr -= tempPulledPreds;
+ myLocalExpr.weedOutUnreferenced(newInputs);
--- End diff --
I'd like to step through this example myself to understand it better. Could you provide the DDL to the tables mentioned in the JIRA? Thanks.
---
[GitHub] trafodion pull request #1543: [TRAFODION 3047] Cannot get right result using...
Posted by andyyangcn <gi...@git.apache.org>.
Github user andyyangcn commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1543#discussion_r186602771
--- Diff: core/sql/optimizer/RelExpr.cpp ---
@@ -7880,11 +7880,17 @@ NABoolean GroupByAgg::tryToPullUpPredicatesInPreCodeGen(
else
pulledPredicates += tempPulledPreds;
+ // just remove pulled up predicates from char. input
+ ValueIdSet newInputs(getGroupAttr()->getCharacteristicInputs());
+ myLocalExpr += selectionPred();
+ myLocalExpr -= tempPulledPreds;
+ myLocalExpr.weedOutUnreferenced(newInputs);
--- End diff --
Dave, you can use the follow SQLs to reproduce this issue.
create table dom(area_id varchar(50), type_cd varchar(4));
create table dic(area_id varchar(50) );
CREATE TABLE CODE
(
LANG_CD VARCHAR(10)
, UP_CD_ID VARCHAR(50)
, CD_ID VARCHAR(50)
, CD_NM VARCHAR(100)
);
insert into dom values('12345678', '0003');
insert into dic values('12345678');
insert into code values('zh_CN','6023', '0003', 'level code');
prepare s1 from SELECT (SELECT 'Y'
FROM dual
WHERE EXISTS (SELECT 1
FROM dic
WHERE area_id = dom.area_id)) impact_yn
, (SELECT cd_nm
FROM code
WHERE lang_cd = 'zh_CN'
AND up_cd_id = '6023'
AND cd_id = dom.type_cd) dic_gbn_nm
FROM dom
WHERE dom.area_id = ?
AND dom.type_cd = '0003';
execute s1 using '12345678';
---
[GitHub] trafodion pull request #1543: [TRAFODION 3047] Cannot get right result using...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/trafodion/pull/1543
---
[GitHub] trafodion pull request #1543: [TRAFODION 3047] Cannot get right result using...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1543#discussion_r187139067
--- Diff: core/sql/optimizer/RelExpr.cpp ---
@@ -7880,11 +7880,17 @@ NABoolean GroupByAgg::tryToPullUpPredicatesInPreCodeGen(
else
pulledPredicates += tempPulledPreds;
+ // just remove pulled up predicates from char. input
+ ValueIdSet newInputs(getGroupAttr()->getCharacteristicInputs());
+ myLocalExpr += selectionPred();
+ myLocalExpr -= tempPulledPreds;
+ myLocalExpr.weedOutUnreferenced(newInputs);
--- End diff --
Hi Andy. Thanks! I figured out why I could not reproduce the issue. (The issue is that the parameterized version of the query returns zero rows when it should return one.) There is another change which I recently merged, https://github.com/apache/trafodion/pull/1546, that either fixes this problem or masks it. I am not sure which yet. If I set CQD COMP_BOOL_158 'OFF', I can now reproduce the problem on the latest code. I'll spend some time playing with your code change today or tomorrow and evaluate it. Thanks.
---