Michael Blow has posted comments on this change. Change subject: Fulltext search initial implementation ......................................................................
Patch Set 5: (19 comments) https://asterix-gerrit.ics.uci.edu/#/c/989/5/asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/expression/TypeExpression.java File asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/expression/TypeExpression.java: Line 23: public interface TypeExpression extends ILangExpression { Should this be renamed to ITypeExpression? https://asterix-gerrit.ics.uci.edu/#/c/989/5/asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/rewrites/ExpressionSubstitutionEnvironment.java File asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/rewrites/ExpressionSubstitutionEnvironment.java: Line 54: private Map<Expression, Expression> exprMap = new HashMap<>(); final? Line 55: private Map<VariableExpr, Expression> freeVarToExprMap = new HashMap<>(); final? Line 63: private Deque<Multiset<Expression>> disabledExprBackup = new ArrayDeque<>(); final? https://asterix-gerrit.ics.uci.edu/#/c/989/5/asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/rewrites/LangRewritingContext.java File asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/rewrites/LangRewritingContext.java: Line 28: private int systemVarCounter = 1; Can we leave this the same as varCounter (i.e. 0) for consistency, and change the increment below to increment before use? (i.e. ++systemVarCounter) Line 29: private HashMap<Integer, VarIdentifier> oldVarIdToNewVarId = new HashMap<>(); final? Line 67: return new VarIdentifier("#" + (systemVarCounter++), id); See comment above, change to ++systemVarCounter https://asterix-gerrit.ics.uci.edu/#/c/989/5/asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/statement/Query.java File asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/statement/Query.java: Line 34: private List<String> dataverses = new ArrayList<>(); final? Line 35: private List<String> datasets = new ArrayList<>(); final? https://asterix-gerrit.ics.uci.edu/#/c/989/5/asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/clause/FromClause.java File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/clause/FromClause.java: Line 33: private List<FromTerm> fromTerms = new ArrayList<>(); final? https://asterix-gerrit.ics.uci.edu/#/c/989/5/asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/clause/FromTerm.java File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/clause/FromTerm.java: Line 37: private List<AbstractBinaryCorrelateClause> correlateClauses = new ArrayList<>(); final? https://asterix-gerrit.ics.uci.edu/#/c/989/5/asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/clause/SelectBlock.java File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/clause/SelectBlock.java: Line 155: sb.append(fromClause); Shouldn't we append some separator between clauses? i.e. whitespace or something https://asterix-gerrit.ics.uci.edu/#/c/989/5/asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/clause/SelectSetOperation.java File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/clause/SelectSetOperation.java: Line 36: private List<SetOperationRight> rightInputs = new ArrayList<>(); final? https://asterix-gerrit.ics.uci.edu/#/c/989/5/asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/expression/SelectExpression.java File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/expression/SelectExpression.java: Line 36: private List<LetClause> letList = new ArrayList<>(); final? Line 125: sb.append(letList.toString()); should there be some separator between the clauses, i.e. whitespace https://asterix-gerrit.ics.uci.edu/#/c/989/5/asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/struct/SetOperationInput.java File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/struct/SetOperationInput.java: Line 81: return selectBlock.toString(); should subquery be included? https://asterix-gerrit.ics.uci.edu/#/c/989/5/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/physical/RangePartitionExchangePOperator.java File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/physical/RangePartitionExchangePOperator.java: Line 61: public RangePartitionExchangePOperator(List<OrderColumn> partitioningFields, INodeDomain domain, IRangeMap rangeMap) { line too long https://asterix-gerrit.ics.uci.edu/#/c/989/5/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/physical/RangePartitionMergeExchangePOperator.java File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/physical/RangePartitionMergeExchangePOperator.java: Line 66: public RangePartitionMergeExchangePOperator(List<OrderColumn> partitioningFields, INodeDomain domain, IRangeMap rangeMap) { line too long https://asterix-gerrit.ics.uci.edu/#/c/989/5/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/EnforceStructuralPropertiesRule.java File hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/EnforceStructuralPropertiesRule.java: Line 576: pop = new RangePartitionExchangePOperator(((OrderedPartitionedProperty) pp).getOrderColumns(), domain, line too long -- To view, visit https://asterix-gerrit.ics.uci.edu/989 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I71887c2ea847e4488f4c98a11f8a5bcad02cac5a Gerrit-PatchSet: 5 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Taewoo Kim <[email protected]> Gerrit-Reviewer: Heri Ramampiaro <[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-HasComments: Yes
