You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by comnetwork <gi...@git.apache.org> on 2019/01/03 08:51:51 UTC
[GitHub] phoenix pull request #424: PHOENIX-4820
GitHub user comnetwork opened a pull request:
https://github.com/apache/phoenix/pull/424
PHOENIX-4820
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/comnetwork/phoenix 4.x-HBase-1.3
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/phoenix/pull/424.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 #424
----
commit edf6a5cd2e150e44b60731206e065c744dbb0956
Author: chenglei <ch...@...>
Date: 2019-01-03T08:48:58Z
PHOENIX-4820
----
---
[GitHub] phoenix pull request #424: PHOENIX-4820
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/424#discussion_r245188153
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/expression/BaseExpression.java ---
@@ -255,4 +255,15 @@ public boolean requiresFinalEvaluation() {
return false;
}
+ @Override
+ public boolean isCloneExpression() {
+ return isCloneExpressionByDeterminism(this);
+ }
+
+ protected static boolean isCloneExpressionByDeterminism(BaseExpression expression) {
--- End diff --
+1 LGTM
---
[GitHub] phoenix pull request #424: PHOENIX-4820
Posted by comnetwork <gi...@git.apache.org>.
Github user comnetwork commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/424#discussion_r245186554
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/expression/BaseExpression.java ---
@@ -255,4 +255,15 @@ public boolean requiresFinalEvaluation() {
return false;
}
+ @Override
+ public boolean isCloneExpression() {
+ return isCloneExpressionByDeterminism(this);
+ }
+
+ protected static boolean isCloneExpressionByDeterminism(BaseExpression expression) {
--- End diff --
@twdsilva , thank you for the review, I added BaseExpression.isCloneExpressionByDeterminism method here for the purpose that it can also be invoked by RandomFunction. RandomFunction is derived from BaseCompoundExpression , and if RandomFunction does not override isCloneExpression method, the isCloneExpression method of RandomFunction would use the isCloneExpression method of BaseCompoundExpression and return false, the existing Unit Test ArithmeticOperationTest would fail.
---
[GitHub] phoenix issue #424: PHOENIX-4820
Posted by comnetwork <gi...@git.apache.org>.
Github user comnetwork commented on the issue:
https://github.com/apache/phoenix/pull/424
This PR is merged @twdsilva reviews of PR 417 and made a few modifications for BaseExpression ,BaseCompoundExpression and BaseSingleExpression in order to pass the Unit tests and IT tests.
---
[GitHub] phoenix pull request #424: PHOENIX-4820
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/424#discussion_r245187754
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/expression/BaseExpression.java ---
@@ -255,4 +255,15 @@ public boolean requiresFinalEvaluation() {
return false;
}
+ @Override
+ public boolean isCloneExpression() {
+ return isCloneExpressionByDeterminism(this);
+ }
+
+ protected static boolean isCloneExpressionByDeterminism(BaseExpression expression) {
--- End diff --
If isCloneExpression() is implemented as follows, do you still need isCloneExpressionByDeterminism() ?
```
@Override
public boolean isCloneExpression() {
if(getDeterminism() == Determinism.PER_INVOCATION) {
return true;
}
return false;
}
```
---
[GitHub] phoenix pull request #424: PHOENIX-4820
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/424#discussion_r245145423
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/expression/BaseExpression.java ---
@@ -255,4 +255,15 @@ public boolean requiresFinalEvaluation() {
return false;
}
+ @Override
+ public boolean isCloneExpression() {
+ return isCloneExpressionByDeterminism(this);
+ }
+
+ protected static boolean isCloneExpressionByDeterminism(BaseExpression expression) {
--- End diff --
nit: I don't think this method is need, you can just inline this in ```public boolean isCloneExpression() ```
---