Ildar Absalyamov has posted comments on this change. Change subject: Enabled Datasets to use Datatypes from foreign Dataverses ......................................................................
Patch Set 1: (15 comments) Following our conversation on Tuesday I believe the static field offset constants for in *TupleTranalators are not needed. Those constants are not duplicates of the constants in MetadataRecordTypes either. The payload offset in each in the translators is numerically equal to number of primary keys for particular dataset, which is statically assigned in MetadataPrimaryIndexes. Neither of offsets other than payload are used, so I thinks since we're there we can refactor this code https://asterix-gerrit.ics.uci.edu/#/c/558/1/asterix-app/src/test/resources/metadata/queries/basic/meta16/meta16.3.query.aql File asterix-app/src/test/resources/metadata/queries/basic/meta16/meta16.3.query.aql: Line 24 Why all these tests were deleted? https://asterix-gerrit.ics.uci.edu/#/c/558/1/asterix-app/src/test/resources/runtimets/queries/cross-dataverse/cross-dv01/cross-dv01.1.ddl.aql File asterix-app/src/test/resources/runtimets/queries/cross-dataverse/cross-dv01/cross-dv01.1.ddl.aql: Line 33: use dataverse teacher; If we can use fully qualified name for types/dataset is there still a need for use statement? https://asterix-gerrit.ics.uci.edu/#/c/558/1/asterix-app/src/test/resources/runtimets/queries/cross-dataverse/cross-dv01/cross-dv01.2.update.aql File asterix-app/src/test/resources/runtimets/queries/cross-dataverse/cross-dv01/cross-dv01.2.update.aql: Line 28: use dataverse teacher; Same here https://asterix-gerrit.ics.uci.edu/#/c/558/1/asterix-app/src/test/resources/runtimets/results/cross-dataverse/cross-dv04/cross-dv04.1.adm File asterix-app/src/test/resources/runtimets/results/cross-dataverse/cross-dv04/cross-dv04.1.adm: Line 1: { "DataverseName": "student", "DatasetName": "gdstd", "DatatypeDataverseName": "student", "DatatypeName": "stdType", "DatasetType": "INTERNAL", "GroupName": "DEFAULT_NG_ALL_NODES", "CompactionPolicy": "prefix", "CompactionPolicyProperties": [ { "Name": "max-mergable-component-size", "Value": "1073741824" }, { "Name": "max-tolerance-component-count", "Value": "5" } ], "InternalDetails": { "FileStructure": "BTREE", "PartitioningStrategy": "HASH", "PartitioningKey": [ [ "id" ] ], "PrimaryKey": [ [ "id" ] ], "Autogenerated": false }, "ExternalDetails": null, "Hints": {{ }}, "Timestamp": "Thu Dec 17 12:58:43 PST 2015", "DatasetId": 118i32, "PendingOp": 0i32 } Why dataset IDs changed here, but not in the previous test case? https://asterix-gerrit.ics.uci.edu/#/c/558/1/asterix-lang-aql/src/main/javacc/AQL.jj File asterix-lang-aql/src/main/javacc/AQL.jj: Line 422: <LEFTPAREN> typeComponents = TypeName() <RIGHTPAREN> Documentation, describing AQL should be updated accordingly https://asterix-gerrit.ics.uci.edu/#/c/558/1/asterix-metadata/src/main/java/org/apache/asterix/metadata/MetadataNode.java File asterix-metadata/src/main/java/org/apache/asterix/metadata/MetadataNode.java: Line 719: private boolean confirmDataverseCanBeDeleted(JobId jobId, String dataverseName) Seems like the method does not need to return anything. Line 737: private boolean confirmDatatypeIsUnused(JobId jobId, String dataverseName, String datatypeName) Same here Line 743: private boolean confirmDatatypeIsUnusedByDatasets(JobId jobId, String dataverseName, String datatypeName) And here Line 756: private boolean confirmDatatypeIsUnusedByDatatypes(JobId jobId, String dataverseName, String datatypeName) And here Line 775: public String getDatatypeNameUsingThisAnonymousDatatype(JobId jobId, String dataverseName, String datatypeName) The method has nothing specific to anonymous types, right? Line 781: if (dataverseDatatypes != null && dataverseDatatypes.size() > 0) { This if statement is completely redundant here, getDataverseDatatypes() does not return null, and the 2nd part of the predicate is redundant Line 795: private List<String> getNestedDatatypeNamesForThisDatatype(JobId jobId, String dataverseName, String datatypeName) The name of the method should imply that the returned datatypes are complex rather then builtin Line 802: if (!(subType instanceof BuiltinType)) { Use typeTags here instead of "instanceof" Line 816: nodeGroupDatasets.add(new Pair<String, String>(set.getDataverseName(), set.getDatasetName())); I am generally concerned about generating all those intermediate Pair<> objects. If the only thing they are designed for is to carry information about dataverse+dataset, then we need to refactor how this infomation is stored in Dataset object. Maybe it's worth to introduce nested object https://asterix-gerrit.ics.uci.edu/#/c/558/1/asterix-metadata/src/main/java/org/apache/asterix/metadata/valueextractors/DatatypeNameValueExtractor.java File asterix-metadata/src/main/java/org/apache/asterix/metadata/valueextractors/DatatypeNameValueExtractor.java: Line 59: // Get index 0 because it is anonymous type, and it is used in Comment seems to be not valid anymore. -- To view, visit https://asterix-gerrit.ics.uci.edu/558 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I24dbc04dcb2a4126fc8361ebe3104877a0d1f2bb Gerrit-PatchSet: 1 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Steven Jacobs <[email protected]> Gerrit-Reviewer: Ildar Absalyamov <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Preston Carman <[email protected]> Gerrit-HasComments: Yes
