This is an automated email from the ASF dual-hosted git repository.
vjasani pushed a commit to branch 5.1
in repository https://gitbox.apache.org/repos/asf/phoenix.git
The following commit(s) were added to refs/heads/5.1 by this push:
new b1665b7 PHOENIX-6343 : Phoenix allows duplicate column names when one
of them is a primary key (#1117)
b1665b7 is described below
commit b1665b779d2821e2dbca6f0700f0ee0127773c5c
Author: Viraj Jasani <[email protected]>
AuthorDate: Sun Feb 14 12:47:20 2021 +0530
PHOENIX-6343 : Phoenix allows duplicate column names when one of them is a
primary key (#1117)
Signed-off-by: Geoffrey Jacoby <[email protected]>
---
.../org/apache/phoenix/end2end/CreateTableIT.java | 131 +++++++++++++++++++++
.../monitoring/PhoenixLoggingMetricsIT.java | 17 +--
.../phoenix/coprocessor/AddColumnMutator.java | 24 ++++
.../org/apache/phoenix/schema/MetaDataClient.java | 23 +++-
.../apache/phoenix/compile/QueryOptimizerTest.java | 5 +-
5 files changed, 186 insertions(+), 14 deletions(-)
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 9b5098e..57714a2 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
@@ -53,6 +53,7 @@ import org.apache.phoenix.query.BaseTest;
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;
@@ -92,6 +93,136 @@ public class CreateTableIT extends ParallelStatsDisabledIT {
}
@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";
String tableName = schemaName + generateUniqueName();
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 914282d..2f9b96f 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.SQLException;
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 class PhoenixLoggingMetricsIT extends
BasePhoenixMetricsIT {
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 51d9e6b..7c9ece5 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
@@ -17,6 +17,7 @@
*/
package org.apache.phoenix.coprocessor;
+import org.apache.phoenix.query.QueryConstants;
import org.apache.phoenix.thirdparty.com.google.common.collect.Lists;
import org.apache.hadoop.hbase.Cell;
import org.apache.hadoop.hbase.ExtendedCellBuilder;
@@ -335,6 +336,11 @@ public class AddColumnMutator implements ColumnMutator {
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);
@@ -437,6 +443,24 @@ public class AddColumnMutator implements ColumnMutator {
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();
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 0049111..1fdf0fc 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 @@ public class MetaDataClient {
// 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 @@ public class MetaDataClient {
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 class MetaDataClient {
}
}
+ 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) {
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 8eb804f..7e336b9 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 @@ import
org.apache.phoenix.thirdparty.com.google.common.base.Splitter;
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 @@ public class QueryOptimizerTest extends
BaseConnectionlessQueryTest {
// 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");
