Issue 238: Can drop a column that has a single-column constraint.

h2/src/docsrc/html/changelog.html

@@ -18,7 +18,7 @@ Change Log
 <h1>Change Log</h1>
 
 <h2>Next Version (unreleased)</h2>
-<ul><li>-
+<ul><li>Issue 238: Can drop a column that has a single-column constraint.
 </li></ul>
 
 <h2>Version 1.3.150 Beta (2011-01-28)</h2>
@@ -26,7 +26,7 @@ Change Log
     all options can be combined into a space separated key-value pairs.
 </li><li>CSVREAD / CSV tool: there is a new option "lineCommentCharacter" to set or disable line comments.
     For H2 version 1.2, the default is '#' (as before). For H2 version 1.3, line comments are disabled by default.
-</li><li>Issue 277:  JaQu didn't correctly convert a CLOB column to a String.
+</li><li>Issue 277: JaQu didn't correctly convert a CLOB column to a String.
 </li><li>./build.sh testNetwork could block if there was a network configuration problem.
 </li><li>Reading input streams from the classpath is now supported. Example:
     RUNSCRIPT FROM 'classpath:org/h2/samples/newsfeed.sql'.

h2/src/main/org/h2/command/ddl/AlterTableAlterColumn.java

@@ -126,8 +126,7 @@ public class AlterTableAlterColumn extends SchemaCommand {
             if (table.getColumns().length == 1) {
                 throw DbException.get(ErrorCode.CANNOT_DROP_LAST_COLUMN, oldColumn.getSQL());
             }
-            table.checkColumnIsNotReferenced(oldColumn);
-            dropSingleColumnIndexes();
+            table.dropSingleColumnConstraintsAndIndexes(session, oldColumn);
             copyData();
             break;
         }
@@ -389,33 +388,6 @@ public class AlterTableAlterColumn extends SchemaCommand {
         }
     }
 
-    private void dropSingleColumnIndexes() {
-        Database db = session.getDatabase();
-        ArrayList<Index> indexes = table.getIndexes();
-        for (int i = 0; i < indexes.size(); i++) {
-            Index index = indexes.get(i);
-            if (index.getCreateSQL() == null) {
-                continue;
-            }
-            boolean dropIndex = false;
-            Column[] cols = index.getColumns();
-            for (Column c : cols) {
-                if (c == oldColumn) {
-                    if (cols.length == 1) {
-                        dropIndex = true;
-                    } else {
-                        throw DbException.get(ErrorCode.COLUMN_IS_PART_OF_INDEX_1, index.getSQL());
-                    }
-                }
-            }
-            if (dropIndex) {
-                db.removeSchemaObject(session, index);
-                indexes = table.getIndexes();
-                i = -1;
-            }
-        }
-    }
-
     private void checkNullable() {
         for (Index index : table.getIndexes()) {
             if (index.getColumnIndex(oldColumn) < 0) {

h2/src/main/org/h2/constant/ErrorCode.java

@@ -1255,7 +1255,7 @@ public class ErrorCode {
      * ALTER TABLE TEST DROP COLUMN PID;
      * </pre>
      */
-    public static final int COLUMN_MAY_BE_REFERENCED_1 = 90083;
+    public static final int COLUMN_IS_REFERENCED_1 = 90083;
 
     /**
      * The error with code <code>90084</code> is thrown when

h2/src/main/org/h2/constraint/Constraint.java

@@ -6,6 +6,7 @@
  */
 package org.h2.constraint;
 
+import java.util.HashSet;
 import org.h2.engine.DbObject;
 import org.h2.engine.Session;
 import org.h2.index.Index;
@@ -87,12 +88,12 @@ public abstract class Constraint extends SchemaObjectBase implements Comparable<
     public abstract void setIndexOwner(Index index);
 
     /**
-     * Check if this constraint contains the given column.
+     * Get all referenced columns.
      *
-     * @param col the column
-     * @return true if it does
+     * @param table the table
+     * @return the set of referenced columns
      */
-    public abstract boolean containsColumn(Column col);
+    public abstract HashSet<Column> getReferencedColumns(Table table);
 
     /**
      * Get the SQL statement to create this constraint.

h2/src/main/org/h2/constraint/ConstraintCheck.java

@@ -6,9 +6,12 @@
  */
 package org.h2.constraint;
 
+import java.util.HashSet;
+import java.util.Iterator;
 import org.h2.constant.ErrorCode;
 import org.h2.engine.Session;
 import org.h2.expression.Expression;
+import org.h2.expression.ExpressionVisitor;
 import org.h2.index.Index;
 import org.h2.message.DbException;
 import org.h2.result.ResultInterface;
@@ -17,6 +20,7 @@ import org.h2.schema.Schema;
 import org.h2.table.Column;
 import org.h2.table.Table;
 import org.h2.table.TableFilter;
+import org.h2.util.New;
 import org.h2.util.StringUtils;
 
 /**
@@ -97,12 +101,15 @@ public class ConstraintCheck extends Constraint {
         DbException.throwInternalError();
     }
 
-    public boolean containsColumn(Column col) {
-        // TODO check constraints / containsColumn: this is cheating, maybe the
-        // column is not referenced
-        String s = col.getSQL();
-        String sql = getCreateSQL();
-        return sql.indexOf(s) >= 0;
+    public HashSet<Column> getReferencedColumns(Table table) {
+        HashSet<Column> columns = New.hashSet();
+        expr.isEverything(ExpressionVisitor.getColumnsVisitor(columns));
+        for (Iterator<Column> it = columns.iterator(); it.hasNext();) {
+            if (it.next().getTable() != table) {
+                it.remove();
+            }
+        }
+        return columns;
     }
 
     public Expression getExpression() {

h2/src/main/org/h2/constraint/ConstraintReferential.java

@@ -7,6 +7,7 @@
 package org.h2.constraint;
 
 import java.util.ArrayList;
+import java.util.HashSet;
 import org.h2.command.Parser;
 import org.h2.command.Prepared;
 import org.h2.constant.ErrorCode;
@@ -23,6 +24,7 @@ import org.h2.schema.Schema;
 import org.h2.table.Column;
 import org.h2.table.IndexColumn;
 import org.h2.table.Table;
+import org.h2.util.New;
 import org.h2.util.StatementBuilder;
 import org.h2.util.StringUtils;
 import org.h2.value.Value;
@@ -199,6 +201,20 @@ public class ConstraintReferential extends Constraint {
         return columns;
     }
 
+    public HashSet<Column> getReferencedColumns(Table table) {
+        HashSet<Column> result = New.hashSet();
+        if (table == this.table) {
+            for (IndexColumn c : columns) {
+                result.add(c.column);
+            }
+        } else if (table == this.refTable) {
+            for (IndexColumn c : refColumns) {
+                result.add(c.column);
+            }
+        }
+        return result;
+    }
+
     public void setRefColumns(IndexColumn[] refCols) {
         refColumns = refCols;
     }

h2/src/main/org/h2/constraint/ConstraintUnique.java

@@ -6,6 +6,7 @@
  */
 package org.h2.constraint;
 
+import java.util.HashSet;
 import org.h2.command.Parser;
 import org.h2.engine.Session;
 import org.h2.index.Index;
@@ -14,6 +15,7 @@ import org.h2.schema.Schema;
 import org.h2.table.Column;
 import org.h2.table.IndexColumn;
 import org.h2.table.Table;
+import org.h2.util.New;
 import org.h2.util.StatementBuilder;
 import org.h2.util.StringUtils;
 
@@ -121,13 +123,12 @@ public class ConstraintUnique extends Constraint {
         indexOwner = true;
     }
 
-    public boolean containsColumn(Column col) {
+    public HashSet<Column> getReferencedColumns(Table table) {
+        HashSet<Column> result = New.hashSet();
         for (IndexColumn c : columns) {
-            if (c.column == col) {
-                return true;
-            }
+            result.add(c.column);
         }
-        return false;
+        return result;
     }
 
     public boolean isBefore() {

h2/src/main/org/h2/expression/ExpressionColumn.java

@@ -260,6 +260,9 @@ public class ExpressionColumn extends Expression {
         case ExpressionVisitor.GET_DEPENDENCIES:
             visitor.addDependency(column.getTable());
             return true;
+        case ExpressionVisitor.GET_COLUMNS:
+            visitor.addColumn(column);
+            return true;
         default:
             throw DbException.throwInternalError("type=" + visitor.getType());
         }

h2/src/main/org/h2/expression/ExpressionVisitor.java

@@ -8,6 +8,7 @@ package org.h2.expression;
 
 import java.util.HashSet;
 import org.h2.engine.DbObject;
+import org.h2.table.Column;
 import org.h2.table.ColumnResolver;
 import org.h2.table.Table;
 
@@ -90,6 +91,11 @@ public class ExpressionVisitor {
      */
     public static final int QUERY_COMPARABLE = 8;
 
+    /**
+     * Get all referenced columns.
+     */
+    public static final int GET_COLUMNS = 9;
+
     /**
      * The visitor singleton for the type QUERY_COMPARABLE.
      */
@@ -98,14 +104,16 @@ public class ExpressionVisitor {
     private final int type;
     private final int queryLevel;
     private final HashSet<DbObject> dependencies;
+    private final HashSet<Column> columns;
     private final Table table;
     private final long[] maxDataModificationId;
     private final ColumnResolver resolver;
 
-    private ExpressionVisitor(int type, int queryLevel, HashSet<DbObject> dependencies, Table table, ColumnResolver resolver, long[] maxDataModificationId) {
+    private ExpressionVisitor(int type, int queryLevel, HashSet<DbObject> dependencies, HashSet<Column> columns, Table table, ColumnResolver resolver, long[] maxDataModificationId) {
         this.type = type;
         this.queryLevel = queryLevel;
         this.dependencies = dependencies;
+        this.columns = columns;
         this.table = table;
         this.resolver = resolver;
         this.maxDataModificationId = maxDataModificationId;
@@ -115,6 +123,7 @@ public class ExpressionVisitor {
         this.type = type;
         this.queryLevel = 0;
         this.dependencies = null;
+        this.columns = null;
         this.table = null;
         this.resolver = null;
         this.maxDataModificationId = null;
@@ -127,7 +136,7 @@ public class ExpressionVisitor {
      * @return the new visitor
      */
     public static ExpressionVisitor getDependenciesVisitor(HashSet<DbObject> dependencies) {
-        return new ExpressionVisitor(GET_DEPENDENCIES, 0, dependencies, null, null, null);
+        return new ExpressionVisitor(GET_DEPENDENCIES, 0, dependencies, null, null, null, null);
     }
 
     /**
@@ -137,7 +146,7 @@ public class ExpressionVisitor {
      * @return the new visitor
      */
     public static ExpressionVisitor getOptimizableVisitor(Table table) {
-        return new ExpressionVisitor(OPTIMIZABLE_MIN_MAX_COUNT_ALL, 0, null, table, null, null);
+        return new ExpressionVisitor(OPTIMIZABLE_MIN_MAX_COUNT_ALL, 0, null, null, table, null, null);
     }
 
     /**
@@ -148,11 +157,21 @@ public class ExpressionVisitor {
      * @return the new visitor
      */
     public static ExpressionVisitor getNotFromResolverVisitor(ColumnResolver resolver) {
-        return new ExpressionVisitor(NOT_FROM_RESOLVER, 0, null, null, resolver, null);
+        return new ExpressionVisitor(NOT_FROM_RESOLVER, 0, null, null, null, resolver, null);
+    }
+
+    /**
+     * Create a new visitor to get all referenced columns.
+     *
+     * @param columns the columns map
+     * @return the new visitor
+     */
+    public static ExpressionVisitor getColumnsVisitor(HashSet<Column> columns) {
+        return new ExpressionVisitor(GET_COLUMNS, 0, null, columns, null, null, null);
     }
 
     public static ExpressionVisitor getMaxModificationIdVisitor() {
-        return new ExpressionVisitor(SET_MAX_DATA_MODIFICATION_ID, 0, null, null, null, new long[1]);
+        return new ExpressionVisitor(SET_MAX_DATA_MODIFICATION_ID, 0, null, null, null, null, new long[1]);
     }
 
     /**
@@ -165,6 +184,16 @@ public class ExpressionVisitor {
         dependencies.add(obj);
     }
 
+    /**
+     * Add a new column to the set of columns.
+     * This is used for GET_COLUMNS visitors.
+     *
+     * @param column the additional column.
+     */
+    public void addColumn(Column column) {
+        columns.add(column);
+    }
+
     /**
      * Get the dependency set.
      * This is used for GET_DEPENDENCIES visitors.
@@ -182,7 +211,7 @@ public class ExpressionVisitor {
      * @return a clone of this expression visitor, with the changed query level
      */
     public ExpressionVisitor incrementQueryLevel(int offset) {
-        return new ExpressionVisitor(type, queryLevel + offset, dependencies, table, resolver, maxDataModificationId);
+        return new ExpressionVisitor(type, queryLevel + offset, dependencies, columns, table, resolver, maxDataModificationId);
     }
 
     /**

h2/src/main/org/h2/expression/Function.java

@@ -1983,6 +1983,7 @@ public class Function extends Expression implements FunctionCall {
         case ExpressionVisitor.NOT_FROM_RESOLVER:
         case ExpressionVisitor.OPTIMIZABLE_MIN_MAX_COUNT_ALL:
         case ExpressionVisitor.SET_MAX_DATA_MODIFICATION_ID:
+        case ExpressionVisitor.GET_COLUMNS:
             return true;
         default:
             throw DbException.throwInternalError("type=" + visitor.getType());

h2/src/main/org/h2/expression/Parameter.java

@@ -136,6 +136,7 @@ public class Parameter extends Expression implements ParameterInterface {
         case ExpressionVisitor.OPTIMIZABLE_MIN_MAX_COUNT_ALL:
         case ExpressionVisitor.DETERMINISTIC:
         case ExpressionVisitor.READONLY:
+        case ExpressionVisitor.GET_COLUMNS:
             return true;
         case ExpressionVisitor.INDEPENDENT:
             return value != null;

h2/src/main/org/h2/expression/Rownum.java

@@ -77,6 +77,7 @@ public class Rownum extends Expression {
         case ExpressionVisitor.NOT_FROM_RESOLVER:
         case ExpressionVisitor.GET_DEPENDENCIES:
         case ExpressionVisitor.SET_MAX_DATA_MODIFICATION_ID:
+        case ExpressionVisitor.GET_COLUMNS:
             // if everything else is the same, the rownum is the same
             return true;
         default:

h2/src/main/org/h2/expression/SequenceValue.java

@@ -73,6 +73,7 @@ public class SequenceValue extends Expression {
         case ExpressionVisitor.EVALUATABLE:
         case ExpressionVisitor.OPTIMIZABLE_MIN_MAX_COUNT_ALL:
         case ExpressionVisitor.NOT_FROM_RESOLVER:
+        case ExpressionVisitor.GET_COLUMNS:
             return true;
         case ExpressionVisitor.DETERMINISTIC:
         case ExpressionVisitor.READONLY:

h2/src/main/org/h2/expression/ValueExpression.java

@@ -144,6 +144,7 @@ public class ValueExpression extends Expression {
         case ExpressionVisitor.NOT_FROM_RESOLVER:
         case ExpressionVisitor.GET_DEPENDENCIES:
         case ExpressionVisitor.QUERY_COMPARABLE:
+        case ExpressionVisitor.GET_COLUMNS:
             return true;
         default:
             throw DbException.throwInternalError("type=" + visitor.getType());

h2/src/main/org/h2/expression/Variable.java

@@ -67,6 +67,7 @@ public class Variable extends Expression {
         case ExpressionVisitor.NOT_FROM_RESOLVER:
         case ExpressionVisitor.QUERY_COMPARABLE:
         case ExpressionVisitor.GET_DEPENDENCIES:
+        case ExpressionVisitor.GET_COLUMNS:
             return true;
         case ExpressionVisitor.DETERMINISTIC:
             return false;

h2/src/main/org/h2/table/Table.java

@@ -464,36 +464,57 @@ public abstract class Table extends SchemaObjectBase {
     }
 
     /**
-     * Check that this column is not referenced by a referential constraint or
-     * multi-column index.
+     * Check that this column is not referenced by a multi-column constraint or
+     * multi-column index. If it is, an exception is thrown. Single-column
+     * references and indexes are dropped.
      *
      * @param col the column
-     * @throws SQLException if the column is referenced
+     * @throws SQLException if the column is referenced by multi-column
+     *             constraints or indexes
      */
-    public void checkColumnIsNotReferenced(Column col) {
+    public void dropSingleColumnConstraintsAndIndexes(Session session, Column col) {
+        ArrayList<Constraint> constraintsToDrop = New.arrayList();
         if (constraints != null) {
             for (int i = 0, size = constraints.size(); i < size; i++) {
                 Constraint constraint = constraints.get(i);
-                if (constraint.containsColumn(col)) {
-                    throw DbException.get(ErrorCode.COLUMN_MAY_BE_REFERENCED_1, constraint.getSQL());
+                HashSet<Column> columns = constraint.getReferencedColumns(this);
+                if (!columns.contains(col)) {
+                    continue;
+                }
+                if (columns.size() == 1) {
+                    constraintsToDrop.add(constraint);
+                } else {
+                    throw DbException.get(ErrorCode.COLUMN_IS_REFERENCED_1, constraint.getSQL());
                 }
             }
         }
+        ArrayList<Index> indexesToDrop = New.arrayList();
         ArrayList<Index> indexes = getIndexes();
         if (indexes != null) {
             for (int i = 0, size = indexes.size(); i < size; i++) {
                 Index index = indexes.get(i);
-                if (index.getColumns().length == 1) {
+                if (index.getCreateSQL() == null) {
                     continue;
                 }
-                if (index.getCreateSQL() == null) {
+                if (index.getColumnIndex(col) < 0) {
                     continue;
                 }
-                if (index.getColumnIndex(col) >= 0) {
-                    throw DbException.get(ErrorCode.COLUMN_MAY_BE_REFERENCED_1, index.getSQL());
+                if (index.getColumns().length == 1) {
+                    indexesToDrop.add(index);
+                } else {
+                    throw DbException.get(ErrorCode.COLUMN_IS_REFERENCED_1, index.getSQL());
                 }
             }
         }
+        for (Constraint c : constraintsToDrop) {
+            session.getDatabase().removeSchemaObject(session, c);
+        }
+        for (Index i : indexesToDrop) {
+            // the index may already have been dropped when dropping the constraint
+            if (getIndexes().contains(i)) {
+                session.getDatabase().removeSchemaObject(session, i);
+            }
+        }
     }
 
     public Row getTemplateRow() {

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

@@ -35,12 +35,63 @@ public class TestAlter extends TestBase {
         deleteDb("alter");
         conn = getConnection("alter");
         stat = conn.createStatement();
+        testAlterTableDropColumWithReferences();
         testAlterTableAlterColumn();
         testAlterTableDropIdentityColumn();
         conn.close();
         deleteDb("alter");
     }
 
+    private void testAlterTableDropColumWithReferences() throws SQLException {
+
+        stat.execute("create table parent(id int, b int)");
+        stat.execute("create table child(p int primary key)");
+        stat.execute("alter table child add foreign key(p) references parent(id)");
+        stat.execute("alter table parent drop column id");
+        stat.execute("drop table parent");
+        stat.execute("drop table child");
+
+        stat.execute("create table test(id int, name varchar(255))");
+        stat.execute("alter table test add constraint x check (id > name)");
+        try {
+            // the constraint references multiple columns
+            stat.execute("alter table test drop column id");
+            fail();
+        } catch (SQLException e) {
+            assertEquals(ErrorCode.COLUMN_IS_REFERENCED_1, e.getErrorCode());
+        }
+        stat.execute("drop table test");
+
+        stat.execute("create table test(id int, name varchar(255))");
+        stat.execute("alter table test add constraint x unique(id, name)");
+        try {
+            // the constraint references multiple columns
+            stat.execute("alter table test drop column id");
+            fail();
+        } catch (SQLException e) {
+            assertEquals(ErrorCode.COLUMN_IS_REFERENCED_1, e.getErrorCode());
+        }
+        stat.execute("drop table test");
+
+        stat.execute("create table test(id int, name varchar(255))");
+        stat.execute("alter table test add constraint x check (id > 1)");
+        stat.execute("alter table test drop column id");
+        stat.execute("drop table test");
+
+        stat.execute("create table test(id int, name varchar(255))");
+        stat.execute("alter table test add constraint x check (name > 'TEST.ID')");
+        // previous versions of H2 used sql.indexOf(columnName)
+        // to check if the column is referenced
+        stat.execute("alter table test drop column id");
+        stat.execute("drop table test");
+
+        stat.execute("create table test(id int, name varchar(255))");
+        stat.execute("alter table test add constraint x unique(id)");
+        stat.execute("alter table test drop column id");
+        stat.execute("drop table test");
+
+    }
+
     private void testAlterTableDropIdentityColumn() throws SQLException {
         stat.execute("create table test(id int auto_increment, name varchar)");
         stat.execute("alter table test drop column id");

h2/src/test/org/h2/test/test-1.3.txt

@@ -4426,15 +4426,15 @@ ALTER TABLE TEST ALTER COLUMN PARENTID VARCHAR;
 > ok
 
 ALTER TABLE TEST DROP COLUMN PARENTID;
-> exception
+> ok
 
 ALTER TABLE TEST DROP COLUMN CHILD_ID;
 > ok
 
 SELECT * FROM TEST;
-> ID PARENTID
-> -- --------
-> 0  0
+> ID
+> --
+> 0
 > rows: 1
 
 DROP TABLE TEST;