>From Dmitry Lychagin <[email protected]>: Dmitry Lychagin has posted comments on this change. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/3609 )
Change subject: [WIP] create function & adapter ddl ...................................................................... Patch Set 16: (22 comments) Here's the first batch. Let's discuss tomorrow. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/3609/16/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/jobgen/QueryLogicalExpressionJobGen.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/jobgen/QueryLogicalExpressionJobGen.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/3609/16/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/jobgen/QueryLogicalExpressionJobGen.java@144 PS16, Line 144: fd = ExternalFunctionDescriptorProvider.getExternalFunctionDescriptor( minor. Can we refactor lines 144-147 into a separate method (resolveExternalFunction())? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/3609/16/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/SetAsterixMemoryRequirementsRule.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/SetAsterixMemoryRequirementsRule.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/3609/16/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/SetAsterixMemoryRequirementsRule.java@89 PS16, Line 89: } minor. let's remove these empty lines. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/3609/16/asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/statement/CreateAdapterStatement.java File asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/statement/CreateAdapterStatement.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/3609/16/asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/statement/CreateAdapterStatement.java@35 PS16, Line 35: public CreateAdapterStatement(AdapterIdentifier signature, String lang, String libName, String externalIdent, Can we avoid using AdapterIdentifier here, so we could avoid dependency on asterix-external-data? Instead we could have two parameters/fields: DataverseName dataverseName, and String adapterName https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/3609/16/asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/struct/Identifier.java File asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/struct/Identifier.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/3609/16/asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/struct/Identifier.java@25 PS16, Line 25: protected String value; can we keep it final? It's useful to be able to share Identifier instances without having to worry that another code might change the value. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/3609/16/asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/struct/VarIdentifier.java File asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/struct/VarIdentifier.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/3609/16/asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/struct/VarIdentifier.java@24 PS16, Line 24: protected int id = 0; 0 is the default, no need to assign it here. Also why is it protected and to private anymore? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/3609/16/asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/visitor/FormatPrintVisitor.java File asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/visitor/FormatPrintVisitor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/3609/16/asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/visitor/FormatPrintVisitor.java@830 PS16, Line 830: out.print("("); this method is incomplete. doesn't print the adapter info. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/3609/16/asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/visitor/FormatPrintVisitor.java@994 PS16, Line 994: protected String normalize(DataverseName dataverseName) { we need to remove this method. it's a duplicate of generateDataverseName(). https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/3609/16/asterixdb/asterix-lang-sqlpp/pom.xml File asterixdb/asterix-lang-sqlpp/pom.xml: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/3609/16/asterixdb/asterix-lang-sqlpp/pom.xml@149 PS16, Line 149: <groupId>org.apache.asterix</groupId> let's see if we can remove this new dependency if we modify CreateAdapterStatement to accept DataverseName and String for adapter name instead. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/3609/16/asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj File asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/3609/16/asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj@912 PS16, Line 912: createNewScope(); don't call createNewScope() here. Function production does it because it processes function body, but adapters have no body expression. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/3609/16/asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj@915 PS16, Line 915: <IDENTIFIER> {expectToken(ADAPTER);} fctName = FunctionName() use QualifiedName() production instead of FunctionName(). It returns a pair. DataverseName can be returned as null, this case is handled inside QueryTranslator. See how QueryTranslator.getActiveDataverseName() is used. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/3609/16/asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj@917 PS16, Line 917: defaultDataverse = fctName.dataverse; See my comment above. default dataverse handling for statements is done by the QueryTranslator. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/3609/16/asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj@922 PS16, Line 922: defaultDataverse = currentDataverse; this line needs to be removed. See above comments. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/3609/16/asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj@924 PS16, Line 924: new CreateAdapterStatement(signature, lang, libName, externalIdent, ifNotExists); ifNotExists is not assigned. Add ifNotExists = IfNotExists() after line 915 https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/3609/16/asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj@931 PS16, Line 931: List<TypedVarIdentifier> FunctionArgs() : I thought we decided not to have parameter types initially? So then this production won't be needed. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/3609/16/asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj@1010 PS16, Line 1010: createNewScope(); createNewScope() only needed for an inline function. Let's move it into that branch. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/3609/16/asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj@1023 PS16, Line 1023: ( (<RETURN> returnType = TypeExpr() (<QUES> {returnNullable = true;})?)? Same comment regarding types. I thought we decided not to have the return type initially. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/3609/16/asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj@1024 PS16, Line 1024: (LOOKAHEAD(3,{getToken(1).kind == IDENTIFIER && getToken(2).kind == IDENTIFIER && getToken(3).kind != AS})(<IDENTIFIER> {deterministic = true;}))? try: ( LOOKAHEAD({ laIdentifier(DETERMINISTIC) }) <IDENTIFIER> { deterministic = true } )? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/3609/16/asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj@1025 PS16, Line 1025: (<NULL> <IDENTIFIER> { if(isToken(CALL)) nullCall = true;})? change action to: { expectToken(CALL); nullCall = true } This current code doesn't fail it the token is not CALL, but it should. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/3609/16/asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj@1026 PS16, Line 1026: (<EXTERNAL> <IDENTIFIER> {if(isToken(ACTION)) externalAction = true;})? We don't have a usecase for EXTERNAL ACTION at this point. Can we remove it from the grammar until we have one? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/3609/16/asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj@1042 PS16, Line 1042: | ( <LEFTBRACE> I'd put LEFTBRACE alternative (inline function) first, so before the external function. It's easier to deal with, because LEFTBRACE is required for the parsing decision, while the external function alternative has several optional tokens. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/3609/16/asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj@1759 PS16, Line 1759: remove empty line https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/3609/16/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entities/BuiltinTypeMap.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entities/BuiltinTypeMap.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/3609/16/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entities/BuiltinTypeMap.java@82 PS16, Line 82: _builtinTypeMap.put("any", BuiltinType.ANY); why 'any' was added? is it necessary? -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/3609 To unsubscribe, or for help writing mail filters, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Change-Id: Ic3c1e98c183cd214eea3e4fee24b2b7c46166b32 Gerrit-Change-Number: 3609 Gerrit-PatchSet: 16 Gerrit-Owner: Ian Maxon <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Dmitry Lychagin <[email protected]> Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Comment-Date: Tue, 26 Nov 2019 01:24:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
