asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Taewoo Kim (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: AQLPlus Refactoring 1: add a new rule controller
Date Wed, 18 Jan 2017 05:18:39 GMT
Taewoo Kim has posted comments on this change.

Change subject: AQLPlus Refactoring 1: add a new rule controller
......................................................................


Patch Set 5:

(8 comments)

https://asterix-gerrit.ics.uci.edu/#/c/1432/5/hyracks-fullstack/algebricks/algebricks-common/src/main/java/org/apache/hyracks/algebricks/common/exceptions/AlgebricksException.java
File hyracks-fullstack/algebricks/algebricks-common/src/main/java/org/apache/hyracks/algebricks/common/exceptions/AlgebricksException.java:

Line 45:     public static AlgebricksException create(int code, Serializable... params) {
> code -> errorCode
Done


https://asterix-gerrit.ics.uci.edu/#/c/1432/5/hyracks-fullstack/algebricks/algebricks-compiler/src/main/java/org/apache/hyracks/algebricks/compiler/rewriter/rulecontrollers/SequentialFirstRuleCheckFixpointRuleController.java
File hyracks-fullstack/algebricks/algebricks-compiler/src/main/java/org/apache/hyracks/algebricks/compiler/rewriter/rulecontrollers/SequentialFirstRuleCheckFixpointRuleController.java:

Line 56:         boolean anyRuleFired;
> move the declaration to line 69, combing that with the rest of the line.
Done


Line 57:         boolean anyChange;
> move the declaration to line 79.
Done


Line 68:         // Checks whether the first rule returns true.
> Make sure that empty input collection can still work gracefully?
Done


Line 70:         if (!anyRuleFired) {
> Adding a "if" check after line 83 should be able to get rid of line 68 to l
Done


Line 75:         for (int i = 1; i < rules.size(); i++) {
> Why do you need that for-loop iteration?
I have added this not to execute the first rule twice in the first iteration. But, I think
addressing your above comment will solve this, too.


https://asterix-gerrit.ics.uci.edu/#/c/1432/5/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/exceptions/ErrorCode.java
File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/exceptions/ErrorCode.java:

Line 29:  */
> describe the error code boundary here.
Done


Line 38:     public static final int RUNTIME_FULLTEXT_PHRASE_FOUND = 5;
> There is no corresponding message in the properties file.
Done


-- 
To view, visit https://asterix-gerrit.ics.uci.edu/1432
To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2742e891339e5aba37a00f77d7f18cb3c09bcfe2
Gerrit-PatchSet: 5
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Taewoo Kim <wangsaeu@yahoo.com>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Taewoo Kim <wangsaeu@yahoo.com>
Gerrit-Reviewer: Yingyi Bu <buyingyi@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message