Return-Path: X-Original-To: archive-asf-public-internal@cust-asf2.ponee.io Delivered-To: archive-asf-public-internal@cust-asf2.ponee.io Received: from cust-asf.ponee.io (cust-asf.ponee.io [163.172.22.183]) by cust-asf2.ponee.io (Postfix) with ESMTP id 4E373200B62 for ; Fri, 12 Aug 2016 11:13:34 +0200 (CEST) Received: by cust-asf.ponee.io (Postfix) id 4CFC7160AB0; Fri, 12 Aug 2016 09:13:34 +0000 (UTC) Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by cust-asf.ponee.io (Postfix) with SMTP id 1DEF5160ABA for ; Fri, 12 Aug 2016 11:13:32 +0200 (CEST) Received: (qmail 89171 invoked by uid 500); 12 Aug 2016 08:58:56 -0000 Mailing-List: contact notifications-help@asterixdb.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@asterixdb.apache.org Delivered-To: mailing list notifications@asterixdb.apache.org Received: (qmail 51039 invoked by uid 99); 12 Aug 2016 07:02:53 -0000 Received: from pnap-us-west-generic-nat.apache.org (HELO spamd2-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 12 Aug 2016 07:02:53 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd2-us-west.apache.org (ASF Mail Server at spamd2-us-west.apache.org) with ESMTP id D56211A1039 for ; Fri, 12 Aug 2016 07:02:52 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd2-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: 0.919 X-Spam-Level: X-Spam-Status: No, score=0.919 tagged_above=-999 required=6.31 tests=[SPF_FAIL=0.919] autolearn=disabled Received: from mx2-lw-us.apache.org ([10.40.0.8]) by localhost (spamd2-us-west.apache.org [10.40.0.9]) (amavisd-new, port 10024) with ESMTP id H9S_I6zaQzjH for ; Fri, 12 Aug 2016 07:02:50 +0000 (UTC) Received: from unhygienix.ics.uci.edu (unhygienix.ics.uci.edu [128.195.14.130]) by mx2-lw-us.apache.org (ASF Mail Server at mx2-lw-us.apache.org) with ESMTP id 08E805F230 for ; Fri, 12 Aug 2016 07:02:50 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by unhygienix.ics.uci.edu (Postfix) with ESMTP id AD00C241E5C; Fri, 12 Aug 2016 00:02:49 -0700 (PDT) Date: Fri, 12 Aug 2016 00:02:49 -0700 From: "Yingyi Bu (Code Review)" To: abdullah alamoudi CC: Till Westmann , Michael Blow , Jenkins , Steven Jacobs Reply-To: buyingyi@gmail.com X-Gerrit-MessageType: comment Subject: Change in asterixdb[master]: Add Asterix Extension Manager X-Gerrit-Change-Id: I280268495cc3aad00f898cba21f7299f7120ce5c X-Gerrit-ChangeURL: X-Gerrit-Commit: 3a2f4b3d682f066bc5a326088418251db0d28ffa In-Reply-To: References: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Content-Disposition: inline User-Agent: Gerrit/2.8.4 Message-Id: <20160812070249.AD00C241E5C@unhygienix.ics.uci.edu> archived-at: Fri, 12 Aug 2016 09:13:34 -0000 Yingyi Bu has posted comments on this change. Change subject: Add Asterix Extension Manager ...................................................................... Patch Set 7: (61 comments) 1. The handling of insertion for datasets with meta doesn't make sense to me. Maybe I miss something. 2. copy() and setOperatorId() sounds a bit scary to me. Why do we have to change those low-level stuff? Detailed comments are inlined. https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-active/src/main/java/org/apache/asterix/active/ActiveRuntime.java File asterixdb/asterix-active/src/main/java/org/apache/asterix/active/ActiveRuntime.java: Line 36: Why not move start() and stop() cannot stay in IActiveRuntime https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-algebra/pom.xml File asterixdb/asterix-algebra/pom.xml: Line 163: It seems you don't need to add this dependency explicitly here as it is a dependency of asterix-metadata? https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/base/ILangExtension.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/base/ILangExtension.java: Line 44: ILangCompilationProvider getAqlLangCompilationProvider(); > It seems that the extension mechanism shouldn't hard-code the AQL/SQL++ dua Agree with Till. Line 60: */ The presence of this method here seems odd to me, as this interface is supposed to be at a higher abstraction level. But I understand why it is put here. I think we could abstract Rule collections as well, for the case that an extension may customize/disable/enable some rewriting rules. Then, you don't need this method, but instead, you can have IRuleCollectionProvider getRuleCollectionProvider(); I can see that might require bigger changes and might not be a good idea to put into this change. If you don't address this, please file an issue for refactoring this interface and assign to me. https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/extension/IAlgebraExtensionManager.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/extension/IAlgebraExtensionManager.java: Line 32: boolean unnestToDataScan(Mutable opRef, IOptimizationContext context, UnnestOperator unnestOp, Again, the presence of this method seems odd to me. I think the right way to refactor rules and provide abstractions like IRuleCollection and IRuleCollectionProvider. https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/extension/IExtensionStatement.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/extension/IExtensionStatement.java: Line 26: public interface IExtensionStatement extends Statement { Document this public interface. Line 34: * Called when the {@code IQueryTranslator} encounters an extension statement The annotation could be sth. like "An implementation class should implement the actual processing of the statement in this method." Line 42: void handle(IStatementExecutor queryTranslator, AqlMetadataProvider metadataProvider, IHyracksClientConnection hcc) queryTranslator -> statementExecutor https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/base/RuleCollections.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/base/RuleCollections.java: Line 154: IAlgebraExtensionManager algebraExtensionManager) { refactoring needed. Line 176: normalization.add(new UnnestToDataScanRule(algebraExtensionManager)); refactoring needed. Not necessary in this change. File an issue if it is not addressed. https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/IStatementExecutor.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/IStatementExecutor.java: Line 34: Add a brief comment for the interface. Line 40: ASYNC_DEFERRED Add brief comments for each enum value. Line 50: * @param resultDelivery The comment seems out-dated -- resultDelivery is not a boolean value. Line 52: * @return A List containing a QueryResult instance corresponding to each submitted query. The documentation of @return seems not right? Line 62: ICompiledDmlStatement clfrqs) throws AsterixException, RemoteException, AlgebricksException, JSONException, ACIDException; meaningful variable name -- what does clfrqs mean? Line 63: The relationship of compileAndExecute and rewriteCompileQuery is not very clear to me. It seems "rewriteCompileQuery" is one step in compileAndExecute(...), though for DML/DDLs it is a non-op. Do we need to expose that as an interface method? https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/IStatementExecutorFactory.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/IStatementExecutorFactory.java: Line 28: public interface IStatementExecutorFactory { Document this interface and each method. Line 33: void setCCExtensionManager(ICCExtensionManager ccExtensionManager); Why do we need this set method here? Can't this parameter be passed to the implementation class through its constructor? https://asterix-gerrit.ics.uci.edu/#/c/1017/7/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 525: ArrayList> exprs, LogicalVariable resVar, ArrayList -> List Line 540: IFunctionInfo finfoMeta = LangUtils.getFunctionInfo(AsterixBuiltinFunctions.META); The handling of meta part doesn't make sense to me. Maybe I miss something. The meta part of a target dataset tuple could be composed by any valid expressions rather than only meta(foo) from the source dataset. In fact, there is no relationship between the meta part of a target dataset and the meta part of a source dataset. https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/util/AlgebraUtils.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/util/AlgebraUtils.java: Line 31: public class AlgebraUtils { 1. Rename: "AlgebraUtils" -> "ConstantExpressionUtil" The original name indicates a larger scope. 2. Can we make the methods more general? Take an ILogicalExpression, return a constant integer or string, instead of only being able to process function expressions' arguments. https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/APIServlet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/APIServlet.java: Line 70: ILangCompilationProvider sqlppCompilationProvider, IStatementExecutorFactory queryTranslatorFactory) { "queryTranslatorFactory" -> "statementExecutorFactory" https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/AQLAPIServlet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/AQLAPIServlet.java: Line 35: public AQLAPIServlet(ILangCompilationProvider compilationProvider, IStatementExecutorFactory queryTranslatorFactory) { variable name doesn't seem to match the type: "queryTranslatorFactory" --> "statementExecutorFactory" https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/DDLAPIServlet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/DDLAPIServlet.java: Line 43: public DDLAPIServlet(ILangCompilationProvider compilationProvider, IStatementExecutorFactory queryTranslatorFactory) { "queryTranslatorFactory"-> "statementExecutorFactory" https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/QueryServiceServlet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/QueryServiceServlet.java: Line 70: this.queryTranslatorFactory = queryTranslatorFactory; "queryTranslatorFactory" -> "statementExecutorFactory" https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/UpdateAPIServlet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/UpdateAPIServlet.java: Line 40: IStatementExecutorFactory queryTranslatorFactory) { "queryTranslatorFactory" -> "statementExecutorFactory" https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/java/AsterixJavaClient.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/java/AsterixJavaClient.java: Line 65: ILangCompilationProvider compilationProvider, IStatementExecutorFactory queryTranslatorFactory) { queryTranslatorFactory -> statementExecutorFactory https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/cc/CCExtensionManager.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/cc/CCExtensionManager.java: Line 49: * AsterixDB's implementation of {@code ICCExtensionManager} which takes care of ICCExtensionManager --> IAlgebraExtensionManager Line 50: * initializing extensions on the Cluster Controller Conceptually, this class has nothing to do with "cluster controller". Line 52: public class CCExtensionManager implements IAlgebraExtensionManager { CCExtensionManager --> "CompilerExtensionManager"? https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/cc/IQueryTranslatorExtension.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/cc/IQueryTranslatorExtension.java: Line 25: * An interface for extensions of {@code IQueryTranslator} "IQueryTranslatorFactory" --> "IExtension" Line 34: * @return The extension implementation of the {@code IQueryTranslatorFactory} "IQueryTranslatorFactory" --> "IExtension" https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/DefaultQueryTranslatorFactory.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/DefaultQueryTranslatorFactory.java: Line 35: public void setCCExtensionManager(ICCExtensionManager ccExtensionManager) { Pass that in the constructor? https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/api/ExtensionId.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/api/ExtensionId.java: Line 44: ExtensionId oExtensionId = (ExtensionId) o; Use org.apache.commons.lang3.ObjectUtils. https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/extension/ICCExtensionManager.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/extension/ICCExtensionManager.java: Line 21: public interface ICCExtensionManager { I guess the interface should have sth... Otherwise its role can be played by Object. https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/extension/INCExtensionManager.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/extension/INCExtensionManager.java: Line 21: public interface INCExtensionManager { I guess the interface should have sth... Otherwise its role can be played by Object. https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/indexing/ExternalFile.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/indexing/ExternalFile.java: Line 119: You should override hashCode(). Use org.apache.commons.lang3.ObjectUtils to implement that. Line 123: return false; Remove the first if block, as it is implied by the third if block. null is not an instance of anything. Line 127: } Uses org.apache.commons.lang3.ObjectUtils. https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/util/LangUtils.java File asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/util/LangUtils.java: Line 27: public class LangUtils { Why renaming this class? Function is a common concept across different languages, and this class only cares about functions... https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-maven-plugins/asterix-grammar-extension-maven-plugin/pom.xml File asterixdb/asterix-maven-plugins/asterix-grammar-extension-maven-plugin/pom.xml: Line 36: 2.0.8 Use 3.4 as well, like the above one? https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-maven-plugins/asterix-grammar-extension-maven-plugin/src/test/resources/lang/extension.jj File asterixdb/asterix-maven-plugins/asterix-grammar-extension-maven-plugin/src/test/resources/lang/extension.jj: Line 8: // As a workaround, you can always override >> refrain from using the strings "before:" and "after:". Not sure what does it mean? Do you mean any text in the merge areas couldn't have substrings be "before:" or "after:" Line 9: // one additional possible change is direct replacement and it can be done through @merge replace existing with new 1. Does the sentence complete? 2. WS. Line 26: what does "The default" mean? Line 27: // The default what is "node"? https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-maven-plugins/asterix-grammar-extension-maven-plugin/src/test/resources/unit/basic-test/basic-test-plugin-config.xml File asterixdb/asterix-maven-plugins/asterix-grammar-extension-maven-plugin/src/test/resources/unit/basic-test/basic-test-plugin-config.xml: Line 29: 2.0.6 Using the maven version consistent to other places? e.g., 3.4? https://asterix-gerrit.ics.uci.edu/#/c/1017/7/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: Line 135: private Map> extensionIndexes; Why is variable called "...Indexes"? Call it "....Map"? https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/api/ExtensionMetadataDatasetId.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/api/ExtensionMetadataDatasetId.java: Line 31: public ExtensionMetadataDatasetId(ExtensionId extensionId, String indexName) { indexName -> datasetName? Line 40: public String getIndexName() { IndexName -> DatasetName? (since this class is called ExtensionMetadataDatasetId) Line 48: return extensionId.equals(otherId.getExtensionId()) && indexName.equals(otherId.getIndexName()); Use org.apache.commons.lang3.ObjectUtils.equals(...) to make it simple. Line 55: return indexName.hashCode(); Why not also use extendId here? Use org.apache.commons.lang3.ObjectUtils.hashCodeMulti(...) to make it simple. https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/api/ICachableMetadataEntity.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/api/ICachableMetadataEntity.java: Line 24: public interface ICachableMetadataEntity extends IMetadataEntity { Why do we need the "Cachable" prefix? Is there any MetadataEntity that is not "cachable"? https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/api/IExtensionMetadataEntity.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/api/IExtensionMetadataEntity.java: Line 23: public interface IExtensionMetadataEntity extends IMetadataEntity { Document the interface and methods Line 25: ExtensionMetadataDatasetId getIndexId(); Why does every extension metadata entity have an index? https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/api/IExtensionMetadataSearchKey.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/api/IExtensionMetadataSearchKey.java: Line 25: public interface IExtensionMetadataSearchKey extends Serializable { Document the interface and methods. https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/api/IMetadataExtension.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/api/IMetadataExtension.java: Line 29: /** A new line between the last import and comments. (Or, format the source file.) https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/api/IMetadataManager.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/api/IMetadataManager.java: Line 659: void deleteEntity(MetadataTransactionContext mdTxnCtx, T entity) Document those new methods in this interface. https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/api/IMetadataNode.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/api/IMetadataNode.java: Line 736: void deleteEntity(JobId jobId, T entity) Document those new methods. https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entitytupletranslators/MetadataTupleTranslatorProvider.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entitytupletranslators/MetadataTupleTranslatorProvider.java: Line 26: public class MetadataTupleTranslatorProvider implements Serializable{ Does this Provider need to transfer across nodes? From the name "provider", it doesn't sound like sth. that needs to move across nodes. https://asterix-gerrit.ics.uci.edu/#/c/1017/7/hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/base/AbstractOperatorDescriptor.java File hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/base/AbstractOperatorDescriptor.java: Line 59: public void setOperatorId(OperatorDescriptorId id) { This seems a bit scary to me... The id should always be calculated instead of assigned. https://asterix-gerrit.ics.uci.edu/#/c/1017/7/hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/connectors/LocalityAwareMToNPartitioningConnectorDescriptor.java File hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/connectors/LocalityAwareMToNPartitioningConnectorDescriptor.java: Line 95: return new LocalityAwareMToNPartitioningConnectorDescriptor(spec, tpcf, localityMap).getConnectorId(); The "copy" of IConnectorDescriptorRegistry becomes a ConnectorDescriptorId? :-) -- To view, visit https://asterix-gerrit.ics.uci.edu/1017 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I280268495cc3aad00f898cba21f7299f7120ce5c Gerrit-PatchSet: 7 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: abdullah alamoudi Gerrit-Reviewer: Jenkins Gerrit-Reviewer: Michael Blow Gerrit-Reviewer: Steven Jacobs Gerrit-Reviewer: Till Westmann Gerrit-Reviewer: Yingyi Bu Gerrit-Reviewer: abdullah alamoudi Gerrit-HasComments: Yes