asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ian Maxon (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: Full-text implementation step 1
Date Wed, 19 Oct 2016 23:47:20 GMT
Ian Maxon has posted comments on this change.

Change subject: Full-text implementation step 1
......................................................................


Patch Set 11:

(12 comments)

More thoughts/questions...

https://asterix-gerrit.ics.uci.edu/#/c/1228/11/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/FullTextContainsParameterCheckRule.java
File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/FullTextContainsParameterCheckRule.java:

Line 131:                 && ConstantExpressionUtil.getConstantIaObjectType(firstExpr)
!= ATypeTag.STRING) {
"The first expression should be a string"


Line 144:                 default:
"The second expression should be a string, unordered list, or ordered list."


Line 167:         if (openRecConsExpr.getArguments().size() > FullTextContainsDescriptor.paramTypeMap.size()
* 2) {
Why is it * 2? Because opRecConsExpr.getArguments() is like [expr,val,expr,val...] ?


Line 176:                 throw new AlgebricksException(
"Options must be in the form of constant strings. Check that the option at " + (i%2+1) + "
is indeed a constant string"


Line 181:             if (!FullTextContainsDescriptor.paramTypeMap.containsKey(option)) {
The English here is not right, it might be something like "The given option " + option + "
is not a valid argument to ftcontains()"


Line 206:                 throw new AlgebricksException(
"The given value for option " + option + " was not of the expected type"


Line 225:     boolean checkSearchModeOption(String optionVal) throws AlgebricksException {
Why's this a boolean when the value is never used?


Line 229:         } else {
"The given value for the search mode ("+optionVal+") is not valid. Valid modes are " + FullTextContainsDescriptor.CONJUNCTIVE_SEARCH_MODE_OPTION
+ " or " + FullTextContainsDescriptor.DISJUNCTIVE_SEARCH_MODE_OPTION +"."


https://asterix-gerrit.ics.uci.edu/#/c/1228/11/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/common/FullTextContainsEvaluator.java
File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/common/FullTextContainsEvaluator.java:

Line 177:      */
This method is really, really long, and quite nested. If it could be broken up more, say betwen
the init, right, and left parts, that might help legibility.


Line 313:             // array that contains the key (this array will be set later.)
What is the meaning behind 101 and 32768? Can those be static final variables if they are
supposed to be?


Line 394:     protected boolean checkArgTypes(ATypeTag typeTag1, ATypeTag typeTag2) throws
AlgebricksException {
The if/else here is superfluous.


Line 398: 
Likewise here.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If00a871a8241d6aa6931f97b694d65f164d3ab8c
Gerrit-PatchSet: 11
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Taewoo Kim <wangsaeu@yahoo.com>
Gerrit-Reviewer: Heri Ramampiaro <heriram@gmail.com>
Gerrit-Reviewer: Ian Maxon <imaxon@apache.org>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Jianfeng Jia <jianfeng.jia@gmail.com>
Gerrit-Reviewer: Michael Blow <mblow@apache.org>
Gerrit-Reviewer: Taewoo Kim <wangsaeu@yahoo.com>
Gerrit-Reviewer: Till Westmann <tillw@apache.org>
Gerrit-Reviewer: Yingyi Bu <buyingyi@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message