>From Dmitry Lychagin <[email protected]>: Dmitry Lychagin has posted comments on this change. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/7684 )
Change subject: [NO-ISSUE][IDX] Adding support for array-indexes. ...................................................................... Patch Set 36: (11 comments) 1. Can we add testcases that return more than 1 tuple? 2. We need to discuss the use of "?" quantifier in type definitions. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/7684/36/asterixdb/asterix-app/src/test/resources/runtimets/results/array-index/metadata/closed/use-case-1/use-case-1.1.adm File asterixdb/asterix-app/src/test/resources/runtimets/results/array-index/metadata/closed/use-case-1/use-case-1.1.adm: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/7684/36/asterixdb/asterix-app/src/test/resources/runtimets/results/array-index/metadata/closed/use-case-1/use-case-1.1.adm@1 PS36, Line 1: { "SearchKey": [ ], "SearchKeyElements": [ { "UnnestList": [ [ "dates" ] ], "ProjectList": [ null ] } ] } Remind me. Did we intend to have a ProjectList:[null] in this case. Seems like we could eliminate it altogether from the metadata if there's not project list specified in the index definition. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/7684/36/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/MetadataNode.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/MetadataNode.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/7684/36/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/MetadataNode.java@1241 PS36, Line 1241: if (index.getIndexDetails() instanceof Index.TextIndexDetails) { We should use IndexCategory.of()==TEXT instead of "instance of" https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/7684/36/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/ArrayBTreeResourceFactoryProvider.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/ArrayBTreeResourceFactoryProvider.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/7684/36/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/ArrayBTreeResourceFactoryProvider.java@88 PS36, Line 88: return index.getIndexName().equals(IndexingConstants.getFilesIndexName(dataset.getDatasetName())) We don't support array indexes for external datasets. Seems that we should just fail with COMPILATION_ILLEGAL_STATE here https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/7684/36/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/ArrayBTreeResourceFactoryProvider.java@105 PS36, Line 105: //Compress only primary index Array indexes cannot be primary. Seems that we should just fail with COMPILATION_ILLEGAL_STATE here https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/7684/36/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/ArrayBTreeResourceFactoryProvider.java@126 PS36, Line 126: if (index.isPrimaryIndex()) { array indexes are not supported as primary indexes and also for external datasets. We should fail here in those cases. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/7684/36/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/ArrayBTreeResourceFactoryProvider.java@165 PS36, Line 165: if (index.isPrimaryIndex()) { array indexes are not supported as primary indexes and also for external datasets. We should fail here in those cases. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/7684/36/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/ArrayBTreeResourceFactoryProvider.java@204 PS36, Line 204: if (index.isPrimaryIndex() || index.isPrimaryKeyIndex()) { array indexes are not supported as primary indexes and also for external datasets. We should fail here in those cases. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/7684/36/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/ArrayBTreeResourceFactoryProvider.java@217 PS36, Line 217: return null; Looks like this whole method should just return 'null' for array indexes. May be we should just eliminate this method then. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/7684/36/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/utils/KeyFieldTypeUtil.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/utils/KeyFieldTypeUtil.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/7684/36/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/utils/KeyFieldTypeUtil.java@153 PS36, Line 153: ARecordType sourceType = (e.getSourceIndicator() == 0) ? recordType : metaRecordType; Use Index.RECORD_INDICATOR instead of 0 https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/7684/36/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/utils/KeyFieldTypeUtil.java@311 PS36, Line 311: boolean itemTypeNullable = false, itemTypeMissalbe = false; minor. typo: Missalbe. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/7684/36/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: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/7684/36/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/visitors/IsomorphismOperatorVisitor.java@610 PS36, Line 610: return (!compareSubplans(op.getNestedPlans(), insertOpArg.getNestedPlans())) ? Boolean.TRUE : Boolean.FALSE; This comparison was and still incomplete. We need to ensure that all expressions, variables, etc held by IndexInsertDeleteUpsertOperator are the same in order to declare these two operators isomorphic. -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/7684 To unsubscribe, or for help writing mail filters, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Change-Id: Id0e9eee940cc94819e169a74ed180387b7a3093b Gerrit-Change-Number: 7684 Gerrit-PatchSet: 36 Gerrit-Owner: Glenn Galvizo <[email protected]> Gerrit-Reviewer: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Dmitry Lychagin <[email protected]> Gerrit-Reviewer: Glenn Galvizo <[email protected]> Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Comment-Date: Wed, 03 Mar 2021 23:15:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
