You are viewing a plain text version of this content. The canonical link for it is here.
Posted to codereview@trafodion.apache.org by Guhaiyan <gi...@git.apache.org> on 2018/05/28 01:50:33 UTC
[GitHub] trafodion pull request #1581: [TRAFODION-2901] Self-reference Holloween prob...
GitHub user Guhaiyan opened a pull request:
https://github.com/apache/trafodion/pull/1581
[TRAFODION-2901] Self-reference Holloween problem fix
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Guhaiyan/traffork_ghy jira2901
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafodion/pull/1581.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 #1581
----
commit c8c5bce5baf23c131aa622c6ca6672daa0eb2318
Author: Guhaiyan <ha...@...>
Date: 2018-05-28T01:40:27Z
[TRAFODION-2901] Self-reference Holloween problem fix
----
---
[GitHub] trafodion pull request #1581: [TRAFODION-2901] Self-reference Holloween prob...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1581#discussion_r191576581
--- Diff: core/sql/optimizer/RelExpr.cpp ---
@@ -13341,6 +13345,33 @@ void GenericUpdate::pushdownCoveredExpr(const ValueIdSet &outputExpr,
newExternalInputs,
predicatesOnParent,
&localExprs);
+
+ if (avoidHalloween() && child(0) && child(0)->getGroupAttr())
+ {
+ ValueIdSet cur_output = child(0)->getGroupAttr()->getCharacteristicOutputs();
+ if (cur_output.isEmpty())
+ {
+ ValueId exprId;
+ ValueId atLeastOne;
+
+ for (exprId = original_output.init();
+ original_output.next(exprId);
+ original_output.advance(exprId))
+ {
+ atLeastOne = exprId;
+ if (!(exprId.getItemExpr()->doesExprEvaluateToConstant(FALSE, TRUE)))
+ {
+ child(0)->getGroupAttr()->addCharacteristicOutputs(exprId);
+ break;
+ }
+ }
+ cur_output = child(0)->getGroupAttr()->getCharacteristicOutputs();
+ if (cur_output.isEmpty())
--- End diff --
I don't understand this code path. If cur_output is empty, that means that the "for" loop did not find a ValueId to add to it. In particular. "atLeastOne" has its initial value, which I think is the null ValueId, 0. This "if" would cause us to add the null ValueId to child(0)'s characteristic outputs, which does not sound correct to me.
---
[GitHub] trafodion pull request #1581: [TRAFODION-2901] Self-reference Holloween prob...
Posted by Guhaiyan <gi...@git.apache.org>.
Github user Guhaiyan commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1581#discussion_r191620727
--- Diff: core/sql/optimizer/RelExpr.cpp ---
@@ -13341,6 +13345,33 @@ void GenericUpdate::pushdownCoveredExpr(const ValueIdSet &outputExpr,
newExternalInputs,
predicatesOnParent,
&localExprs);
+
+ if (avoidHalloween() && child(0) && child(0)->getGroupAttr())
+ {
+ ValueIdSet cur_output = child(0)->getGroupAttr()->getCharacteristicOutputs();
+ if (cur_output.isEmpty())
+ {
+ ValueId exprId;
+ ValueId atLeastOne;
+
+ for (exprId = original_output.init();
+ original_output.next(exprId);
+ original_output.advance(exprId))
+ {
+ atLeastOne = exprId;
+ if (!(exprId.getItemExpr()->doesExprEvaluateToConstant(FALSE, TRUE)))
+ {
+ child(0)->getGroupAttr()->addCharacteristicOutputs(exprId);
+ break;
+ }
+ }
+ cur_output = child(0)->getGroupAttr()->getCharacteristicOutputs();
+ if (cur_output.isEmpty())
--- End diff --
If cur_output is empty, it means that in RelExpr::pushdownCoveredExpr, child(0).getPtr()->getGroupAttr()->computeCharacteristicIO set requiredOutputs_ to be empty, that is there is no output for Scan. So I go through the original_output and pick a ValueId which does NOT EvaluateToConst and add it to the characteristic output of Scan. Since the original_output is not empty, so we can always find one ValueId to be added to Scan's output . After "for" loop, atLeastOne is the last ValueId in the original_output, not its initial value(0), so I do not think the "if" would cause adding an null ValueId in Scan's output.
---
[GitHub] trafodion pull request #1581: [TRAFODION-2901] Self-reference Holloween prob...
Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1581#discussion_r196475078
--- Diff: core/sql/optimizer/RelExpr.cpp ---
@@ -13341,6 +13341,36 @@ void GenericUpdate::pushdownCoveredExpr(const ValueIdSet &outputExpr,
newExternalInputs,
predicatesOnParent,
&localExprs);
+
+ if (avoidHalloween() && child(0) &&
+ child(0)->getOperatorType() == REL_SCAN &&
+ child(0)->getGroupAttr())
+ {
+ ValueIdSet cur_output = child(0)->getGroupAttr()->getCharacteristicOutputs();
+ if (cur_output.isEmpty())
+ {
+ ValueId exprId;
+ ValueId atLeastOne;
+
+ ValueIdSet output_source = child(0)->getTableDescForExpr()->getColumnList();
+ for (exprId = output_source.init();
+ output_source.next(exprId);
+ output_source.advance(exprId))
+ {
+ atLeastOne = exprId;
+ if (!(exprId.getItemExpr()->doesExprEvaluateToConstant(FALSE, TRUE)))
+ {
+ child(0)->getGroupAttr()->addCharacteristicOutputs(exprId);
+ break;
+ }
+ }
+ cur_output = child(0)->getGroupAttr()->getCharacteristicOutputs();
--- End diff --
Same comment as above, on line 13349.
---
[GitHub] trafodion pull request #1581: [TRAFODION-2901] Self-reference Holloween prob...
Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1581#discussion_r196474928
--- Diff: core/sql/optimizer/RelExpr.cpp ---
@@ -13341,6 +13341,36 @@ void GenericUpdate::pushdownCoveredExpr(const ValueIdSet &outputExpr,
newExternalInputs,
predicatesOnParent,
&localExprs);
+
+ if (avoidHalloween() && child(0) &&
+ child(0)->getOperatorType() == REL_SCAN &&
+ child(0)->getGroupAttr())
+ {
+ ValueIdSet cur_output = child(0)->getGroupAttr()->getCharacteristicOutputs();
--- End diff --
This works, but is is not very efficient to create a copy of a ValueIdSet just to check whether it is empty. You could just check
```
if (child(0)->getGroupAttr()->getCharacteristicOutputs().isEmpty())
```
in the line below.
---
[GitHub] trafodion pull request #1581: [TRAFODION-2901] Self-reference Holloween prob...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1581#discussion_r191817577
--- Diff: core/sql/optimizer/RelExpr.cpp ---
@@ -13341,6 +13345,33 @@ void GenericUpdate::pushdownCoveredExpr(const ValueIdSet &outputExpr,
newExternalInputs,
predicatesOnParent,
&localExprs);
+
+ if (avoidHalloween() && child(0) && child(0)->getGroupAttr())
+ {
+ ValueIdSet cur_output = child(0)->getGroupAttr()->getCharacteristicOutputs();
+ if (cur_output.isEmpty())
+ {
+ ValueId exprId;
+ ValueId atLeastOne;
+
+ for (exprId = original_output.init();
+ original_output.next(exprId);
+ original_output.advance(exprId))
+ {
+ atLeastOne = exprId;
+ if (!(exprId.getItemExpr()->doesExprEvaluateToConstant(FALSE, TRUE)))
+ {
+ child(0)->getGroupAttr()->addCharacteristicOutputs(exprId);
+ break;
+ }
+ }
+ cur_output = child(0)->getGroupAttr()->getCharacteristicOutputs();
+ if (cur_output.isEmpty())
--- End diff --
Ah, thank you! I misread the code. But is it possible that original_output itself is empty? If so, the "for" loop would not be entered and exprId would remain 0. Do we need a check for that?
---
[GitHub] trafodion pull request #1581: [TRAFODION-2901] Self-reference Holloween prob...
Posted by Guhaiyan <gi...@git.apache.org>.
Github user Guhaiyan commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1581#discussion_r191980234
--- Diff: core/sql/optimizer/RelExpr.cpp ---
@@ -13341,6 +13345,33 @@ void GenericUpdate::pushdownCoveredExpr(const ValueIdSet &outputExpr,
newExternalInputs,
predicatesOnParent,
&localExprs);
+
+ if (avoidHalloween() && child(0) && child(0)->getGroupAttr())
+ {
+ ValueIdSet cur_output = child(0)->getGroupAttr()->getCharacteristicOutputs();
+ if (cur_output.isEmpty())
+ {
+ ValueId exprId;
+ ValueId atLeastOne;
+
+ for (exprId = original_output.init();
+ original_output.next(exprId);
+ original_output.advance(exprId))
+ {
+ atLeastOne = exprId;
+ if (!(exprId.getItemExpr()->doesExprEvaluateToConstant(FALSE, TRUE)))
+ {
+ child(0)->getGroupAttr()->addCharacteristicOutputs(exprId);
+ break;
+ }
+ }
+ cur_output = child(0)->getGroupAttr()->getCharacteristicOutputs();
+ if (cur_output.isEmpty())
--- End diff --
Thank for the comment. I think your concern about the empty original_output seems reasonable. After binder, the original characteristic output for Scan consists of getTableDesc()->getColumnList() and getTableDesc()->hbaseAttrList(). But it is hard to decide what happened to the output during transformer. So there may be possibility that the characteristic output for Scan becomes empty after transform, and I am not quite sure about that.
So I think the simplest way is taking child(0)->getTableDescForExpr()->getColumnList() as the output source picked from, instead of original_output in the PR. In this way, the source output will never be empty.
---
[GitHub] trafodion pull request #1581: [TRAFODION-2901] Self-reference Holloween prob...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/trafodion/pull/1581
---