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 <[email protected]> Gerrit-Reviewer: Heri Ramampiaro <[email protected]> Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Jianfeng Jia <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-Reviewer: Taewoo Kim <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: Yingyi Bu <[email protected]> Gerrit-HasComments: Yes
