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: </dependency> 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<ILogicalOperator> 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<QueryResult> 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<Mutable<ILogicalExpression>> 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: <version>2.0.8</version> 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: <version>2.0.6</version> 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<ExtensionMetadataDatasetId, ExtensionMetadataDataset<?>> 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<T> 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: <T extends IExtensionMetadataEntity> 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: <T extends IExtensionMetadataEntity> 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 <bamou...@gmail.com> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Michael Blow <mb...@apache.org> Gerrit-Reviewer: Steven Jacobs <sjaco...@ucr.edu> Gerrit-Reviewer: Till Westmann <ti...@apache.org> Gerrit-Reviewer: Yingyi Bu <buyin...@gmail.com> Gerrit-Reviewer: abdullah alamoudi <bamou...@gmail.com> Gerrit-HasComments: Yes
