ChinmaySKulkarni commented on a change in pull request #616: PHOENIX-5432:
Refactor LiteralExpression to use the builder pattern
URL: https://github.com/apache/phoenix/pull/616#discussion_r347669919
##########
File path:
phoenix-core/src/main/java/org/apache/phoenix/expression/ComparisonExpression.java
##########
@@ -158,19 +163,19 @@ public static Expression create(CompareOp op,
List<Expression> children, Immutab
if (lhsExpr instanceof LiteralExpression) {
lhsValue = ((LiteralExpression)lhsExpr).getValue();
if (lhsValue == null) {
- return LiteralExpression.newConstant(null, PBoolean.INSTANCE,
lhsExpr.getDeterminism());
+ return new
LiteralExpression.Builder().setDataType(PBoolean.INSTANCE).setDeterminism(lhsExpr.getDeterminism()).build();
}
}
Object rhsValue = null;
// TODO: use lhsExpr.isStateless instead
if (rhsExpr instanceof LiteralExpression) {
rhsValue = ((LiteralExpression)rhsExpr).getValue();
if (rhsValue == null) {
- return LiteralExpression.newConstant(null, PBoolean.INSTANCE,
rhsExpr.getDeterminism());
+ return new
LiteralExpression.Builder().setDataType(PBoolean.INSTANCE).setDeterminism(rhsExpr.getDeterminism()).build();
}
}
if (lhsValue != null && rhsValue != null) {
- return
LiteralExpression.newConstant(ByteUtil.compare(op,lhsExprDataType.compareTo(lhsValue,
rhsValue, rhsExprDataType)), determinism);
+ return new
LiteralExpression.Builder().setValue(ByteUtil.compare(op,lhsExprDataType.compareTo(lhsValue,
rhsValue, rhsExprDataType))).setDeterminism(determinism).build();
Review comment:
@christinefeng This is a general comment overall; it is much easier to read
builder pattern code when each setter is on a separate line, for ex:
[this](https://github.com/apache/phoenix/blob/234660ea0082815c59704a61dc9c33bb11d64381/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java#L1170-L1213).
I'm not sure how feasible it is to change this everywhere given your current
patch is pretty huge, but it would definitely improve the readability of the
code.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
