asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Yingyi Bu (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: Add Asterix Extension Manager
Date Fri, 12 Aug 2016 07:02:49 GMT
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 <bamousaa@gmail.com>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Michael Blow <mblow@apache.org>
Gerrit-Reviewer: Steven Jacobs <sjaco002@ucr.edu>
Gerrit-Reviewer: Till Westmann <tillw@apache.org>
Gerrit-Reviewer: Yingyi Bu <buyingyi@gmail.com>
Gerrit-Reviewer: abdullah alamoudi <bamousaa@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message