[jira] [Commented] (PHOENIX-5432) Refactor LiteralExpression to use the builder pattern

["Hadoop QA (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/PHOENIX-5432?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16969837#comment-16969837 ] 

Hadoop QA commented on PHOENIX-5432:
------------------------------------

{color:red}-1 overall{color}.  Here are the results of testing the latest attachment 
  http://issues.apache.org/jira/secure/attachment/12985304/PHOENIX-5432.master.v2.patch
  against master branch at commit 70baf4c6805eb46420dd1fbaf71a093ab7d84a8b.
  ATTACHMENT ID: 12985304

    {color:green}+1 @author{color}.  The patch does not contain any @author tags.

    {color:green}+1 tests included{color}.  The patch appears to include 133 new or modified tests.

    {color:red}-1 javac{color}.  The applied patch generated 382 javac compiler warnings (more than the master's current 381 warnings).

    {color:green}+1 release audit{color}.  The applied patch does not increase the total number of release audit warnings.

    {color:red}-1 lineLengths{color}.  The patch introduces the following lines longer than 100:
    +	  dv == null ? null : new LiteralExpression.Builder().setValue(((LiteralParseNode)dv).getValue()).buildValueAndDeterminism(false),
+      minv == null ? null : new LiteralExpression.Builder().setValue(((LiteralParseNode)minv).getValue()).buildValueAndDeterminism(false),
+      maxv == null ? null : new LiteralExpression.Builder().setValue(((LiteralParseNode)maxv).getValue()).buildValueAndDeterminism(false));})
+    private static final Expression NOT_NULL_STRING = new LiteralExpression.Builder().setValue(PVarchar.INSTANCE.toObject(KeyRange.IS_NOT_NULL_RANGE.getLowerRange())).buildValueAndDeterminism(false);
+            return new LiteralExpression.Builder().setValue(false).setDeterminism(determinism).buildValueAndDeterminism(false);
+        return new LiteralExpression.Builder().setValue(value).setDeterminism(Determinism.ALWAYS).build();
+        return new LiteralExpression.Builder().setValue(node.getValue()).setDataType(node.getType()).setDeterminism(Determinism.ALWAYS).build();
+                return new LiteralExpression.Builder().setDataType(PBoolean.INSTANCE).setDeterminism(rhs.getDeterminism()).build();
+                return new LiteralExpression.Builder().setValue(false).setDeterminism(rhs.getDeterminism()).buildValueAndDeterminism(false);
+                      rhs = new LiteralExpression.Builder().setValue(rhsLiteral).setDataType(PChar.INSTANCE)

     {color:red}-1 core tests{color}.  The patch failed these unit tests:
     ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.IndexMaintenanceIT
./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.LocalImmutableTxIndexIT
./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.txn.RollbackIT
./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.DropColumnIT
./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.GlobalImmutableNonTxIndexIT
./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.RowTimestampIT
./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.rpc.PhoenixClientRpcIT
./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.LocalImmutableNonTxIndexIT
./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.IndexUsageIT
./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.tx.TxCheckpointIT
./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.DeleteIT
./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.IndexToolIT
./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.GlobalImmutableTxIndexIT

Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/3101//testReport/
Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/3101//console

This message is automatically generated.

> Refactor LiteralExpression to use the builder pattern
> -----------------------------------------------------
>
>                 Key: PHOENIX-5432
>                 URL: https://issues.apache.org/jira/browse/PHOENIX-5432
>             Project: Phoenix
>          Issue Type: Improvement
>    Affects Versions: 4.15.0, 5.1.0
>            Reporter: Chinmay Kulkarni
>            Assignee: Christine Feng
>            Priority: Major
>         Attachments: PHOENIX-5432-master-v1.patch, PHOENIX-5432.master.v2.patch, PHOENIX-5432.patch
>
>          Time Spent: 1h 10m
>  Remaining Estimate: 0h
>
> LiteralExpression is a mess. While it provides newConstant() APIs to build the object, it also provides two public constructors. There are 10 overloaded newConstant() methods and it is unclear which API to use in which case.
> This should be refactored to use the builder pattern and final member variables. Ideally, getters such as getMaxLength() should be simple member variable accessors and other ad-hoc logic surrounding those variables should be handled correctly when setting their respective values.
>  
>  



--
This message was sent by Atlassian Jira
(v8.3.4#803005)