asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Luo Chen (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: Add Create Secondary BTree for Correlated Datasets
Date Fri, 16 Jun 2017 16:33:29 GMT
Luo Chen has posted comments on this change.

Change subject: Add Create Secondary BTree for Correlated Datasets
......................................................................


Patch Set 1:

(12 comments)

> (16 comments)
 > 
 > It looks that new bulkload code hasn't actually been run?
 > 
 > Check the following method in IndexUtil:
 > 
 > public static JobSpecification buildSecondaryIndexLoadingJobSpec(Dataset
 > dataset, Index index,
 > MetadataProvider metadataProvider) throws AlgebricksException {
 > SecondaryIndexOperationsHelper secondaryIndexHelper =
 > SecondaryIndexOperationsHelper.createIndexOperationsHelper(dataset,
 > index, metadataProvider,
 > physicalOptimizationConfig);
 > return secondaryIndexHelper.buildLoadingJobSpec();
 > }
 > 
 > That shows how harmful the static method is?  Maybe it's time to
 > refactor the SecondaryIndexHelper using the factory pattern?

https://asterix-gerrit.ics.uci.edu/#/c/1813/10/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/QueryTranslator.java
File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/QueryTranslator.java:

PS10, Line 959: x = MetadataManager.INSTANCE.beginTransaction();
> Inside this method, IndexUtil still uses the old SecondaryIndexOperationsHe
Done. Previously I changed the IndexUtil to use SecondaryCorrelatedIndexOperationsHelper whenever
necessary, but the change was somehow reverted...


https://asterix-gerrit.ics.uci.edu/#/c/1813/10/asterixdb/asterix-app/src/test/resources/runtimets/queries/dml/using-correlated-prefix-merge-policy-with-feed/using-correlated-prefix-merge-policy-with-feed.1.ddl.aql
File asterixdb/asterix-app/src/test/resources/runtimets/queries/dml/using-correlated-prefix-merge-policy-with-feed/using-correlated-prefix-merge-policy-with-feed.1.ddl.aql:

PS10, Line 21:  the correlated prefix merge policy guarantees that disk components of
             :  * the primary index and the secondary indexes of a dataset are always aligned.
> It only tests the query correctness of correlated indexes?  Disk layout is 
These integration tests only test the query correctness not the disk layout for now. I couldn't
think of a way to test disk layout only using these queries... Maybe using unit tests for
each HyrackOperator?


https://asterix-gerrit.ics.uci.edu/#/c/1813/10/asterixdb/asterix-app/src/test/resources/runtimets/queries/temp-dataset/insert-and-scan-dataset-with-correlated-index/insert-and-scan-dataset-with-correlated-index.1.ddl.aql
File asterixdb/asterix-app/src/test/resources/runtimets/queries/temp-dataset/insert-and-scan-dataset-with-correlated-index/insert-and-scan-dataset-with-correlated-index.1.ddl.aql:

PS10, Line 20:  
> WS
Done


https://asterix-gerrit.ics.uci.edu/#/c/1813/10/asterixdb/asterix-app/src/test/resources/runtimets/queries/temp-dataset/insert-and-scan-dataset-with-correlated-index/insert-and-scan-dataset-with-correlated-index.2.update.aql
File asterixdb/asterix-app/src/test/resources/runtimets/queries/temp-dataset/insert-and-scan-dataset-with-correlated-index/insert-and-scan-dataset-with-correlated-index.2.update.aql:

PS10, Line 22: deadlatch
> deadlock or dead latch?
I copied the description from insert-and-scan-dataset-with-index.2.update, where the original
description is "deadlatch".


https://asterix-gerrit.ics.uci.edu/#/c/1813/10/asterixdb/asterix-app/src/test/resources/runtimets/queries/temp-dataset/insert-and-scan-dataset-with-correlated-index/insert-and-scan-dataset-with-correlated-index.3.ddl.aql
File asterixdb/asterix-app/src/test/resources/runtimets/queries/temp-dataset/insert-and-scan-dataset-with-correlated-index/insert-and-scan-dataset-with-correlated-index.3.ddl.aql:

PS10, Line 20:  
> Set IDE to trim WS?
Done


https://asterix-gerrit.ics.uci.edu/#/c/1813/10/asterixdb/asterix-app/src/test/resources/runtimets/queries/temp-dataset/insert-and-scan-dataset-with-correlated-index/insert-and-scan-dataset-with-correlated-index.4.query.aql
File asterixdb/asterix-app/src/test/resources/runtimets/queries/temp-dataset/insert-and-scan-dataset-with-correlated-index/insert-and-scan-dataset-with-correlated-index.4.query.aql:

PS10, Line 21: e we insert a materializing to prevent the possib
> materialization is no longer need.
Do we still need create secondary index on temp dataset? If no, I'll remove this test...


https://asterix-gerrit.ics.uci.edu/#/c/1813/10/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/utils/SecondaryCorrelatedTreeIndexOperationsHelper.java
File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/utils/SecondaryCorrelatedTreeIndexOperationsHelper.java:

PS10, Line 89: -
> why negative here?
This ensures component_pos are in a descending order. In the disk component list, components
are ordered from newest to oldest. This order here ensures older component can be bulk loaded
first so that the component file name has a small timestamp (where we relies on the component
file timestamp to restore the component list).

I added this explanation to avoid confusion in the future.


PS10, Line 290: createIndexOperationsHelper
> It looks no one calls this method? 
Done.
I fixed this at IndexUtil.


https://asterix-gerrit.ics.uci.edu/#/c/1813/10/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/operators/AbstractLSMSecondaryCreationNodePushable.java
File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/operators/AbstractLSMSecondaryCreationNodePushable.java:

PS10, Line 31: AbstractLSMSecondaryCreationNodePushable
> AbstractLSMSecondaryCreationNodePushable -> AbstractLSMSecondaryIndexCreati
Done


https://asterix-gerrit.ics.uci.edu/#/c/1813/10/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/operators/LSMSecondaryCreationBulkLoadNodePushable.java
File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/operators/LSMSecondaryCreationBulkLoadNodePushable.java:

PS10, Line 46: Creation
> Creation -> Index
Done


https://asterix-gerrit.ics.uci.edu/#/c/1813/10/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/dataflow/LSMBTreeDiskComponentScanOperatorNodePushable.java
File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/dataflow/LSMBTreeDiskComponentScanOperatorNodePushable.java:

PS10, Line 54: return null;
> Throw UnSupportedException, if no one should call into this method for this
I think this method is actually called by parent.open().


PS10, Line 60: 
> Throw UnSupportedException, if no one should call into this method for this
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2a3435e6720f07bd6a5092d4d9ce42e8d4b7894c
Gerrit-PatchSet: 1
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Luo Chen <cluo8@uci.edu>
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: Luo Chen <cluo8@uci.edu>
Gerrit-Reviewer: Till Westmann <tillw@apache.org>
Gerrit-Reviewer: Yingyi Bu <buyingyi@gmail.com>
Gerrit-Reviewer: abdullah alamoudi <bamousaa@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message