Dmitry Lychagin has posted comments on this change. Change subject: [ASTERIXDB-2286][COMP][FUN][HYR] Parallel Sort Optimization ......................................................................
Patch Set 32: (7 comments) https://asterix-gerrit.ics.uci.edu/#/c/2393/31/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/common/APIFramework.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/common/APIFramework.java: Line 342: final PhysicalOptimizationConfig physOptConf = new PhysicalOptimizationConfig(); OptimizationConfUtil class is no longer used after this change, right? Let's remove it (in a separate change) https://asterix-gerrit.ics.uci.edu/#/c/2393/31/asterixdb/asterix-app/src/test/resources/optimizerts/results/btree-index-join/secondary-self-equi-join-index-only.plan File asterixdb/asterix-app/src/test/resources/optimizerts/results/btree-index-join/secondary-self-equi-join-index-only.plan: Line 26: -- ONE_TO_ONE_EXCHANGE |PARTITIONED| why ASSIGN and STREAM_PROJECT were eliminated? https://asterix-gerrit.ics.uci.edu/#/c/2393/31/asterixdb/asterix-app/src/test/resources/optimizerts/results/btree-index-join/sidx-non-idxonly-to-sidx-idxonly-equi-join_01.plan File asterixdb/asterix-app/src/test/resources/optimizerts/results/btree-index-join/sidx-non-idxonly-to-sidx-idxonly-equi-join_01.plan: Line 27: -- ASSIGN |PARTITIONED| why stream_project operators were eliminated? https://asterix-gerrit.ics.uci.edu/#/c/2393/31/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/config/CompilerProperties.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/config/CompilerProperties.java: Line 67: "The number of samples parallel sorting should " + "take from each partition"); minor. merge into a single string (separate change) https://asterix-gerrit.ics.uci.edu/#/c/2393/31/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/physical/SequentialMergeExchangePOperator.java File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/physical/SequentialMergeExchangePOperator.java: Line 51: return emptyUnaryRequirements(); I think this needs more thought (in a follow up change). getRequiredPropertiesForChildren() should require ORDERED_PARTITIONED physical property from its child because otherwise we cannot do sequential merge. Therefore this operator should be constructed with a list of OrderColumns that it'll require from its child. Line 57: AbstractLogicalOperator childOp = (AbstractLogicalOperator) op.getInputs().get(0).getValue(); This method also needs more thought (in a follow up change). 1) Need to analyze the propagation of the LOCAL_ORDER_PROPERTY from its child. Keep in mind that it may have more columns than the ORDERED_PARTITIONED partitioning property (see how SortMergeExchangePOperator handles this, for example). 2) Child's LOCAL_GROUPING_PROPERTY cannot be propagated. Let's discuss this. https://asterix-gerrit.ics.uci.edu/#/c/2393/31/hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/collectors/DeterministicPartitionBatchManager.java File hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/collectors/DeterministicPartitionBatchManager.java: Line 72: private synchronized boolean allPartitionsAdded() { minor. this method does not need to be synchronized because it's always called from synchronized methods -- 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: 32 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Dmitry Lychagin <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Taewoo Kim <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-HasComments: Yes
