Ali Alsuliman has posted comments on this change. Change subject: [ASTERIXDB-2286][COMP][FUN][HYR] Parallel Sort Optimization ......................................................................
Patch Set 11: (17 comments) https://asterix-gerrit.ics.uci.edu/#/c/2393/11/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/subplan/InlineLeftNtsInSubplanJoinFlatteningVisitor.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/subplan/InlineLeftNtsInSubplanJoinFlatteningVisitor.java: Line 385: public ILogicalOperator visitForwardOperator(ForwardOperator op, Void arg) throws AlgebricksException { > union/intersect operators are not supported by this visitor. Done. Check if it's correct. https://asterix-gerrit.ics.uci.edu/#/c/2393/11/asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/clause/OrderbyClause.java File asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/clause/OrderbyClause.java: Line 103: return isRPSSort; > need to copy this new property in CloneAndSubstituteVariablesVisitor.visit( Done Line 106: public void setRPSSort(boolean isRPSSort) { > need to give a better name to this property "sort" is redundant. "FullParal Done Line 112: return Objects.hash(modifierList, numFrames, numTuples, orderbyList); > rangeMap and isRPSSort should be used in hashCode() Done Line 116: public boolean equals(Object object) { > rangeMap and isRPSSort should be used in equals(). we don't really need an Done https://asterix-gerrit.ics.uci.edu/#/c/2393/11/asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj File asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj: Line 206: private static final String NO_PARALLEL_SORT_HINT = "no-rps-sort"; > can we give this hint a better name? it's on the orderby clause, so "-sort" Done. I changed it to "unpartitioned". BTW, I didn't add this hint to AQL since I assumed we are deprecating it. Should I add it to AQL as well? or? Line 3048: oc.setRangeMap(RangeMapBuilder.parseHint(hint.substring(RANGE_HINT.length()), parserFactory)); > why do we need to create and pass SqllppParserFactory when we're already in Done Line 3050: throw new ParseException(e.getMessage()); > need to add source location: throw SqlppParseException(getSourceLocation(ge Done https://asterix-gerrit.ics.uci.edu/#/c/2393/11/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/expressions/AbstractLogicalExpression.java File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/expressions/AbstractLogicalExpression.java: Line 41: public void setParameters(Object[] expressionParameters) { > why can't we use AbstractFunctionCallExpression.setOpaqueParameters() for t I don't think that's the right way to go about this. The behaviour of TypeInferer is gradually diverging from its semantics. In my case, it is quite confusing to use TypeInferer to pass information like "number of partitions" (I know it does the job). We should fix this some time soon as it is becoming a growing need to pass information from expressions to function descriptors. For now, I changed the code to use TypeInferer. https://asterix-gerrit.ics.uci.edu/#/c/2393/11/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/visitors/IsomorphismOperatorVisitor.java File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/visitors/IsomorphismOperatorVisitor.java: Line 604: return true; > need to implement this. look at other operators in this class. Done https://asterix-gerrit.ics.uci.edu/#/c/2393/11/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/properties/IPartitioningProperty.java File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/properties/IPartitioningProperty.java: Line 72: /** > it seems that it should return a new object if there need to be any changes I remembered why I had this note. In "OrderedPartitionedProperty", even though it returns a new object, it also modified the calling object. I think that was inadvertent, and I faced some issues with it while testing. So, I changed it so that it does not change the current (calling) object. Check PropertiesUtil.applyFDsToOrderColumns. That's where I changed it. "OrderedPartitionedProperty" is using this method. I had this note just in case someone had some thoughts about my fix. Note removed. https://asterix-gerrit.ics.uci.edu/#/c/2393/11/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 320: throw new IllegalStateException("Couldn't remove the redundant parallel sort and its subtree"); > are we sure it can never happen? what if there're other rewriting rules tha Done. Now it's different with the new code changes. Line 689: if (parentOp.getAnnotations().get(OperatorAnnotations.USE_RPS_SORT) == Boolean.TRUE > looking for a specific operator pattern is not great. you just need to pass Done. Now it's different with the new code changes. https://asterix-gerrit.ics.uci.edu/#/c/2393/11/hyracks-fullstack/hyracks/hyracks-dataflow-common/src/main/java/org/apache/hyracks/dataflow/common/data/partition/range/FieldRangePartitionComputerFactory.java File hyracks-fullstack/hyracks/hyracks-dataflow-common/src/main/java/org/apache/hyracks/dataflow/common/data/partition/range/FieldRangePartitionComputerFactory.java: Line 31: public class FieldRangePartitionComputerFactory implements ITuplePartitionComputerFactory { > may be we should split it into 3 classes: 1) AbstractFieldRangePartitionCom Done Line 72: throw HyracksDataException.create(ErrorCode.RANGEMAP_NOT_FOUND); > pass source location Could you help and point out what I should do? The tuple partition computers and their factories don't have SourceLocation. Speaking of this, could you also check EnforceStructuralPropertiesRule for SourceLocation as I'm not much clear about SourceLocation change? https://asterix-gerrit.ics.uci.edu/#/c/2393/11/hyracks-fullstack/hyracks/hyracks-dataflow-common/src/main/java/org/apache/hyracks/dataflow/common/data/partition/range/RangeMap.java File hyracks-fullstack/hyracks/hyracks-dataflow-common/src/main/java/org/apache/hyracks/dataflow/common/data/partition/range/RangeMap.java: Line 29: * split_point_idx0 split_point_idx1 split_point_idx2 split_point_idx3 split_point_idx4 > need better formatting. create html table or use <pre>? Done https://asterix-gerrit.ics.uci.edu/#/c/2393/11/hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/base/AbstractReplicateOperatorDescriptor.java File hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/base/AbstractReplicateOperatorDescriptor.java: Line 183: // TODO: shouldn't we fail the MaterializerTaskState state? > there're no IFrameWriter associated with MaterializerTaskState, so I guess But there is the RunFileWriter in the MaterializerTaskState. Isn't that an IFrameWriter? -- To view, visit https://asterix-gerrit.ics.uci.edu/2393 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I73e128029a46f45e6b68c23dfb9310d5de10582f Gerrit-PatchSet: 11 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Ali Alsuliman <ali.al.solai...@gmail.com> Gerrit-Reviewer: Ali Alsuliman <ali.al.solai...@gmail.com> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Dmitry Lychagin <dmitry.lycha...@couchbase.com> Gerrit-Reviewer: Ildar Absalyamov <ildar.absalya...@gmail.com> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Taewoo Kim <wangs...@gmail.com> Gerrit-Reviewer: Till Westmann <ti...@apache.org> Gerrit-Reviewer: abdullah alamoudi <bamou...@gmail.com> Gerrit-HasComments: Yes
