This is an automated email from the ASF dual-hosted git repository. stoty pushed a commit to branch 4.16 in repository https://gitbox.apache.org/repos/asf/phoenix.git
The following commit(s) were added to refs/heads/4.16 by this push: new d4ea7d3 PHOENIX-6271: Effective DDL generated by SchemaExtractionTool should maintain the order of PK and other columns (#1212) d4ea7d3 is described below commit d4ea7d3832485b6f7cfa26508e8e72b25e9eb0de Author: Swaroopa Kadam <swaroopa.kada...@gmail.com> AuthorDate: Thu Apr 29 11:05:38 2021 -0700 PHOENIX-6271: Effective DDL generated by SchemaExtractionTool should maintain the order of PK and other columns (#1212) Co-authored-by: Swaroopa Kadam <s.ka...@apache.org> --- .../java/org/apache/phoenix/util/SchemaUtil.java | 13 +- .../phoenix/schema/SchemaExtractionToolIT.java | 159 +++++++++++++++++++-- .../phoenix/schema/SchemaExtractionProcessor.java | 68 +++++---- 3 files changed, 194 insertions(+), 46 deletions(-) diff --git a/phoenix-core/src/main/java/org/apache/phoenix/util/SchemaUtil.java b/phoenix-core/src/main/java/org/apache/phoenix/util/SchemaUtil.java index 014ea24..49961f4 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/util/SchemaUtil.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/util/SchemaUtil.java @@ -47,6 +47,7 @@ import java.util.TreeSet; import javax.annotation.Nullable; +import com.google.common.base.Strings; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hbase.Cell; import org.apache.hadoop.hbase.HColumnDescriptor; @@ -1271,15 +1272,21 @@ public class SchemaUtil { pTableName = "\""+pTableName+"\""; } if(tableNameNeedsQuotes || schemaNameNeedsQuotes) { - pTableFullName = pSchemaName + "." + pTableName; + if (!Strings.isNullOrEmpty(pSchemaName)) { + return String.format("%s.%s", pSchemaName, pTableName); + } else { + return pTableName; + } } - return pTableFullName; } private static boolean isQuotesNeeded(String name) { // first char numeric or non-underscore - if(!Character.isAlphabetic(name.charAt(0)) && name.charAt(0)!='_') { + if (Strings.isNullOrEmpty(name)) { + return false; + } + if (!Character.isAlphabetic(name.charAt(0)) && name.charAt(0)!='_') { return true; } // for all other chars diff --git a/phoenix-tools/src/it/java/org/apache/phoenix/schema/SchemaExtractionToolIT.java b/phoenix-tools/src/it/java/org/apache/phoenix/schema/SchemaExtractionToolIT.java index 4f9b11b..34b09db 100644 --- a/phoenix-tools/src/it/java/org/apache/phoenix/schema/SchemaExtractionToolIT.java +++ b/phoenix-tools/src/it/java/org/apache/phoenix/schema/SchemaExtractionToolIT.java @@ -31,7 +31,9 @@ import org.junit.Test; import java.sql.Connection; import java.sql.DriverManager; +import java.sql.ResultSet; import java.sql.SQLException; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.ArrayList; @@ -154,18 +156,46 @@ public class SchemaExtractionToolIT extends ParallelStatsEnabledIT { + "id3 VARCHAR NOT NULL CONSTRAINT PKVIEW PRIMARY KEY (id2, id3 DESC)) " + "AS SELECT * FROM "+pTableFullName; String createView1 = "CREATE VIEW "+childviewName + " AS SELECT * FROM "+viewFullName; - String createIndexStatement = "CREATE INDEX "+indexName + " ON "+childviewName+"(id1) INCLUDE (v1)"; + String createIndexStatement = "CREATE INDEX "+indexName + " ON "+childviewName+"(id2, id1) INCLUDE (v1)"; List<String> queries = new ArrayList<String>(){}; queries.add(createTableStmt); queries.add(createView); queries.add(createView1); queries.add(createIndexStatement); + String expected = "CREATE INDEX %s ON " +childviewName +"(ID2, ID1, K, ID3 DESC) INCLUDE (V1)"; String result = runSchemaExtractionTool(schemaName, indexName, null, queries); - Assert.assertEquals(createIndexStatement.toUpperCase(), result.toUpperCase()); + Assert.assertEquals(String.format(expected, indexName).toUpperCase(), result.toUpperCase()); + queries.clear(); + String newIndex =indexName+"_NEW"; + queries.add(String.format(expected, newIndex)); + Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES); + try (Connection conn = DriverManager.getConnection(getUrl(), props)) { + executeCreateStatements(conn, queries); + } + compareOrdinalPositions(indexName, newIndex); + } + + private void compareOrdinalPositions(String table, String newTable) throws SQLException { + String ordinalQuery = "SELECT COLUMN_NAME, " + + "ORDINAL_POSITION FROM SYSTEM.CATALOG" + + " WHERE TABLE_NAME='%s' AND ORDINAL_POSITION IS NOT NULL ORDER BY COLUMN_NAME"; + Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES); + Map<String, Integer> ordinalMap = new HashMap<>(); + try (Connection conn = DriverManager.getConnection(getUrl(), props)) { + ResultSet rs = conn.createStatement().executeQuery(String.format(ordinalQuery, table)); + while(rs.next()) { + ordinalMap.put(rs.getString(1), rs.getInt(2)); + } + rs = conn.createStatement().executeQuery(String.format(ordinalQuery, newTable)); + while(rs.next()) { + Assert.assertEquals(ordinalMap.get(rs.getString(1)).intValue(), + rs.getInt(2)); + } + } } @Test - public void testCreateTableStatement_tenant() throws Exception { + public void testCreateViewStatement_tenant() throws Exception { String tableName = generateUniqueName(); String viewName = generateUniqueName(); String schemaName = generateUniqueName(); @@ -218,8 +248,7 @@ public class SchemaExtractionToolIT extends ParallelStatsEnabledIT { @Test public void testCreateTableWithArrayColumn() throws Exception { String tableName = generateUniqueName(); - String schemaName = generateUniqueName(); - String pTableFullName = SchemaUtil.getQualifiedTableName(schemaName, tableName); + String pTableFullName = tableName; String query = "create table " + pTableFullName + "(a_char CHAR(15) NOT NULL, " + "b_char CHAR(10) NOT NULL, " + @@ -228,7 +257,7 @@ public class SchemaExtractionToolIT extends ParallelStatsEnabledIT { "TTL=2592000, IMMUTABLE_STORAGE_SCHEME='ONE_CELL_PER_COLUMN', REPLICATION_SCOPE=1"; List<String> queries = new ArrayList<String>(){}; queries.add(query); - String result = runSchemaExtractionTool(schemaName, tableName, null, queries); + String result = runSchemaExtractionTool("", tableName, null, queries); Assert.assertEquals(query.toUpperCase(), result.toUpperCase()); } @@ -297,9 +326,13 @@ public class SchemaExtractionToolIT extends ParallelStatsEnabledIT { public void testCreateTableWithMultipleCFProperties() throws Exception { String tableName = "07"+generateUniqueName(); String schemaName = generateUniqueName(); - String properties = "\"av\".DATA_BLOCK_ENCODING='DIFF', \"bv\".DATA_BLOCK_ENCODING='DIFF', \"cv\".DATA_BLOCK_ENCODING='DIFF', " + - "IMMUTABLE_STORAGE_SCHEME='ONE_CELL_PER_COLUMN', SALT_BUCKETS=16, MULTI_TENANT=true, BLOOMFITER='ROW'"; - String simplifiedProperties = "DATA_BLOCK_ENCODING='DIFF', IMMUTABLE_STORAGE_SCHEME='ONE_CELL_PER_COLUMN', SALT_BUCKETS=16, MULTI_TENANT=true, BLOOMFITER='ROW'"; + String properties = "\"av\".DATA_BLOCK_ENCODING='DIFF', \"bv\".DATA_BLOCK_ENCODING='DIFF', " + + "\"cv\".DATA_BLOCK_ENCODING='DIFF', " + + "IMMUTABLE_STORAGE_SCHEME='ONE_CELL_PER_COLUMN', " + + "SALT_BUCKETS=16, MULTI_TENANT=true, BLOOMFITER='ROW'"; + String simplifiedProperties = "DATA_BLOCK_ENCODING='DIFF', " + + "IMMUTABLE_STORAGE_SCHEME='ONE_CELL_PER_COLUMN', " + + "SALT_BUCKETS=16, MULTI_TENANT=true, BLOOMFITER='ROW'"; String query = "create table " + schemaName+".\""+tableName+"\"" + "(a_char CHAR(15) NOT NULL, " + "b_char CHAR(10) NOT NULL, " + @@ -318,18 +351,119 @@ public class SchemaExtractionToolIT extends ParallelStatsEnabledIT { } @Test + public void testColumnAndPKOrdering() throws Exception { + String table = "CREATE TABLE IF NOT EXISTS MY_SCHEMA.MY_DATA_TABLE (\n" + + " ORGANIZATION_ID CHAR(15) NOT NULL, \n" + + " KEY_PREFIX CHAR(3) NOT NULL,\n" + + " CREATED_DATE DATE,\n" + + " CREATED_BY CHAR(15) \n" + + " CONSTRAINT PK PRIMARY KEY (\n" + + " ORGANIZATION_ID, \n" + + " KEY_PREFIX\n" + " )\n" + + ") VERSIONS=1, IMMUTABLE_ROWS=true, MULTI_TENANT=true, REPLICATION_SCOPE=1"; + + String view = "CREATE VIEW IF NOT EXISTS MY_SCHEMA.MY_DATA_VIEW (\n" + + " DATE_TIME1 DATE NOT NULL,\n" + + " TEXT1 VARCHAR NOT NULL,\n" + + " INT1 BIGINT NOT NULL,\n" + + " DOUBLE1 DECIMAL(12, 3),\n" + + " DOUBLE2 DECIMAL(12, 3),\n" + + " DOUBLE3 DECIMAL(12, 3),\n" + + " CONSTRAINT PKVIEW PRIMARY KEY\n" + " (\n" + + " DATE_TIME1, TEXT1, INT1\n" + " )\n" + ")\n" + + "AS SELECT * FROM MY_SCHEMA.MY_DATA_TABLE WHERE KEY_PREFIX = '9Yj'"; + + String index = "CREATE INDEX IF NOT EXISTS MY_VIEW_INDEX\n" + + "ON MY_SCHEMA.MY_DATA_VIEW (TEXT1, DATE_TIME1 DESC, DOUBLE1)\n" + + "INCLUDE (CREATED_BY, CREATED_DATE)"; + List<String> queries = new ArrayList<String>(){}; + + queries.add(table); + queries.add(view); + queries.add(index); + + String expectedIndex = "CREATE INDEX MY_VIEW_INDEX " + + "ON MY_SCHEMA.MY_DATA_VIEW(TEXT1, DATE_TIME1 DESC, DOUBLE1, INT1)" + + " INCLUDE (CREATED_BY, CREATED_DATE)"; + String result = runSchemaExtractionTool("MY_SCHEMA", "MY_VIEW_INDEX", null, queries); + Assert.assertEquals(expectedIndex.toUpperCase(), result.toUpperCase()); + + String expectedView = "CREATE VIEW MY_SCHEMA.MY_DATA_VIEW(DATE_TIME1 DATE NOT NULL, " + + "TEXT1 VARCHAR NOT NULL, INT1 BIGINT NOT NULL, DOUBLE1 DECIMAL(12,3), " + + "DOUBLE2 DECIMAL(12,3), DOUBLE3 DECIMAL(12,3)" + + " CONSTRAINT PKVIEW PRIMARY KEY (DATE_TIME1, TEXT1, INT1))" + + " AS SELECT * FROM MY_SCHEMA.MY_DATA_TABLE WHERE KEY_PREFIX = '9YJ'"; + result = runSchemaExtractionTool("MY_SCHEMA", "MY_DATA_VIEW", null, new ArrayList<String>()); + Assert.assertEquals(expectedView.toUpperCase(), result.toUpperCase()); + } + + @Test + public void testColumnAndPKOrdering_nonView() throws Exception { + String indexName = "MY_DATA_TABLE_INDEX"; + String table = "CREATE TABLE MY_SCHEMA.MY_SAMPLE_DATA_TABLE(" + + "ORGANIZATION_ID CHAR(15) NOT NULL," + + " SOME_ID_COLUMN CHAR(3) NOT NULL," + + " SOME_ID_COLUMN_2 CHAR(15) NOT NULL," + + " CREATED_DATE DATE NOT NULL," + + " SOME_ID_COLUMN_3 CHAR(15) NOT NULL," + + " SOME_ID_COLUMN_4 CHAR(15)," + + " CREATED_BY_ID VARCHAR," + + " VALUE_FIELD VARCHAR" + + " CONSTRAINT PK PRIMARY KEY (ORGANIZATION_ID, SOME_ID_COLUMN, SOME_ID_COLUMN_2," + + " CREATED_DATE DESC, SOME_ID_COLUMN_3))" + + " IMMUTABLE_ROWS=true, IMMUTABLE_STORAGE_SCHEME='ONE_CELL_PER_COLUMN'," + + " MULTI_TENANT=true, REPLICATION_SCOPE=1\n"; + + String index = "CREATE INDEX IF NOT EXISTS MY_DATA_TABLE_INDEX\n" + + " ON MY_SCHEMA.MY_SAMPLE_DATA_TABLE (SOME_ID_COLUMN, CREATED_DATE DESC," + + " SOME_ID_COLUMN_2, SOME_ID_COLUMN_3)\n" + + " INCLUDE\n" + + "(SOME_ID_COLUMN_4, CREATED_BY_ID, VALUE_FIELD)\n"; + List<String> queries = new ArrayList<String>(){}; + + queries.add(table); + queries.add(index); + String result = runSchemaExtractionTool("MY_SCHEMA", + "MY_DATA_TABLE_INDEX", null, queries); + String expected = "CREATE INDEX %s ON MY_SCHEMA.MY_SAMPLE_DATA_TABLE" + + "(SOME_ID_COLUMN, CREATED_DATE DESC, SOME_ID_COLUMN_2, SOME_ID_COLUMN_3) " + + "INCLUDE (SOME_ID_COLUMN_4, CREATED_BY_ID, VALUE_FIELD)"; + + Assert.assertEquals(String.format(expected, indexName).toUpperCase(), result.toUpperCase()); + queries.clear(); + String newIndex = indexName+"_NEW"; + queries.add(String.format(expected, newIndex)); + Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES); + try (Connection conn = DriverManager.getConnection(getUrl(), props)) { + executeCreateStatements(conn, queries); + } + compareOrdinalPositions(indexName, newIndex); + } + + @Test public void testCreateIndexStatementWithColumnFamily() throws Exception { String tableName = generateUniqueName(); String schemaName = generateUniqueName(); String indexName = generateUniqueName(); String pTableFullName = SchemaUtil.getQualifiedTableName(schemaName, tableName); - String createTableStmt = "CREATE TABLE "+pTableFullName + "(k VARCHAR NOT NULL PRIMARY KEY, \"av\".\"_\" CHAR(1), v2 VARCHAR)"; + String createTableStmt = "CREATE TABLE "+pTableFullName + "(k VARCHAR NOT NULL PRIMARY KEY, " + + "\"av\".\"_\" CHAR(1), v2 VARCHAR)"; String createIndexStmt = "CREATE INDEX "+ indexName + " ON "+pTableFullName+ "(\"av\".\"_\")"; List<String> queries = new ArrayList<String>() {}; queries.add(createTableStmt); queries.add(createIndexStmt); + //by the principle of having maximal columns in pk + String expected = "CREATE INDEX %s ON "+pTableFullName+ "(\"av\".\"_\", K)"; String result = runSchemaExtractionTool(schemaName, indexName, null, queries); - Assert.assertEquals(createIndexStmt.toUpperCase(), result.toUpperCase()); + Assert.assertEquals(String.format(expected, indexName).toUpperCase(), result.toUpperCase()); + queries.clear(); + String newIndex = indexName+"_NEW"; + queries.add(String.format(expected, newIndex)); + Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES); + try (Connection conn = DriverManager.getConnection(getUrl(), props)) { + executeCreateStatements(conn, queries); + } + compareOrdinalPositions(indexName, newIndex); } private Connection getTenantConnection(String url, String tenantId) throws SQLException { @@ -338,7 +472,8 @@ public class SchemaExtractionToolIT extends ParallelStatsEnabledIT { return DriverManager.getConnection(url, props); } - private String runSchemaExtractionTool(String schemaName, String tableName, String tenantId, List<String> queries) throws Exception { + private String runSchemaExtractionTool(String schemaName, String tableName, String tenantId, + List<String> queries) throws Exception { Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES); String output; if (tenantId == null) { diff --git a/phoenix-tools/src/main/java/org/apache/phoenix/schema/SchemaExtractionProcessor.java b/phoenix-tools/src/main/java/org/apache/phoenix/schema/SchemaExtractionProcessor.java index 93632cb..33095fc 100644 --- a/phoenix-tools/src/main/java/org/apache/phoenix/schema/SchemaExtractionProcessor.java +++ b/phoenix-tools/src/main/java/org/apache/phoenix/schema/SchemaExtractionProcessor.java @@ -16,11 +16,7 @@ * limitations under the License. */ package org.apache.phoenix.schema; - -import com.google.common.base.Strings; -import com.google.common.collect.Sets; import org.apache.commons.lang.StringUtils; -import org.apache.commons.lang.math.NumberUtils; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hbase.HColumnDescriptor; import org.apache.hadoop.hbase.HTableDescriptor; @@ -107,40 +103,60 @@ public class SchemaExtractionProcessor { List<PColumn> indexPK = indexPTable.getPKColumns(); List<PColumn> dataPK = dataPTable.getPKColumns(); - Set<String> indexPkSet = new HashSet<>(); - Set<String> dataPkSet = new HashSet<>(); - Map<String, SortOrder> sortOrderMap = new HashMap<>(); + List<String> indexPKName = new ArrayList<>(); + List<String> dataPKName = new ArrayList<>(); + Map<String, SortOrder> indexSortOrderMap = new HashMap<>(); StringBuilder indexedColumnsBuilder = new StringBuilder(); + for (PColumn indexedColumn : indexPK) { String indexColumn = extractIndexColumn(indexedColumn.getName().getString(), defaultCF); if(indexColumn.equalsIgnoreCase(MetaDataUtil.VIEW_INDEX_ID_COLUMN_NAME)) { continue; } - indexPkSet.add(indexColumn); - sortOrderMap.put(indexColumn, indexedColumn.getSortOrder()); + indexPKName.add(indexColumn); + indexSortOrderMap.put(indexColumn, indexedColumn.getSortOrder()); } - for(PColumn pColumn : dataPK) { - dataPkSet.add(pColumn.getName().getString()); + dataPKName.add(pColumn.getName().getString()); } - Set<String> effectivePK = Sets.symmetricDifference(indexPkSet, dataPkSet); - if (effectivePK.isEmpty()) { - effectivePK = indexPkSet; + // This is added because of PHOENIX-2340 + String tenantIdColumn = dataPKName.get(0); + if (dataPTable.isMultiTenant() && indexPKName.contains(tenantIdColumn)) { + indexPKName.remove(tenantIdColumn); } - for (String column : effectivePK) { + + for (String column : indexPKName) { if(indexedColumnsBuilder.length()!=0) { indexedColumnsBuilder.append(", "); } indexedColumnsBuilder.append(column); - if(sortOrderMap.containsKey(column) && sortOrderMap.get(column) != SortOrder.getDefault()) { + if(indexSortOrderMap.containsKey(column) + && indexSortOrderMap.get(column) != SortOrder.getDefault()) { indexedColumnsBuilder.append(" "); - indexedColumnsBuilder.append(sortOrderMap.get(column)); + indexedColumnsBuilder.append(indexSortOrderMap.get(column)); } } return indexedColumnsBuilder.toString(); } + private List<PColumn> getSymmetricDifferencePColumns(List<PColumn> firstList, List<PColumn> secondList) { + List<PColumn> effectivePK = new ArrayList<>(); + for(PColumn column : firstList) { + if(secondList.contains(column)) { + continue; + } + effectivePK.add(column); + } + for(PColumn column : secondList) { + if(firstList.contains(column)) { + continue; + } + effectivePK.add(column); + } + return effectivePK; + } + private String extractIndexColumn(String columnName, String defaultCF) { String [] columnNameSplit = columnName.split(":"); if(columnNameSplit[0].equals("") || columnNameSplit[0].equalsIgnoreCase(defaultCF)) { @@ -273,8 +289,8 @@ public class SchemaExtractionProcessor { for(Map.Entry<ImmutableBytesWritable, ImmutableBytesWritable> entry : propsMap.entrySet()) { ImmutableBytesWritable key = entry.getKey(); ImmutableBytesWritable globalValue = entry.getValue(); - Map<String, String> cfToPropertyValueMap = new HashMap<String, String>(); - Set<ImmutableBytesWritable> cfPropertyValueSet = new HashSet<ImmutableBytesWritable>(); + Map<String, String> cfToPropertyValueMap = new HashMap<>(); + Set<ImmutableBytesWritable> cfPropertyValueSet = new HashSet<>(); for(HColumnDescriptor columnDescriptor: columnDescriptors) { String columnFamilyName = Bytes.toString(columnDescriptor.getName()); ImmutableBytesWritable value = columnDescriptor.getValues().get(key); @@ -406,21 +422,11 @@ public class SchemaExtractionProcessor { List<PColumn> columns = table.getColumns(); List<PColumn> pkColumns = table.getPKColumns(); - Set<PColumn> columnSet = new HashSet<>(columns); - Set<PColumn> pkSet = new HashSet<>(pkColumns); - List<PColumn> baseColumns = baseTable.getColumns(); List<PColumn> basePkColumns = baseTable.getPKColumns(); - Set<PColumn> baseColumnSet = new HashSet<>(baseColumns); - Set<PColumn> basePkSet = new HashSet<>(basePkColumns); - - Set<PColumn> columnsSet = Sets.symmetricDifference(baseColumnSet, columnSet); - Set<PColumn> pkColumnsSet = Sets.symmetricDifference(basePkSet, pkSet); - - columns = new ArrayList<>(columnsSet); - pkColumns = new ArrayList<>(pkColumnsSet); - + columns = getSymmetricDifferencePColumns(baseColumns, columns); + pkColumns = getSymmetricDifferencePColumns(basePkColumns, pkColumns); return getColumnInfoString(table, colInfo, columns, pkColumns); }