abdullah alamoudi has posted comments on this change. Change subject: ASTERIXDB-1711: remove some Aql-prefixes ......................................................................
Patch Set 2: (8 comments) https://asterix-gerrit.ics.uci.edu/#/c/1314/2/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/OptimizableOperatorSubTree.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/OptimizableOperatorSubTree.java: Line 230: if (datasource instanceof DataSource) { this has nothing to do with the change but can the datasource be an instance of something other than DataSource? If yes, give an example and whether we should return false in that case? if no, then why do we even have this check? https://asterix-gerrit.ics.uci.edu/#/c/1314/2/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/LangExpressionToPlanTranslator.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/LangExpressionToPlanTranslator.java: Line 1235: FunctionIdentifier fid = (lc.getType() == ListConstructor.Type.ORDERED_LIST_CONSTRUCTOR) why did we remove the import for ListConstructor.Type and imported DataSource.Type instead? I think we should have consistency. we either always use the qualified enum name, or have a different name that is more descriptive for enums. https://asterix-gerrit.ics.uci.edu/#/c/1314/2/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/AqlIndex.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/AqlIndex.java: Line 46: DataSourceId asid = new DataSourceId(dataverse, dataset); since we changed the class name, let's also change the variable name? https://asterix-gerrit.ics.uci.edu/#/c/1314/2/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/AqlMetadataProvider.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/AqlMetadataProvider.java: Line 170: public class AqlMetadataProvider implements IMetadataProvider<DataSourceId, String> { create an issue to rename this? Line 345: DataSource ads = findDataSource(dataSourceId); what is ads? rename the variable? https://asterix-gerrit.ics.uci.edu/#/c/1314/2/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/DatasetDataSource.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/DatasetDataSource.java: Line 113: DataSourceId asid = getId(); rename var? https://asterix-gerrit.ics.uci.edu/#/c/1314/2/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/LoadableDataSource.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/LoadableDataSource.java: Line 59: super(new DataSourceId("loadable_dv", "loadable_ds"), itemType, metaItemType, Type.LOADABLE, null); these two strings are supposed to be dataverse and dataset names. do we have dataverse or dataset with those names? no this then is broken and needs to be fixed. Not necessarily in this change but at least, let's create an issue for it. https://asterix-gerrit.ics.uci.edu/#/c/1314/2/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/MetadataManagerUtil.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/MetadataManagerUtil.java: Line 156: DataSourceId aqlId = id; why assign to another variable? -- To view, visit https://asterix-gerrit.ics.uci.edu/1314 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia0b64ffa7c50cd62fc3303fdb44eb769f56c978a Gerrit-PatchSet: 2 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Till Westmann <ti...@apache.org> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: abdullah alamoudi <bamou...@gmail.com> Gerrit-HasComments: Yes
