Steven Jacobs has posted comments on this change. Change subject: Enabled Datasets to use Datatypes from foreign Dataverses ......................................................................
Patch Set 1: (26 comments) I addressed comments, and I am uploading my change. Hopefully it will build this time :( 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? I discovered that these tests really duplicate the work done by other tests, specifically, the tests called dataset, datatype, dataverse. 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 In this test the last two statements don't have paths on the type usage (tchrType) meaning that they will use the teacher dataverse 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 Done https://asterix-gerrit.ics.uci.edu/#/c/558/1/asterix-app/src/test/resources/runtimets/queries/cross-dataverse/drop-dataverse/drop-dataverse.1.ddl.aql File asterix-app/src/test/resources/runtimets/queries/cross-dataverse/drop-dataverse/drop-dataverse.1.ddl.aql: Line 22: : Try dropping the foreign type's dataverse > Remove tabs. Done https://asterix-gerrit.ics.uci.edu/#/c/558/1/asterix-app/src/test/resources/runtimets/queries/cross-dataverse/drop-dataverse/drop-dataverse.2.update.aql File asterix-app/src/test/resources/runtimets/queries/cross-dataverse/drop-dataverse/drop-dataverse.2.update.aql: Line 22: : Try dropping the foreign type's dataverse > Remove tabs. Done https://asterix-gerrit.ics.uci.edu/#/c/558/1/asterix-app/src/test/resources/runtimets/queries/cross-dataverse/drop-type-used-elsewhere/drop-type-used-elsewhere.1.ddl.aql File asterix-app/src/test/resources/runtimets/queries/cross-dataverse/drop-type-used-elsewhere/drop-type-used-elsewhere.1.ddl.aql: Line 22: : Try dropping the foreign datatype > Remove tabs. Done https://asterix-gerrit.ics.uci.edu/#/c/558/1/asterix-app/src/test/resources/runtimets/queries/cross-dataverse/drop-type-used-elsewhere/drop-type-used-elsewhere.2.update.aql File asterix-app/src/test/resources/runtimets/queries/cross-dataverse/drop-type-used-elsewhere/drop-type-used-elsewhere.2.update.aql: Line 22: : Try dropping the foreign datatype > Remove tabs. Done https://asterix-gerrit.ics.uci.edu/#/c/558/1/asterix-app/src/test/resources/runtimets/queries/cross-dataverse/drop-type-used-here-dataset/drop-type-used-here-dataset.1.ddl.aql File asterix-app/src/test/resources/runtimets/queries/cross-dataverse/drop-type-used-here-dataset/drop-type-used-here-dataset.1.ddl.aql: Line 22: : Try dropping the dataset > Remove tabs. Done https://asterix-gerrit.ics.uci.edu/#/c/558/1/asterix-app/src/test/resources/runtimets/queries/cross-dataverse/drop-type-used-here-dataset/drop-type-used-here-dataset.2.update.aql File asterix-app/src/test/resources/runtimets/queries/cross-dataverse/drop-type-used-here-dataset/drop-type-used-here-dataset.2.update.aql: Line 22: : Try dropping the dataset > Remove tabs. Done https://asterix-gerrit.ics.uci.edu/#/c/558/1/asterix-app/src/test/resources/runtimets/queries/cross-dataverse/drop-type-used-here-type/drop-type-used-here-type.1.ddl.aql File asterix-app/src/test/resources/runtimets/queries/cross-dataverse/drop-type-used-here-type/drop-type-used-here-type.1.ddl.aql: Line 22: : Try dropping the dataset > Remove tabs. Done https://asterix-gerrit.ics.uci.edu/#/c/558/1/asterix-app/src/test/resources/runtimets/queries/cross-dataverse/drop-type-used-here-type/drop-type-used-here-type.2.update.aql File asterix-app/src/test/resources/runtimets/queries/cross-dataverse/drop-type-used-here-type/drop-type-used-here-type.2.update.aql: Line 22: : Try dropping the dataset > Remove tabs. Done https://asterix-gerrit.ics.uci.edu/#/c/558/1/asterix-app/src/test/resources/runtimets/queries/cross-dataverse/query-dataset-with-foreign-type/query-dataset-with-foreign-type.1.ddl.aql File asterix-app/src/test/resources/runtimets/queries/cross-dataverse/query-dataset-with-foreign-type/query-dataset-with-foreign-type.1.ddl.aql: Line 23: * Expected Res : Success > Remove tabs. Done https://asterix-gerrit.ics.uci.edu/#/c/558/1/asterix-app/src/test/resources/runtimets/queries/cross-dataverse/query-dataset-with-foreign-type/query-dataset-with-foreign-type.2.update.aql File asterix-app/src/test/resources/runtimets/queries/cross-dataverse/query-dataset-with-foreign-type/query-dataset-with-foreign-type.2.update.aql: Line 22: : Query the dataset > Remove tabs. Done https://asterix-gerrit.ics.uci.edu/#/c/558/1/asterix-app/src/test/resources/runtimets/queries/cross-dataverse/query-dataset-with-foreign-type/query-dataset-with-foreign-type.3.query.aql File asterix-app/src/test/resources/runtimets/queries/cross-dataverse/query-dataset-with-foreign-type/query-dataset-with-foreign-type.3.query.aql: Line 22: : Query the dataset > Remove tabs. Done 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? I think this is because the changes I made to these existing queries did alter the results slightly. Previously, the assumption seems to have been "The datatype will always be from the same dataverse as specified by the dataset qualified name" But this seems incorrect now that we allow qualified datatype usage. So now the assumption is "If the datatype wasn't qualified, then it should be from the currently used dataverse" 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 Done 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. Done Line 737: private boolean confirmDatatypeIsUnused(JobId jobId, String dataverseName, String datatypeName) > Same here Done Line 743: private boolean confirmDatatypeIsUnusedByDatasets(JobId jobId, String dataverseName, String datatypeName) > And here Done Line 756: private boolean confirmDatatypeIsUnusedByDatatypes(JobId jobId, String dataverseName, String datatypeName) > And here Done Line 775: public String getDatatypeNameUsingThisAnonymousDatatype(JobId jobId, String dataverseName, String datatypeName) > The method has nothing specific to anonymous types, right? Yes, because it only returns a single type. If called on a non anonymous type, it would return only the "first" type using this type, which is not really valid information Line 781: if (dataverseDatatypes != null && dataverseDatatypes.size() > 0) { > This if statement is completely redundant here, getDataverseDatatypes() doe You are right. I had copied this from elsewhere in this file, so I removed it there as well. 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 Done Line 802: if (!(subType instanceof BuiltinType)) { > Use typeTags here instead of "instanceof" I'm not sure how. There are many different potential type tags that are all builtin Line 816: nodeGroupDatasets.add(new Pair<String, String>(set.getDataverseName(), set.getDatasetName())); > I am generally concerned about generating all those intermediate Pair<> obj Mike said that we shouldn't really be thinking about optimizing this use case, so I think altering the metadata would be a mistake. There is a much simpler solution. This check is only used to see whether a node group can be deleted, so it doesn't matter too much where the datasets come from, just that they exist. I'll make this change and see if you are okey with the new version 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. Done -- 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-Reviewer: Steven Jacobs <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-HasComments: Yes
