diff --git a/phoenix-core/src/it/java/org/apache/phoenix/end2end/CreateTableIT.java b/phoenix-core/src/it/java/org/apache/phoenix/end2end/CreateTableIT.java index 3a79b65cc3a..b69c02b9bfe 100644 --- a/phoenix-core/src/it/java/org/apache/phoenix/end2end/CreateTableIT.java +++ b/phoenix-core/src/it/java/org/apache/phoenix/end2end/CreateTableIT.java @@ -52,6 +52,7 @@ import org.apache.phoenix.query.KeyRange; import org.apache.phoenix.query.QueryServices; import org.apache.phoenix.query.QueryServicesOptions; +import org.apache.phoenix.schema.ColumnAlreadyExistsException; import org.apache.phoenix.schema.PTable; import org.apache.phoenix.schema.PTable.ImmutableStorageScheme; import org.apache.phoenix.schema.PTable.QualifierEncodingScheme; @@ -90,6 +91,136 @@ public void testStartKeyStopKey() throws SQLException { assertTrue(splits.size() > 0); } + @Test + public void testCreateAlterTableWithDuplicateColumn() throws Exception { + Properties props = new Properties(); + int failureCount = 0; + int expectedExecCount = 0; + String tableName = generateUniqueName(); + String viewName = generateUniqueName(); + try (Connection conn = DriverManager.getConnection(getUrl(), props)) { + try { + // Case 1 - Adding a column as "Default_CF.Column" in + // CREATE TABLE query where "Column" is same as single PK Column + conn.createStatement().execute(String.format("CREATE TABLE %s" + + " (name VARCHAR NOT NULL PRIMARY KEY, city VARCHAR," + + " name1 VARCHAR, name VARCHAR)", tableName)); + fail("Should have failed with ColumnAlreadyExistsException"); + } catch (ColumnAlreadyExistsException e) { + // expected + failureCount++; + } + try { + // Case 2 - Adding a column as "Default_CF.Column" in CREATE + // TABLE query where "Default_CF.Column" already exists as non-Pk column + conn.createStatement().execute(String.format("CREATE TABLE %s" + + " (name VARCHAR NOT NULL PRIMARY KEY, city VARCHAR," + + " name1 VARCHAR, name1 VARCHAR)", tableName)); + fail("Should have failed with ColumnAlreadyExistsException"); + } catch (ColumnAlreadyExistsException e) { + // expected + failureCount++; + } + try { + // Case 3 - Adding a column as "Default_CF.Column" in CREATE + // TABLE query where "Column" is same as single PK Column. + // The only diff from Case 1 is that both PK Column and + // "Default_CF.Column" are of different DataType + conn.createStatement().execute(String.format("CREATE TABLE %s" + + " (key1 VARCHAR NOT NULL PRIMARY KEY, city VARCHAR," + + " name1 VARCHAR, key1 INTEGER)", tableName)); + fail("Should have failed with ColumnAlreadyExistsException"); + } catch (ColumnAlreadyExistsException e) { + // expected + failureCount++; + } + try { + // Case 4 - Adding a column as "Default_CF.Column" in + // CREATE TABLE query where "Column" is same as one of the + // PK Column from Composite PK Columns + conn.createStatement().execute(String.format("CREATE TABLE %s" + + " (name VARCHAR NOT NULL, name1 VARCHAR NOT NULL," + + " name2 VARCHAR, name VARCHAR" + + " CONSTRAINT PK PRIMARY KEY(name, name1))", tableName)); + fail("Should have failed with ColumnAlreadyExistsException"); + } catch (ColumnAlreadyExistsException e) { + // expected + failureCount++; + } + try { + // Case 5 - Adding a column as "Default_CF.Column" in + // CREATE VIEW query where "Column" is same as one of the + // PK Column from Composite PK Columns derived from parent table + conn.createStatement().execute(String.format("CREATE TABLE %s" + + " (name VARCHAR NOT NULL, name1 VARCHAR NOT NULL," + + " name2 VARCHAR, name3 VARCHAR" + + " CONSTRAINT PK PRIMARY KEY(name, name1))", tableName)); + expectedExecCount++; + conn.createStatement().execute(String.format("CREATE VIEW %s" + + " (name1 CHAR(5)) AS SELECT * FROM %s", viewName, tableName)); + fail("Should have failed with ColumnAlreadyExistsException"); + } catch (ColumnAlreadyExistsException e) { + // expected + failureCount++; + } + try { + // Case 6 - Adding a column as "Default_CF.Column" in + // ALTER TABLE query where "Column" is same as one of the + // PK Column from Composite PK Columns + conn.createStatement().execute( + String.format("DROP TABLE %s", tableName)); + conn.createStatement().execute(String.format("CREATE TABLE %s" + + " (name VARCHAR NOT NULL, name1 VARCHAR NOT NULL," + + " name2 VARCHAR, name3 VARCHAR" + + " CONSTRAINT PK PRIMARY KEY(name, name1))", tableName)); + expectedExecCount++; + conn.createStatement().execute( + String.format("ALTER TABLE %s ADD name1 INTEGER", tableName)); + fail("Should have failed with ColumnAlreadyExistsException"); + } catch (ColumnAlreadyExistsException e) { + // expected + failureCount++; + } + try { + // Case 7 - Adding a column as "Default_CF.Column" in + // ALTER TABLE query where "Column" is same as single PK Column + conn.createStatement().execute( + String.format("DROP TABLE %s", tableName)); + conn.createStatement().execute(String.format("CREATE TABLE %s" + + " (name VARCHAR NOT NULL PRIMARY KEY, city VARCHAR," + + " name1 VARCHAR, name2 VARCHAR)", tableName)); + expectedExecCount++; + conn.createStatement().execute( + String.format("ALTER TABLE %s ADD name VARCHAR", tableName)); + fail("Should have failed with ColumnAlreadyExistsException"); + } catch (ColumnAlreadyExistsException e) { + // expected + failureCount++; + } + assertEquals(7, failureCount); + assertEquals(3, expectedExecCount); + + conn.createStatement().execute( + String.format("DROP TABLE %s", tableName)); + // Case 8 - Adding a column as "Non_Default_CF.Column" in + // CREATE TABLE query where "Column" is same as single PK Column + // Hence, we allow creating such column + conn.createStatement().execute(String.format("CREATE TABLE %s" + + " (name VARCHAR NOT NULL PRIMARY KEY, city VARCHAR," + + " name1 VARCHAR, a.name VARCHAR)", tableName)); + conn.createStatement().execute( + String.format("DROP TABLE %s", tableName)); + // Case 9 - Adding a column as "Non_Default_CF.Column" in + // ALTER TABLE query where "Column" is same as single PK Column + // Hence, we allow creating such column + conn.createStatement().execute(String.format("CREATE TABLE %s" + + " (name VARCHAR NOT NULL PRIMARY KEY, city VARCHAR," + + " name1 VARCHAR, name2 VARCHAR)", tableName)); + conn.createStatement().execute( + String.format("ALTER TABLE %s ADD a.name VARCHAR", tableName)); + } + } + @Test public void testCreateTable() throws Exception { String schemaName = "TEST"; diff --git a/phoenix-core/src/it/java/org/apache/phoenix/monitoring/PhoenixLoggingMetricsIT.java b/phoenix-core/src/it/java/org/apache/phoenix/monitoring/PhoenixLoggingMetricsIT.java index 483d341913b..b2413ea2e8f 100644 --- a/phoenix-core/src/it/java/org/apache/phoenix/monitoring/PhoenixLoggingMetricsIT.java +++ b/phoenix-core/src/it/java/org/apache/phoenix/monitoring/PhoenixLoggingMetricsIT.java @@ -31,6 +31,7 @@ import java.sql.Statement; import java.util.Map; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; public class PhoenixLoggingMetricsIT extends BasePhoenixMetricsIT { @@ -74,28 +75,28 @@ public void testResultSetTypeForQueries() throws Exception { String tableName3 = generateUniqueName(); String create = "CREATE TABLE " + tableName3 + " (K INTEGER PRIMARY KEY)"; - assertTrue(executeAndGetResultSet(create) == null); + assertNull(executeAndGetResultSet(create)); String upsert = "UPSERT INTO " + tableName3 + " VALUES (42)"; - assertTrue(executeAndGetResultSet(upsert) == null); + assertNull(executeAndGetResultSet(upsert)); String select = "SELECT * FROM " + tableName3; assertTrue(executeAndGetResultSet(select) instanceof LoggingPhoenixResultSet); - String createView = "CREATE VIEW TEST_VIEW (K INTEGER) AS SELECT * FROM " + tableName3; - assertTrue(executeAndGetResultSet(createView) == null); + String createView = "CREATE VIEW TEST_VIEW (K1 INTEGER) AS SELECT * FROM " + tableName3; + assertNull(executeAndGetResultSet(createView)); String createIndex = "CREATE INDEX TEST_INDEX ON " + tableName3 + " (K)"; - assertTrue(executeAndGetResultSet(createIndex) == null); + assertNull(executeAndGetResultSet(createIndex)); String dropIndex = "DROP INDEX TEST_INDEX ON " + tableName3; - assertTrue(executeAndGetResultSet(dropIndex) == null); + assertNull(executeAndGetResultSet(dropIndex)); String dropView = "DROP VIEW TEST_VIEW"; - assertTrue(executeAndGetResultSet(dropView) == null); + assertNull(executeAndGetResultSet(dropView)); String dropTable = "DROP TABLE " + tableName3; - assertTrue(executeAndGetResultSet(dropTable) == null); + assertNull(executeAndGetResultSet(dropTable)); } @Test diff --git a/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/AddColumnMutator.java b/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/AddColumnMutator.java index 7e74c3d71f0..cf6d098f609 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/AddColumnMutator.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/AddColumnMutator.java @@ -27,6 +27,7 @@ import org.apache.phoenix.coprocessor.MetaDataProtocol.MetaDataMutationResult; import org.apache.phoenix.hbase.index.util.ImmutableBytesPtr; import org.apache.phoenix.jdbc.PhoenixDatabaseMetaData; +import org.apache.phoenix.query.QueryConstants; import org.apache.phoenix.schema.ColumnFamilyNotFoundException; import org.apache.phoenix.schema.ColumnNotFoundException; import org.apache.phoenix.schema.PColumn; @@ -333,6 +334,11 @@ public MetaDataMutationResult validateAndAddMetadata(PTable table, byte[][] rowK EnvironmentEdgeManager.currentTimeMillis(), null); } if (familyName!=null && familyName.length > 0) { + MetaDataMutationResult result = + compareWithPkColumns(colName, table, familyName); + if (result != null) { + return result; + } PColumnFamily family = table.getColumnFamily(familyName); family.getPColumnForColumnNameBytes(colName); @@ -434,6 +440,24 @@ public MetaDataMutationResult validateAndAddMetadata(PTable table, byte[][] rowK return null; } + private MetaDataMutationResult compareWithPkColumns(byte[] colName, + PTable table, byte[] familyName) { + // check if column is matching with any of pk columns if given + // column belongs to default CF + if (Bytes.compareTo(familyName, QueryConstants.DEFAULT_COLUMN_FAMILY_BYTES) == 0 + && colName != null && colName.length > 0) { + for (PColumn pColumn : table.getPKColumns()) { + if (Bytes.compareTo( + pColumn.getName().getBytes(), colName) == 0) { + return new MetaDataProtocol.MetaDataMutationResult( + MetaDataProtocol.MutationCode.COLUMN_ALREADY_EXISTS, + EnvironmentEdgeManager.currentTimeMillis(), table); + } + } + } + return null; + } + @Override public List> getTableAndDroppedColumnPairs() { return Collections.emptyList(); diff --git a/phoenix-core/src/main/java/org/apache/phoenix/schema/MetaDataClient.java b/phoenix-core/src/main/java/org/apache/phoenix/schema/MetaDataClient.java index 919e0720ac6..6754fb1c211 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/schema/MetaDataClient.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/schema/MetaDataClient.java @@ -2635,6 +2635,10 @@ else if (!SchemaUtil.isSystemTable(Bytes.toBytes(SchemaUtil.getTableName(schemaN // Keep track of all columns that are newly added to a view Set viewNewColumnPositions = Sets.newHashSetWithExpectedSize(colDefs.size()); + Set pkColumnNames = new HashSet<>(); + for (PColumn pColumn : pkColumns) { + pkColumnNames.add(pColumn.getName().toString()); + } for (ColumnDef colDef : colDefs) { rowTimeStampColumnAlreadyFound = checkAndValidateRowTimestampCol(colDef, pkConstraint, rowTimeStampColumnAlreadyFound, tableType); if (colDef.isPK()) { // i.e. the column is declared as CREATE TABLE COLNAME DATATYPE PRIMARY KEY... @@ -2698,11 +2702,16 @@ else if (!SchemaUtil.isSystemTable(Bytes.toBytes(SchemaUtil.getTableName(schemaN throw new ColumnAlreadyExistsException(schemaName, tableName, column.getName().getString()); } } - if (columns.put(column, column) != null) { - throw new ColumnAlreadyExistsException(schemaName, tableName, column.getName().getString()); + // check for duplicate column + if (isDuplicateColumn(columns, pkColumnNames, column)) { + throw new ColumnAlreadyExistsException(schemaName, tableName, + column.getName().getString()); } else if (tableType == VIEW) { viewNewColumnPositions.add(column.getPosition()); } + if (isPkColumn) { + pkColumnNames.add(column.getName().toString()); + } if ((colDef.getDataType() == PVarbinary.INSTANCE || colDef.getDataType().isArrayType()) && SchemaUtil.isPKColumn(column) && pkColumnsIterator.hasNext()) { @@ -3180,6 +3189,16 @@ public boolean isViewReferenced() { } } + private boolean isDuplicateColumn(LinkedHashMap columns, + Set pkColumnNames, PColumn column) { + // either column name is same within same CF or column name within + // default CF is same as any of PK column + return columns.put(column, column) != null + || (column.getFamilyName() != null + && DEFAULT_COLUMN_FAMILY.equals(column.getFamilyName().toString()) + && pkColumnNames.contains(column.getName().toString())); + } + private void verifyChangeDetectionTableType(PTableType tableType, Boolean isChangeDetectionEnabledProp) throws SQLException { if (isChangeDetectionEnabledProp != null && isChangeDetectionEnabledProp) { if (tableType != TABLE && tableType != VIEW) { diff --git a/phoenix-core/src/test/java/org/apache/phoenix/compile/QueryOptimizerTest.java b/phoenix-core/src/test/java/org/apache/phoenix/compile/QueryOptimizerTest.java index 5810fed6159..7b137e9248b 100644 --- a/phoenix-core/src/test/java/org/apache/phoenix/compile/QueryOptimizerTest.java +++ b/phoenix-core/src/test/java/org/apache/phoenix/compile/QueryOptimizerTest.java @@ -65,10 +65,7 @@ public class QueryOptimizerTest extends BaseConnectionlessQueryTest { public static final String SCHEMA_NAME = ""; - public static final String DATA_TABLE_NAME = "T"; - public static final String INDEX_TABLE_NAME = "I"; public static final String DATA_TABLE_FULL_NAME = SchemaUtil.getTableName(SCHEMA_NAME, "T"); - public static final String INDEX_TABLE_FULL_NAME = SchemaUtil.getTableName(SCHEMA_NAME, "I"); public QueryOptimizerTest() { } @@ -515,7 +512,7 @@ private void testAssertQueryPlanDetails(boolean multitenant, boolean useIndex, b // create a tenant specific view if multi-tenant if (multitenant) { - conn.createStatement().execute("CREATE VIEW ABC_VIEW (ORGANIZATION_ID VARCHAR) AS SELECT * FROM XYZ.ABC"); + conn.createStatement().execute("CREATE VIEW ABC_VIEW (ORG_ID VARCHAR) AS SELECT * FROM XYZ.ABC"); } String expectedColNames = multitenant ? addQuotes(null, "DEC,A_STRING_ARRAY") : addQuotes(null,"ORGANIZATION_ID,DEC,A_STRING_ARRAY");