Code review updates for grandinj

h2/src/main/org/h2/command/Parser.java

@@ -14,6 +14,7 @@ import java.nio.charset.Charset;
 import java.text.Collator;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.HashSet;
@@ -700,7 +701,26 @@ public class Parser {
     private Schema getSchema() {
         return getSchema(schemaName);
     }
-    
+    /*
+     * Gets the current schema for scenarios that need a guranteed, non-null schema object. 
+     * 
+     * This routine is solely here
+     * because of the function readIdentifierWithSchema(String defaultSchemaName) - which 
+     * is often called with a null parameter (defaultSchemaName) - then 6 lines into the function
+     * that routine nullifies the state field schemaName - which I believe is a bug.
+     * 
+     * There are about 7 places where "readIdentifierWithSchema(null)" is called in this file.
+     * 
+     * In other words when is it legal to not have an active schema defined by schemaName ?
+     * I don't think it's ever a valid case. I don't understand when that would be allowed.
+     * I spent a long time trying to figure this out. 
+     * As another proof of this point, the command "SET SCHEMA=NULL" is not a valid command.
+     * 
+     * I did try to fix this in readIdentifierWithSchema(String defaultSchemaName) 
+     * - but every fix I tried cascaded so many unit test errors - so
+     * I gave up. I think this needs a bigger effort to fix his, as part of bigger, dedicated story.
+     * 
+     */
     private Schema getSchemaWithDefault() {
         if(schemaName==null){
             schemaName = session.getCurrentSchemaName();
@@ -825,15 +845,13 @@ public class Parser {
                 currentSelect, orderInFrom, null);
     }
 
-    private TableFilter readSimpleTableFilterWithAliasExcludes(int orderInFrom,List<String> excludeTokens) {
+    private TableFilter readSimpleTableFilterWithAliasExcludes(int orderInFrom,Collection<String> excludeTokens) {
         Table table = readTableOrView();
         String alias = null;
         if (readIf("AS")) {
             alias = readAliasIdentifier();
         } else if (currentTokenType == IDENTIFIER) {
-            String upperCaseCurrentToken = currentToken.toUpperCase();
-            if (!equalsToken("SET", upperCaseCurrentToken) && !excludeTokens.contains(upperCaseCurrentToken)) {
-                System.out.println("currentToken="+currentToken);
+            if (!equalsTokenIgnoreCase(currentToken,"SET") && !isTokenInList(excludeTokens)) {
                 // SET is not a keyword (PostgreSQL supports it as a table name)
                 alias = readAliasIdentifier();
             }
@@ -1145,7 +1163,7 @@ public class Parser {
         command.setOnCondition(condition);
         read(")");
         
-        if(readIfAll(Arrays.asList("WHEN","MATCHED","THEN"))){
+        if(readIfAll("WHEN","MATCHED","THEN")){
             int startMatched = lastParseIndex;
             if (readIf("UPDATE")){
                 Update updateCommand = new Update(session);
@@ -1164,7 +1182,7 @@ public class Parser {
                 command.setDeleteCommand(deleteCommand);
             }            
         }
-        if(readIfAll(Arrays.asList("WHEN","NOT","MATCHED","THEN"))){
+        if(readIfAll("WHEN","NOT","MATCHED","THEN")){
             if (readIf("INSERT")){
                 Insert insertCommand = new Insert(session);
                 insertCommand.setTable(command.getTargetTable());
@@ -1442,7 +1460,7 @@ public class Parser {
     private String readFromAlias(String alias, List<String> excludeIdentifiers) {
         if (readIf("AS")) {
             alias = readAliasIdentifier();
-        } else if (currentTokenType == IDENTIFIER && !excludeIdentifiers.contains(currentToken)) {
+        } else if (currentTokenType == IDENTIFIER && !isTokenInList(excludeIdentifiers)) {
                 alias = readAliasIdentifier();
         }
         return alias;
@@ -3452,16 +3470,15 @@ public class Parser {
      * Reads every token in list, in order - returns true if all are found.
      * If any are not found, returns false - AND resets parsing back to state when called.
      */
-    private boolean readIfAll(List<String> tokens) {
+    private boolean readIfAll(String ... tokens) {
         // save parse location in case we have to fail this test
         int start = lastParseIndex;
         for(String token: tokens){
-            String currentTokenUpper = currentToken.toUpperCase();
-            if (!currentTokenQuoted && equalsToken(token, currentTokenUpper)) {
+            if (!currentTokenQuoted && equalsToken(token, currentToken)) {
                 read();
             }
             else{
-                // revert parse location to when called
+                // read failed - revert parse location to before when called
                 parseIndex = start;
                 read();
                 return false;
@@ -3491,6 +3508,22 @@ public class Parser {
         return false;
     }
     
+    private boolean equalsTokenIgnoreCase(String a, String b) {
+        if (a == null) {
+            return b == null;
+        } else if (a.equals(b)) {
+            return true;
+        } else if (a.equalsIgnoreCase(b)) {
+            return true;
+        }
+        return false;
+    }
+    
+    private boolean isTokenInList(Collection<String> upperCaseTokenList){
+        String upperCaseCurrentToken = currentToken.toUpperCase();
+        return upperCaseTokenList.contains(upperCaseCurrentToken);        
+    }
+
     private void addExpected(String token) {
         if (expectedList != null) {
             expectedList.add(token);
@@ -5195,14 +5228,23 @@ public class Parser {
         return view;
     }
 
-    private List<Column> createQueryColumnTemplateList(String[] cols, Query withQuery, String[] querySQLOutput) {
+    /**
+     * Creates a list of column templates from a query (usually from WITH query, but could be any query)
+     * @param cols - an optional list of column names (can be specified by WITH clause overriding usual select names)
+     * @param theQuery - the query object we want the column list for
+     * @param querySQLOutput - array of length 1 to receive extra 'output' field in addition to return value 
+     *                  - containing the SQL query of the Query object
+     * @return a list of column object returned by withQuery
+     */
+    private List<Column> createQueryColumnTemplateList(String[] cols, Query theQuery, String[] querySQLOutput) {
         List<Column> columnTemplateList = new ArrayList<Column>();
-        withQuery.prepare();
-        querySQLOutput[0] = StringUtils.cache(withQuery.getPlanSQL());
-        ArrayList<Expression> withExpressions = withQuery.getExpressions();
+        theQuery.prepare();
+        // array of length 1 to receive extra 'output' field in addition to return value
+        querySQLOutput[0] = StringUtils.cache(theQuery.getPlanSQL());
+        ArrayList<Expression> withExpressions = theQuery.getExpressions();
         for (int i = 0; i < withExpressions.size(); ++i) {
             Expression columnExp = withExpressions.get(i);
-            // use the passed in column name if supplied, otherwise use alias (if used) otherwise use column name
+            // use the passed in column name if supplied, otherwise use alias (if found) otherwise use column name
             // derived from column expression
             String columnName;
             if (cols != null){
@@ -5228,7 +5270,7 @@ public class Parser {
         // then we just compile it again.
         TableView view = new TableView(schema, id, tempViewName, querySQL,
                 parameters, columnTemplateList.toArray(new Column[0]), session,
-                allowRecursiveQueryDetection/* recursive */, false);
+                allowRecursiveQueryDetection, false);
         if (!view.isRecursiveQueryDetected() && allowRecursiveQueryDetection) {
             view = new TableView(schema, id, tempViewName, querySQL, parameters,
                     columnTemplateList.toArray(new Column[0]), session,
@@ -5236,10 +5278,11 @@ public class Parser {
         }
         view.setTableExpression(true);
         view.setTemporary(true);
+        view.setHidden(true);
         if(addViewToSession){
             session.addLocalTempTable(view);
         }
-        view.setOnCommitDrop(true);
+        view.setOnCommitDrop(false);
         return view;
     }
 

h2/src/main/org/h2/command/dml/Delete.java

@@ -36,6 +36,9 @@ public class Delete extends Prepared {
      * The limit expression as specified in the LIMIT or TOP clause.
      */
     private Expression limitExpr;
+    /**
+     * This table filter is for MERGE..USING support - not used in stand-alone DML
+     */
     private TableFilter sourceTableFilter;
 
     public Delete(Session session) {

h2/src/main/org/h2/command/dml/Insert.java

@@ -47,6 +47,9 @@ public class Insert extends Prepared implements ResultTarget {
     private boolean sortedInsertMode;
     private int rowNumber;
     private boolean insertFromSelect;
+    /**
+     * This table filter is for MERGE..USING support - not used in stand-alone DML
+     */    
     private TableFilter sourceTableFilter;
 
     /**

h2/src/main/org/h2/command/dml/MergeUsing.java

@@ -33,6 +33,68 @@ import org.h2.value.Value;
 /**
  * This class represents the statement syntax
  * MERGE table alias USING...
+ * 
+ * It does not replace the existing MERGE INTO... KEYS... form.
+ * 
+ * It supports the SQL 2003/2008 standard MERGE  statement: 
+ *      http://en.wikipedia.org/wiki/Merge_%28SQL%29
+ * 
+ * Database management systems Oracle Database, DB2, Teradata, EXASOL, Firebird, CUBRID, HSQLDB, 
+ * MS SQL, Vectorwise and Apache Derby & Postgres support the standard syntax of the 
+ * SQL 2003/2008 MERGE command:
+ * 
+ *  MERGE INTO targetTable AS T USING sourceTable AS S ON (T.ID = S.ID)
+ *    WHEN MATCHED THEN
+ *        UPDATE SET column1 = value1 [, column2 = value2 ...] WHERE column1=valueUpdate
+ *        DELETE WHERE column1=valueDelete
+ *    WHEN NOT MATCHED THEN
+ *        INSERT (column1 [, column2 ...]) VALUES (value1 [, value2 ...]);
+ *        
+ * Only Oracle support the additional optional DELETE clause.
+ * 
+ * Implementation notes:
+ * 
+ * 1) The ON clause must specify 1 or more columns from the TARGET table because they are
+ *    used in the plan SQL WHERE statement. Otherwise an exception is raised.
+ *  
+ * 2) The ON clause must specify 1 or more columns from the SOURCE table/query because they
+ *    are used to track the join key values for every source table row - to prevent any
+ *    TARGET rows from being updated twice per MERGE USING statement. 
+ * 
+ *    This is to implement a requirement from the MERGE INTO specification
+ *    requiring each row from being updated more than once per MERGE USING statement. 
+ *    The source columns are used to gather the effective "key" values which have been 
+ *    updated, in order to implement this requirement. 
+ *    If the no SOURCE table/query columns are found in the ON clause, then an exception is 
+ *    raised.
+ *    
+ *    The update row counts of the embedded UPDATE and DELETE statements are also tracked to 
+ *    ensure no more than 1 row is ever updated. (Note One special case of this is that
+ *    the DELETE is allowed to affect the same row which was updated by UPDATE - this is an
+ *    Oracle only extension.) 
+ *    
+ * 3) UPDATE and DELETE statements are allowed to specify extra conditional criteria 
+ *    (in the WHERE clause) to allow fine-grained control of actions when a record is found.
+ *    The ON clause conditions are always prepended to the WHERE clause of these embedded 
+ *    statements, so they will never update more than the ON join condition.
+ *    
+ * POTENTIAL ISSUES
+ * 
+ * 1) If neither UPDATE or DELETE clause is supplied, but INSERT is supplied - the INSERT 
+ *    action is always triggered. This is because the embedded UPDATE and DELETE statement's 
+ *    returned update row count is used to detect a matching join. 
+ *    If neither of the two the statements are provided, no matching join is EVER detected. 
+ *    A fix for this is to generate a "matchSelect" query and use that to always detect 
+ *    a match join - rather than relying on UPDATE or DELETE statements. 
+ *    
+ *    This would be an improvement, especially in the case that if either of the
+ *    UPDATE or DELETE statements had their own fine-grained WHERE conditions, making 
+ *    them completely different conditions than the plain ON condition clause which
+ *    the SQL author would be specifying/expecting.
+ *    
+ *    An additional benefit of this solution would also be that the this "matchSelect" query
+ *    could be used to return the ROWID of the found (or inserted) query - for more accurate 
+ *    enforcing of the only-update-each-target-row-once rule. 
  */
 public class MergeUsing extends Prepared {
     
@@ -299,20 +361,30 @@ public class MergeUsing extends Prepared {
             }
         }
         
+        int embeddedStatementsCount = 0;
+        
         // Prepare each of the sub-commands ready to aid in the MERGE collaboration
         if(updateCommand!=null){
             updateCommand.setSourceTableFilter(sourceTableFilter);
             updateCommand.setCondition(appendOnCondition(updateCommand));
             updateCommand.prepare();
+            embeddedStatementsCount++;
         }
         if(deleteCommand!=null){
             deleteCommand.setSourceTableFilter(sourceTableFilter);
             deleteCommand.setCondition(appendOnCondition(deleteCommand));            
             deleteCommand.prepare();
+            embeddedStatementsCount++;
         }        
         if(insertCommand!=null){
             insertCommand.setSourceTableFilter(sourceTableFilter);
             insertCommand.prepare();
+            embeddedStatementsCount++;
+        }        
+        
+        if(embeddedStatementsCount==0){
+            throw DbException.get(ErrorCode.SYNTAX_ERROR_1,
+                    "At least UPDATE, DELETE or INSERT embedded statement must be supplied.");
         }
     }
 

h2/src/main/org/h2/command/dml/Update.java

@@ -39,7 +39,10 @@ public class Update extends Prepared {
 
     private Expression condition;
     private TableFilter targetTableFilter;// target of update
-    private TableFilter sourceTableFilter;// optional source query
+    /**
+     * This table filter is for MERGE..USING support - not used in stand-alone DML
+     */    
+    private TableFilter sourceTableFilter;
 
     /** The limit expression as specified in the LIMIT clause. */
     private Expression limitExpr;

h2/src/main/org/h2/jdbc/JdbcConnection.java

@@ -103,6 +103,7 @@ public class JdbcConnection extends TraceObject implements Connection,
     /**
      * INTERNAL
      */
+   @SuppressWarnings("resource")// the session closable object does not leak as Eclipse warns - due to the CloseWatcher
    public JdbcConnection(ConnectionInfo ci, boolean useBaseDir)
             throws SQLException {
         try {

h2/src/test/org/h2/test/db/TestMergeUsing.java

@@ -124,13 +124,14 @@ public class TestMergeUsing extends TestBase {
                 3
                 );
         // no insert, no update, no delete clauses - essentially a no-op
-        testMergeUsing(
+        testMergeUsingException(
                 "CREATE TABLE PARENT AS (SELECT X AS ID, 'Marcy'||X AS NAME FROM SYSTEM_RANGE(1,1) );"+
                         "DELETE FROM PARENT;",
                 "MERGE INTO PARENT AS P USING (SELECT X AS ID, 'Marcy'||X AS NAME FROM SYSTEM_RANGE(1,3) ) AS S ON (P.ID = S.ID)",
                 GATHER_ORDERED_RESULTS_SQL,
                 "SELECT X AS ID, 'Marcy'||X AS NAME FROM SYSTEM_RANGE(1,3) WHERE X<0",
-                0
+                0,
+                "At least UPDATE, DELETE or INSERT embedded statement must be supplied."
                 );
         // Two updates to same row - update and delete together - emptying the parent table
         testMergeUsing(