abdullah alamoudi has posted comments on this change. Change subject: Add Asterix Extension Manager ......................................................................
Patch Set 7: (25 comments) 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 Class is gone :) https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-active/src/main/java/org/apache/asterix/active/runtime/StopActiveRuntimeOperatorDescriptor.java File asterixdb/asterix-active/src/main/java/org/apache/asterix/active/runtime/StopActiveRuntimeOperatorDescriptor.java: Line 40: public StopActiveRuntimeOperatorDescriptor(IOperatorDescriptorRegistry spec, ActiveRuntimeId runtimeId) { > This class is never used. So I'm wondering if there's something we can do t Class is gone. As I pointed out before that this mechanism of communicating with the active manager is bad. 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 d Done 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 Done Line 44: ILangCompilationProvider getAqlLangCompilationProvider(); > Agree with Till. Done Line 60: */ > The presence of this method here seems odd to me, as this interface is supp Done Line 62: UnnestOperator unnestOp, ILogicalExpression unnestExpr, AbstractFunctionCallExpression functionCallExpr) throws AlgebricksException; > MAJOR SonarQube violation: Done 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 t I totally agree but think that we should refactor it in another change :) 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. Done Line 34: * Called when the {@code IQueryTranslator} encounters an extension statement > The annotation could be sth. like "An implementation class should implement Done Line 35: * > MAJOR SonarQube violation: Done Line 42: void handle(IStatementExecutor queryTranslator, AqlMetadataProvider metadataProvider, IHyracksClientConnection hcc) > queryTranslator -> statementExecutor Done Line 43: throws Exception; > CRITICAL SonarQube violation: Done 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. Done Line 176: normalization.add(new UnnestToDataScanRule(algebraExtensionManager)); > refactoring needed. Not necessary in this change. File an issue if it is Done 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. Done Line 40: ASYNC_DEFERRED > Add brief comments for each enum value. Done Line 50: * @param resultDelivery > The comment seems out-dated -- resultDelivery is not a boolean value. Done Line 52: * @return A List<QueryResult> containing a QueryResult instance corresponding to each submitted query. > The documentation of @return seems not right? Done Line 56: throws Exception; > CRITICAL SonarQube violation: Done Line 62: ICompiledDmlStatement clfrqs) throws AsterixException, RemoteException, AlgebricksException, JSONException, ACIDException; > meaningful variable name -- what does clfrqs mean? Done Line 63: > The relationship of compileAndExecute and rewriteCompileQuery is not very c I had the same question but then I needed this since I wanted to do the compilation without the execution. 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. Done Line 33: void setCCExtensionManager(ICCExtensionManager ccExtensionManager); > Why do we need this set method here? Can't this parameter be passed to the Done 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 540: IFunctionInfo finfoMeta = LangUtils.getFunctionInfo(AsterixBuiltinFunctions.META); > The handling of meta part doesn't make sense to me. Maybe I miss something Yingyi, the source in this case is always the feed data source. We didn't figure out yet how to handle DML statements with non-feed modifications. -- 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
