PHOENIX-6343 : Phoenix allows duplicate column names when one of them is a primary key

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";

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

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<Pair<PTable, PColumn>> getTableAndDroppedColumnPairs() {
  return Collections.emptyList();

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<Integer> viewNewColumnPositions =
  Sets.newHashSetWithExpectedSize(colDefs.size());
+ Set<String> 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<PColumn, PColumn> columns,
+ Set<String> 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) {

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");