asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Yingyi Bu (Code Review)" <>
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:


1. The handling of insertion for datasets with meta doesn't make sense to me.  Maybe I miss

2. copy() and setOperatorId()  sounds a bit scary to me. Why do we have to change those low-level

Detailed comments are inlined.
File asterixdb/asterix-active/src/main/java/org/apache/asterix/active/

Line 36: 
Why not move start() and stop() cannot stay in IActiveRuntime
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?
File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/base/

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.
File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/extension/

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.
File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/extension/

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
File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/base/

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.
File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/

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?
File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/

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?
File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/

Line 525:             ArrayList<Mutable<ILogicalExpression>> exprs, LogicalVariable
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.
File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/util/

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.
File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/

Line 70:             ILangCompilationProvider sqlppCompilationProvider, IStatementExecutorFactory
queryTranslatorFactory) {
"queryTranslatorFactory" -> "statementExecutorFactory"
File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/

Line 35:     public AQLAPIServlet(ILangCompilationProvider compilationProvider, IStatementExecutorFactory
queryTranslatorFactory) {
variable name doesn't seem to match the type:
"queryTranslatorFactory" --> "statementExecutorFactory"
File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/

Line 43:     public DDLAPIServlet(ILangCompilationProvider compilationProvider, IStatementExecutorFactory
queryTranslatorFactory) {
"queryTranslatorFactory"-> "statementExecutorFactory"
File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/

Line 70:         this.queryTranslatorFactory = queryTranslatorFactory;
"queryTranslatorFactory" -> "statementExecutorFactory"
File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/

Line 40:             IStatementExecutorFactory queryTranslatorFactory) {
"queryTranslatorFactory" -> "statementExecutorFactory"
File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/java/

Line 65:             ILangCompilationProvider compilationProvider, IStatementExecutorFactory
queryTranslatorFactory) {
queryTranslatorFactory -> statementExecutorFactory
File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/cc/

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"?
File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/cc/

Line 25:  * An interface for extensions of {@code IQueryTranslator}
"IQueryTranslatorFactory" --> "IExtension"

Line 34:      * @return The extension implementation of the {@code IQueryTranslatorFactory}
"IQueryTranslatorFactory" --> "IExtension"
File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/

Line 35:     public void setCCExtensionManager(ICCExtensionManager ccExtensionManager) {
Pass that in the constructor?
File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/api/

Line 44:             ExtensionId oExtensionId = (ExtensionId) o;
Use org.apache.commons.lang3.ObjectUtils.
File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/extension/

Line 21: public interface ICCExtensionManager {
I guess the interface should have sth...  Otherwise its role can be played by Object.
File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/extension/

Line 21: public interface INCExtensionManager {
I guess the interface should have sth...  Otherwise its role can be played by Object.
File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/indexing/

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.
File asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/util/

Line 27: public class LangUtils {
Why renaming this class?
Function is a common concept across different languages, and this class only cares about functions...
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?
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"?
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?
File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/

Line 135:     private Map<ExtensionMetadataDatasetId, ExtensionMetadataDataset<?>>
Why is variable called "...Indexes"?  Call it "....Map"?
File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/api/

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.
File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/api/

Line 24: public interface ICachableMetadataEntity<T> extends IMetadataEntity {
Why do we need the "Cachable" prefix? Is there any MetadataEntity that is not "cachable"?
File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/api/

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?
File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/api/

Line 25: public interface IExtensionMetadataSearchKey extends Serializable {
Document the interface and methods.
File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/api/

Line 29: /**
A new line between the last import and comments. (Or, format the source file.)
File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/api/

Line 659:     <T extends IExtensionMetadataEntity> void deleteEntity(MetadataTransactionContext
mdTxnCtx, T entity)
Document those new methods in this interface.
File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/api/

Line 736:     <T extends IExtensionMetadataEntity> void deleteEntity(JobId jobId, T
Document those new methods.
File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entitytupletranslators/

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.
File hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/base/

Line 59:     public void setOperatorId(OperatorDescriptorId id) {
This seems a bit scary to me... The id should always be calculated instead of assigned.
File hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/connectors/

Line 95:         return new LocalityAwareMToNPartitioningConnectorDescriptor(spec, tpcf, localityMap).getConnectorId();
The "copy" of IConnectorDescriptorRegistry becomes a ConnectorDescriptorId? :-)

To view, visit
To unsubscribe, visit

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

View raw message