[NO ISSUE][SQLPP] Refactor grammar for CREATE FUNCTION

- user model changes: no
- storage format changes: no
- interface changes: no

Details:
- Refactor SQL++ grammar for CREATE FUNCTION
  to improve its extensibility

Change-Id: I1d71de74e64318ef5daad91d782f0dc329ee5c5b
Reviewed-on: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/4963
Contrib: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Integration-Tests: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Tested-by: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Reviewed-by: Dmitry Lychagin <dmitry.lychagin@couchbase.com>
Reviewed-by: Ian Maxon <imaxon@uci.edu>
diff --git a/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/QueryTranslator.java b/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/QueryTranslator.java
index 2998f7b..93fcc7e 100644
--- a/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/QueryTranslator.java
+++ b/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/QueryTranslator.java
@@ -1764,16 +1764,27 @@
     protected void handleCreateFunctionStatement(MetadataProvider metadataProvider, Statement stmt,
             IStatementRewriter stmtRewriter) throws Exception {
         CreateFunctionStatement cfs = (CreateFunctionStatement) stmt;
-        SourceLocation sourceLoc = cfs.getSourceLocation();
         FunctionSignature signature = cfs.getFunctionSignature();
-        DataverseName dataverseName = getActiveDataverseName(signature.getDataverseName());
-        signature.setDataverseName(dataverseName);
+        signature.setDataverseName(getActiveDataverseName(signature.getDataverseName()));
+        String libraryName = cfs.getLibName();
 
+        lockUtil.createFunctionBegin(lockManager, metadataProvider.getLocks(), signature.getDataverseName(),
+                signature.getName(), libraryName);
+        try {
+            doCreateFunction(metadataProvider, cfs, signature, stmtRewriter);
+        } finally {
+            metadataProvider.getLocks().unlock();
+            metadataProvider.setDefaultDataverse(activeDataverse);
+        }
+    }
+
+    protected void doCreateFunction(MetadataProvider metadataProvider, CreateFunctionStatement cfs,
+            FunctionSignature signature, IStatementRewriter stmtRewriter) throws Exception {
+        DataverseName dataverseName = signature.getDataverseName();
+        String libraryName = cfs.getLibName();
+        SourceLocation sourceLoc = cfs.getSourceLocation();
         MetadataTransactionContext mdTxnCtx = MetadataManager.INSTANCE.beginTransaction();
         metadataProvider.setMetadataTxnContext(mdTxnCtx);
-        String libraryName = cfs.getLibName();
-        lockUtil.createFunctionBegin(lockManager, metadataProvider.getLocks(), dataverseName, signature.getName(),
-                libraryName);
         try {
             Dataverse dv = MetadataManager.INSTANCE.getDataverse(mdTxnCtx, dataverseName);
             if (dv == null) {
@@ -1872,13 +1883,9 @@
                 }
                 MetadataManager.INSTANCE.commitTransaction(mdTxnCtx);
             }
-
         } catch (Exception e) {
             abort(e, e, mdTxnCtx);
             throw e;
-        } finally {
-            metadataProvider.getLocks().unlock();
-            metadataProvider.setDefaultDataverse(activeDataverse);
         }
     }
 
@@ -1970,14 +1977,23 @@
 
     protected void handleFunctionDropStatement(MetadataProvider metadataProvider, Statement stmt) throws Exception {
         FunctionDropStatement stmtDropFunction = (FunctionDropStatement) stmt;
-        SourceLocation sourceLoc = stmtDropFunction.getSourceLocation();
         FunctionSignature signature = stmtDropFunction.getFunctionSignature();
         signature.setDataverseName(getActiveDataverseName(signature.getDataverseName()));
-        MetadataTransactionContext mdTxnCtx = MetadataManager.INSTANCE.beginTransaction();
-        metadataProvider.setMetadataTxnContext(mdTxnCtx);
         lockUtil.dropFunctionBegin(lockManager, metadataProvider.getLocks(), signature.getDataverseName(),
                 signature.getName());
         try {
+            doDropFunction(metadataProvider, stmtDropFunction, signature);
+        } finally {
+            metadataProvider.getLocks().unlock();
+        }
+    }
+
+    protected void doDropFunction(MetadataProvider metadataProvider, FunctionDropStatement stmtDropFunction,
+            FunctionSignature signature) throws Exception {
+        SourceLocation sourceLoc = stmtDropFunction.getSourceLocation();
+        MetadataTransactionContext mdTxnCtx = MetadataManager.INSTANCE.beginTransaction();
+        metadataProvider.setMetadataTxnContext(mdTxnCtx);
+        try {
             Function function = MetadataManager.INSTANCE.getFunction(mdTxnCtx, signature);
             // If function == null && stmtDropFunction.getIfExists() == true, commit txn directly.
             if (function == null && !stmtDropFunction.getIfExists()) {
@@ -1993,8 +2009,6 @@
         } catch (Exception e) {
             abort(e, e, mdTxnCtx);
             throw e;
-        } finally {
-            metadataProvider.getLocks().unlock();
         }
     }
 
diff --git a/asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj b/asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj
index 768de7e..3681445 100644
--- a/asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj
+++ b/asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj
@@ -467,7 +467,13 @@
     }
 
     private SqlppParseException createUnexpectedTokenError() {
-      return new SqlppParseException(getSourceLocation(token), "Unexpected token: " + LogRedactionUtil.userData(token.image));
+      return createUnexpectedTokenError(null);
+    }
+
+    private SqlppParseException createUnexpectedTokenError(String expected) {
+      String message = "Unexpected token: " + LogRedactionUtil.userData(token.image) +
+        (expected == null ? "" : ". Expected: " + LogRedactionUtil.userData(expected));
+      return new SqlppParseException(getSourceLocation(token), message);
     }
 
     private boolean laToken(int idx, int kind, String image) {
@@ -1039,33 +1045,6 @@
 
 }
 
-
-List<Pair<VarIdentifier,IndexedTypeExpression>> FunctionParameters() :
-{
-    List<Pair<VarIdentifier,IndexedTypeExpression>> paramList = new ArrayList<Pair<VarIdentifier,IndexedTypeExpression>>();
-    Pair<VarIdentifier,IndexedTypeExpression> varPair = new Pair<VarIdentifier,IndexedTypeExpression>(null,null);
-    VarIdentifier var = null;
-    IndexedTypeExpression type = null;
-    boolean typed = false;
-}
-{
-    <LEFTPAREN>( (<IDENTIFIER>
-    {
-        var = SqlppVariableUtil.toInternalVariableIdentifier(token.image);
-        varPair.setFirst(new VarIdentifier(var.toString()));
-        paramList.add(varPair);
-    } (LOOKAHEAD(3)(<COLON>)? type = IndexedTypeExpr() {varPair.setSecond(type);} )? (<COMMA> <IDENTIFIER>{
-        var = SqlppVariableUtil.toInternalVariableIdentifier(token.image);
-        varPair = new Pair<VarIdentifier,IndexedTypeExpression>(null,null);
-        varPair.setFirst(new VarIdentifier(var.toString()));
-        paramList.add(varPair);
-    } (LOOKAHEAD(3)(<COLON>)? type = IndexedTypeExpr() {varPair.setSecond(type);})?)*)? ) <RIGHTPAREN>
-
-    {
-        return paramList;
-    }
-}
-
 CreateFunctionStatement FunctionSpecification(Token startStmtToken) throws ParseException:
 {
   FunctionSignature signature = null;
@@ -1083,7 +1062,7 @@
   String lang = null;
   String libName ="";
   String externalIdent = "";
-  List<Pair<VarIdentifier,IndexedTypeExpression>> args = new ArrayList<Pair<VarIdentifier,IndexedTypeExpression>>();
+  List<Pair<VarIdentifier,IndexedTypeExpression>> args = null;
   RecordConstructor resources = null;
 }
 {
@@ -1093,7 +1072,7 @@
   }
   ifNotExists = IfNotExists()
   args = FunctionParameters()
-  ( LOOKAHEAD({laIdentifier(RETURNS)}) <IDENTIFIER> returnType = IndexedTypeExpr() )?
+  returnType = FunctionReturnType()
   (
     (
       <LEFTBRACE>
@@ -1160,6 +1139,49 @@
   )
 }
 
+List<Pair<VarIdentifier,IndexedTypeExpression>> FunctionParameters() :
+{
+  List<Pair<VarIdentifier,IndexedTypeExpression>> paramList = Collections.<Pair<VarIdentifier,IndexedTypeExpression>>emptyList();
+}
+{
+  <LEFTPAREN> (paramList = FunctionParameterList())? <RIGHTPAREN>
+  {
+    return paramList;
+  }
+}
+
+List<Pair<VarIdentifier,IndexedTypeExpression>> FunctionParameterList() :
+{
+    List<Pair<VarIdentifier,IndexedTypeExpression>> paramList = new ArrayList<Pair<VarIdentifier,IndexedTypeExpression>>();
+    Pair<VarIdentifier,IndexedTypeExpression> varPair = new Pair<VarIdentifier,IndexedTypeExpression>(null,null);
+    String var = null;
+    IndexedTypeExpression type = null;
+}
+{
+    var = VariableIdentifier() {
+        varPair.setFirst(new VarIdentifier(var));
+        paramList.add(varPair);
+    } (LOOKAHEAD(3)(<COLON>)? type = IndexedTypeExpr() {varPair.setSecond(type);} )? (<COMMA> var = VariableIdentifier() {
+        varPair = new Pair<VarIdentifier,IndexedTypeExpression>(null,null);
+        varPair.setFirst(new VarIdentifier(var));
+        paramList.add(varPair);
+    } (LOOKAHEAD(3)(<COLON>)? type = IndexedTypeExpr() {varPair.setSecond(type);})?)*
+    {
+        return paramList;
+    }
+}
+
+IndexedTypeExpression FunctionReturnType() throws ParseException:
+{
+  IndexedTypeExpression returnType = null;
+}
+{
+  ( LOOKAHEAD({laIdentifier(RETURNS)}) <IDENTIFIER> returnType = IndexedTypeExpr() )?
+  {
+    return returnType;
+  }
+}
+
 Expression FunctionBody() throws ParseException:
 {
   Expression functionBodyExpr = null;
@@ -2373,7 +2395,7 @@
 {
   Token startToken = null;
   String functionName;
-  List<Pair<VarIdentifier,IndexedTypeExpression>> paramList = new ArrayList<Pair<VarIdentifier,IndexedTypeExpression>>();
+  List<Pair<VarIdentifier,IndexedTypeExpression>> paramList = Collections.<Pair<VarIdentifier,IndexedTypeExpression>>emptyList();;
   List<VarIdentifier> paramVarOnly = new ArrayList<VarIdentifier>();
   Expression funcBody;
   createNewScope();
@@ -2383,7 +2405,7 @@
     functionName = Identifier()
     paramList = FunctionParameters()
   <LEFTBRACE>
-    (funcBody = SelectExpression(true) | funcBody = Expression())
+    funcBody = FunctionBody()
   <RIGHTBRACE>
   {
     FunctionSignature signature = new FunctionSignature(defaultDataverse, functionName, paramList.size());
@@ -3039,9 +3061,8 @@
     String id = null;
 }
 {
-    (<IDENTIFIER> { id = token.image; } | id = QuotedString())
+    id = VariableIdentifier()
     {
-     id = SqlppVariableUtil.toInternalVariableName(id); // Prefix user-defined variables with "$"
      Identifier ident = lookupSymbol(id);
      if (isInForbiddenScopes(id)) {
        throw new SqlppParseException(getSourceLocation(token),
@@ -3063,9 +3084,8 @@
     String id = null;
 }
 {
-    (<IDENTIFIER> { id = token.image; } | id = QuotedString())
+    id = VariableIdentifier()
     {
-     id = SqlppVariableUtil.toInternalVariableName(id); // prefix user-defined variables with "$".
      Identifier ident = lookupSymbol(id);
      VariableExpr varExp = new VariableExpr(new VarIdentifier(id));
      if (ident != null) { // exist such ident
@@ -3075,6 +3095,17 @@
     }
 }
 
+String VariableIdentifier() throws ParseException:
+{
+  String id = null;
+}
+{
+  (<IDENTIFIER> { id = token.image; } | id = QuotedString())
+  {
+    return SqlppVariableUtil.toInternalVariableName(id); // prefix user-defined variables with "$".
+  }
+}
+
 Pair<VariableExpr, List<Pair<Expression, Identifier>>> VariableWithFieldMap() throws ParseException:
 {
   VariableExpr var = null;