[
https://issues.apache.org/jira/browse/PHOENIX-1030?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14148593#comment-14148593
]
James Taylor commented on PHOENIX-1030:
---------------------------------------
Thanks for the patch, [~tdsilva]. Here's some feedback:
- For this code, why aren't you using your Determinism.combine method in place
of the &= ? Then you'd keep the LiteralExpression.newConstant(null, theType,
isDeterministic); as is, no? Would be good to have a lower-level unit test
around this.
{code}
@@ -740,11 +740,9 @@ public class ExpressionCompiler extends
UnsupportedAllParseNodeVisitor<Expressio
@Override
public Expression create(ArithmeticParseNode node,
List<Expression> children) throws SQLException {
boolean foundDate = false;
- boolean isDeterministic = true;
PDataType theType = null;
for(int i = 0; i < children.size(); i++) {
Expression e = children.get(i);
- isDeterministic &= e.isDeterministic();
PDataType type = e.getDataType();
if (type == null) {
continue;
@@ -779,7 +777,7 @@ public class ExpressionCompiler extends
UnsupportedAllParseNodeVisitor<Expressio
} else if (theType == PDataType.DOUBLE) {
return new DoubleAddExpression(children);
} else if (theType == null) {
- return LiteralExpression.newConstant(null, theType,
isDeterministic);
+ return LiteralExpression.NULL_EXPRESSION;
} else if (theType == PDataType.TIMESTAMP || theType ==
PDataType.UNSIGNED_TIMESTAMP) {
return new TimestampAddExpression(children);
} else if (theType.isCoercibleTo(PDataType.DATE)) {
@@ -867,7 +865,6 @@ public class ExpressionCompiler extends
UnsupportedAllParseNodeVisitor<Expressio
PDataType theType = null;
Expression e1 = children.get(0);
Expression e2 = children.get(1);
- boolean isDeterministic = e1.isDeterministic() &&
e2.isDeterministic();
PDataType type1 = e1.getDataType();
PDataType type2 = e2.getDataType();
// TODO: simplify this special case for DATE conversion
@@ -919,7 +916,6 @@ public class ExpressionCompiler extends
UnsupportedAllParseNodeVisitor<Expressio
// This logic finds the common type to which all child
types are coercible
// without losing precision.
Expression e = children.get(i);
- isDeterministic &= e.isDeterministic();
PDataType type = e.getDataType();
if (type == null) {
continue;
@@ -952,7 +948,7 @@ public class ExpressionCompiler extends
UnsupportedAllParseNodeVisitor<Expressio
} else if (theType == PDataType.DOUBLE) {
return new DoubleSubtractExpression(children);
} else if (theType == null) {
- return LiteralExpression.newConstant(null, theType,
isDeterministic);
+ return LiteralExpression.NULL_EXPRESSION;
} else if (theType == PDataType.TIMESTAMP || theType ==
PDataType.UNSIGNED_TIMESTAMP) {
return new TimestampSubtractExpression(children);
} else if (theType.isCoercibleTo(PDataType.DATE)) {
@@ -975,10 +971,8 @@ public class ExpressionCompiler extends
UnsupportedAllParseNodeVisitor<Expressio
@Override
public Expression create(ArithmeticParseNode node,
List<Expression> children) throws SQLException {
PDataType theType = null;
- boolean isDeterministic = true;
for(int i = 0; i < children.size(); i++) {
Expression e = children.get(i);
- isDeterministic &= e.isDeterministic();
PDataType type = e.getDataType();
if (type == null) {
continue;
@@ -1004,7 +998,7 @@ public class ExpressionCompiler extends
UnsupportedAllParseNodeVisitor<Expressio
case DOUBLE:
return new DoubleMultiplyExpression( children);
default:
- return LiteralExpression.newConstant(null, theType,
isDeterministic);
+ return LiteralExpression.NULL_EXPRESSION;
}
}
});
{code}
- For the code that does boolean isDeterministic = lhsExpr.isDeterministic() ||
rhsExpr.isDeterministic();, I actually don't think that's correct. I think it
should have been an && and thus we should be using combine here as well. Does
that make sense to you too?
- I don't think we need an enum for Determinism.NEVER. I think the "least"
deterministic something is is PER_ROW. So values that we're set as false for
isDeterministic should turn into PER_ROW now. For example a COUNT(*) used
multiple times in the same SQL statement will return the same result. Same with
RAND() - try in Oracle SELECT RAND(),RAND() FROM SYSTEM.DUAL for example.
> Change Expression.isDeterministic() to return an ENUM with three values
> {DETERMINISTIC, UNDETERMINISTIC_ROW, UNDETERMINISTIC_STMT}
> ----------------------------------------------------------------------------------------------------------------------------------
>
> Key: PHOENIX-1030
> URL: https://issues.apache.org/jira/browse/PHOENIX-1030
> Project: Phoenix
> Issue Type: Improvement
> Reporter: Thomas D'Silva
> Assignee: Thomas D'Silva
> Attachments: PHOENIX-1030-3.0.patch, PHOENIX-1030-3.0.patch,
> PHOENIX-1030-4.0.patch, PHOENIX-1030-master.patch
>
>
> Change Expression.isDeterministic() to return an ENUM with three values
> DETERMINISTIC - the expression returns the same output every time given the
> same input.
> UNDETERMINISTIC_ROW - the expression should be computed for every row
> UNDETERMINISTIC_STMT - the expression should be be computed for a given
> statement only once
> See PHOENIX-1001
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
