asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "abdullah alamoudi (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: ASTERIXDB-1711: remove some Aql-prefixes
Date Sat, 29 Oct 2016 02:35:42 GMT
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 <tillw@apache.org>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: abdullah alamoudi <bamousaa@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message