Github user zellerh commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1585#discussion_r191867444
--- Diff: core/sql/optimizer/ItemExpr.cpp ---
@@ -3886,23 +3886,7 @@ ItemExpr * Parameter::copyTopNode(ItemExpr
*derivedNode, CollHeap* outHeap)
ItemExpr * DynamicParam::copyTopNode(ItemExpr *derivedNode, CollHeap*
outHeap)
{
- ItemExpr *result;
-
- if (derivedNode == NULL) {
- result = new (outHeap) DynamicParam(paramName_, indicatorName_,
outHeap);
- ((DynamicParam *) result)->setRowsetSize(rowsetSize_);
- ((DynamicParam *) result)->setRowsetInfo(rowsetInfo_);
- ((DynamicParam *) result)->setParamHeading(heading_);
- ((DynamicParam *) result)->setParamTablename(tablename_);
- // we remember our original dynamic parameter because we
- // must use their valueid at dynamicparam::codegen time
- ((DynamicParam *) result)->setOriginal(this);
- }
-
- else
- result = derivedNode;
-
- return Parameter::copyTopNode(result, outHeap);
+ return this;
--- End diff --
+1
Andy and I discussed this and I like the fix. There are certain ItemExprs
like parameters and BaseColumns that should never be copied. We may have
others, like bound aggregate functions, constants, cuts that also should not be
copied. The copyTopNode and copyTree methods were initially used in very
restricted situations, but we now use them in many more places and some issues
creep in. I think this fix is an improvement over the existing solution.
Someday we should probably fix BaseColumn::copyTopNode in the same way, I doubt
that the current implementation works correctly.
My definition of copyTopNode is that it should create a new copy of a tree
that can be modified and bound _where it is semantically meaningful_, but
things like parameters and base columns refer to and represent (with their
value id) existing objects that cannot be copied. At least, they cannot be
copied as a side-effect of copyTopNode().
Examples: In this particular query, we want to copy an unbound tree as part
of the WITH clause handling. IMHO, sharing the parameter between the two copies
is the right thing to do.
Another example would be making a copy of an unbound aggregate in such a
context (WITH clause parsing), which is probably ok. Making a copy of a bound
aggregate in a later phase would be incorrect, however, because that aggregate
would not be in the aggregate expression of any groupby.
A similar situation exists with the copy of a BaseColumn node. Its new
ValueId would not appear in the column list of any TableDescriptor, so it would
not be meaningful. Copying the unbound form of a BaseColumn, a ColReference, is
ok.
Sorry for the long answer. We probably need to do more work in this area
and define more clearly what we mean by copying trees.
---
