virajjasani commented on a change in pull request #978: URL: https://github.com/apache/phoenix/pull/978#discussion_r531606951
########## File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/DeleteIT.java ########## @@ -943,6 +961,163 @@ public void testDeleteFilterWithMultipleIndexes() throws Exception { } } } -} + /* + Tests whether we have cell tags in delete marker for + ClientSelectDeleteMutationPlan. + */ + @Test + public void testDeleteClientDeleteMutationPlan() throws Exception { + String tableName = generateUniqueName(); + String indexName = generateUniqueName(); + String tagValue = "customer-delete"; + String delete = "DELETE FROM " + tableName + " WHERE v1 = 'foo'"; + Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES); + // Add tag "customer-delete" to delete marker. + props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue); + + createAndUpsertTable(tableName, indexName, props); + // Make sure that the plan creates is of ClientSelectDeleteMutationPlan + verifyDeletePlan(delete, DeleteCompiler.ClientSelectDeleteMutationPlan.class, props); + executeDelete(delete, props, 1); + String startRowKeyForBaseTable = "1"; + String startRowKeyForIndexTable = "foo"; + // Make sure that Delete Marker has cell tag for base table + // and has no cell tag for index table. + checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue); + checkTagPresentInDeleteMarker(indexName, startRowKeyForIndexTable, false, null); + } + + /* + Tests whether we have cell tags in delete marker for + ServerSelectDeleteMutationPlan. + */ + @Test + public void testDeleteServerDeleteMutationPlan() throws Exception { + String tableName = generateUniqueName(); + String indexName = generateUniqueName(); + String tagValue = "customer-delete"; + String delete = "DELETE FROM " + tableName; + Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES); + props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue); + + createAndUpsertTable(tableName, indexName, props); + // Make sure that the plan creates is of ServerSelectDeleteMutationPlan + verifyDeletePlan(delete, DeleteCompiler.ServerSelectDeleteMutationPlan.class, props); + executeDelete(delete, props, 2); + + String startRowKeyForBaseTable = "1"; + String startRowKeyForIndexTable = "foo"; + // Make sure that Delete Marker has cell tag for base table + // and has no cell tag for index table. + checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue); + checkTagPresentInDeleteMarker(indexName, startRowKeyForIndexTable, false, null); + } + + /* + Tests whether we have cell tags in delete marker for + MultiRowDeleteMutationPlan. + */ + @Test + public void testDeleteMultiRowDeleteMutationPlan() throws Exception { + String tableName = generateUniqueName(); + String tagValue = "customer-delete"; + String delete = "DELETE FROM " + tableName + " WHERE k = 1"; + Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES); + props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue); + // Don't create index table. We will use MultiRowDeleteMutationPlan + // if there is no index present for a table. + createAndUpsertTable(tableName, null, props); + // Make sure that the plan creates is of MultiRowDeleteMutationPlan + verifyDeletePlan(delete, DeleteCompiler.MultiRowDeleteMutationPlan.class, props); + executeDelete(delete, props, 1); + String startRowKeyForBaseTable = "1"; + // Make sure that Delete Marker has cell tag for base table. + // We haven't created index table for this test case. + checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue); + } + + /* + Verify whether plan that we create for delete statement is of planClass + */ + private void verifyDeletePlan(String delete, Class planClass, Properties props) Review comment: nit: `Class<? extends MutationPlan> planClass` ? ########## File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/DeleteIT.java ########## @@ -943,6 +961,163 @@ public void testDeleteFilterWithMultipleIndexes() throws Exception { } } } -} + /* + Tests whether we have cell tags in delete marker for + ClientSelectDeleteMutationPlan. + */ + @Test + public void testDeleteClientDeleteMutationPlan() throws Exception { + String tableName = generateUniqueName(); + String indexName = generateUniqueName(); + String tagValue = "customer-delete"; + String delete = "DELETE FROM " + tableName + " WHERE v1 = 'foo'"; + Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES); + // Add tag "customer-delete" to delete marker. + props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue); + + createAndUpsertTable(tableName, indexName, props); + // Make sure that the plan creates is of ClientSelectDeleteMutationPlan + verifyDeletePlan(delete, DeleteCompiler.ClientSelectDeleteMutationPlan.class, props); + executeDelete(delete, props, 1); + String startRowKeyForBaseTable = "1"; + String startRowKeyForIndexTable = "foo"; + // Make sure that Delete Marker has cell tag for base table + // and has no cell tag for index table. + checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue); + checkTagPresentInDeleteMarker(indexName, startRowKeyForIndexTable, false, null); + } + + /* + Tests whether we have cell tags in delete marker for + ServerSelectDeleteMutationPlan. + */ + @Test + public void testDeleteServerDeleteMutationPlan() throws Exception { + String tableName = generateUniqueName(); + String indexName = generateUniqueName(); + String tagValue = "customer-delete"; + String delete = "DELETE FROM " + tableName; + Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES); + props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue); + + createAndUpsertTable(tableName, indexName, props); + // Make sure that the plan creates is of ServerSelectDeleteMutationPlan + verifyDeletePlan(delete, DeleteCompiler.ServerSelectDeleteMutationPlan.class, props); + executeDelete(delete, props, 2); + + String startRowKeyForBaseTable = "1"; + String startRowKeyForIndexTable = "foo"; + // Make sure that Delete Marker has cell tag for base table + // and has no cell tag for index table. + checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue); + checkTagPresentInDeleteMarker(indexName, startRowKeyForIndexTable, false, null); + } + + /* + Tests whether we have cell tags in delete marker for + MultiRowDeleteMutationPlan. + */ + @Test + public void testDeleteMultiRowDeleteMutationPlan() throws Exception { + String tableName = generateUniqueName(); + String tagValue = "customer-delete"; + String delete = "DELETE FROM " + tableName + " WHERE k = 1"; + Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES); + props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue); + // Don't create index table. We will use MultiRowDeleteMutationPlan + // if there is no index present for a table. + createAndUpsertTable(tableName, null, props); + // Make sure that the plan creates is of MultiRowDeleteMutationPlan + verifyDeletePlan(delete, DeleteCompiler.MultiRowDeleteMutationPlan.class, props); + executeDelete(delete, props, 1); + String startRowKeyForBaseTable = "1"; + // Make sure that Delete Marker has cell tag for base table. + // We haven't created index table for this test case. + checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue); + } + + /* + Verify whether plan that we create for delete statement is of planClass + */ + private void verifyDeletePlan(String delete, Class planClass, Properties props) + throws SQLException { + try(Connection conn = DriverManager.getConnection(getUrl(), props)) { + conn.setAutoCommit(true); + PhoenixStatement stmt = conn.createStatement().unwrap(PhoenixStatement.class); + SQLParser parser = new SQLParser(delete); + DeleteStatement deleteStmt = (DeleteStatement) parser.parseStatement(); + DeleteCompiler compiler = new DeleteCompiler(stmt, null); + MutationPlan plan = compiler.compile(deleteStmt); + assertEquals(plan.getClass(), planClass); + } + } + private void createAndUpsertTable(String tableName, String indexName, Properties props) + throws SQLException { + String ddl = "CREATE TABLE " + tableName + + " (k INTEGER NOT NULL PRIMARY KEY, v1 VARCHAR, v2 VARCHAR)"; + try(Connection conn = DriverManager.getConnection(getUrl(), props)) { + conn.setAutoCommit(true); + try (Statement statement = conn.createStatement()) { + statement.execute(ddl); + if (indexName != null) { + String indexDdl1 = "CREATE INDEX " + indexName + " ON " + tableName + "(v1,v2)"; + statement.execute(indexDdl1); + } + } + conn.createStatement().execute( + "upsert into " + tableName + " values (1, 'foo', 'foo1')"); + conn.createStatement().execute( + "upsert into " + tableName + " values (2, 'bar', 'bar1')"); + conn.commit(); + } + } + + private void executeDelete(String delete, Properties props, int deleteRowCount) + throws SQLException { + try(Connection conn = DriverManager.getConnection(getUrl(), props)) { + conn.setAutoCommit(true); + try (Statement statement = conn.createStatement()) { + int rs = statement.executeUpdate(delete); + assertEquals( deleteRowCount, rs); + } + } + } + /* + Verify whether we have tags present for base table and not present for + index tables. + */ + private void checkTagPresentInDeleteMarker(String tableName, String startRowKey, + boolean tagPresent, String tagValue) throws IOException { + List<Cell> values = new ArrayList<>(); + TableName table = TableName.valueOf(tableName); + // Scan table with specified startRowKey + for (HRegion region : getUtility().getHBaseCluster().getRegions(table)) { + values.clear(); + Scan scan = new Scan(); + // Make sure to set rawScan to true so that we will get Delete Markers. + scan.setRaw(true); + scan.withStartRow(Bytes.toBytes(startRowKey)); + RegionScanner scanner = region.getScanner(scan); + scanner.next(values); + if (!values.isEmpty()) { + break; + } + } + assertTrue("Values shouldn't be empty", !values.isEmpty()); Review comment: nit: simplify to `assertFalse("Values shouldn't be empty", values.isEmpty());` ? ########## File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/DeleteIT.java ########## @@ -943,6 +961,163 @@ public void testDeleteFilterWithMultipleIndexes() throws Exception { } } } -} + /* + Tests whether we have cell tags in delete marker for + ClientSelectDeleteMutationPlan. + */ + @Test + public void testDeleteClientDeleteMutationPlan() throws Exception { + String tableName = generateUniqueName(); + String indexName = generateUniqueName(); + String tagValue = "customer-delete"; + String delete = "DELETE FROM " + tableName + " WHERE v1 = 'foo'"; + Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES); + // Add tag "customer-delete" to delete marker. + props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue); + + createAndUpsertTable(tableName, indexName, props); + // Make sure that the plan creates is of ClientSelectDeleteMutationPlan + verifyDeletePlan(delete, DeleteCompiler.ClientSelectDeleteMutationPlan.class, props); + executeDelete(delete, props, 1); + String startRowKeyForBaseTable = "1"; + String startRowKeyForIndexTable = "foo"; + // Make sure that Delete Marker has cell tag for base table + // and has no cell tag for index table. + checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue); + checkTagPresentInDeleteMarker(indexName, startRowKeyForIndexTable, false, null); + } + + /* + Tests whether we have cell tags in delete marker for + ServerSelectDeleteMutationPlan. + */ + @Test + public void testDeleteServerDeleteMutationPlan() throws Exception { + String tableName = generateUniqueName(); + String indexName = generateUniqueName(); + String tagValue = "customer-delete"; + String delete = "DELETE FROM " + tableName; + Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES); + props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue); + + createAndUpsertTable(tableName, indexName, props); + // Make sure that the plan creates is of ServerSelectDeleteMutationPlan + verifyDeletePlan(delete, DeleteCompiler.ServerSelectDeleteMutationPlan.class, props); + executeDelete(delete, props, 2); + + String startRowKeyForBaseTable = "1"; + String startRowKeyForIndexTable = "foo"; + // Make sure that Delete Marker has cell tag for base table + // and has no cell tag for index table. + checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue); + checkTagPresentInDeleteMarker(indexName, startRowKeyForIndexTable, false, null); + } + + /* + Tests whether we have cell tags in delete marker for + MultiRowDeleteMutationPlan. + */ + @Test + public void testDeleteMultiRowDeleteMutationPlan() throws Exception { + String tableName = generateUniqueName(); + String tagValue = "customer-delete"; + String delete = "DELETE FROM " + tableName + " WHERE k = 1"; + Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES); + props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue); + // Don't create index table. We will use MultiRowDeleteMutationPlan + // if there is no index present for a table. + createAndUpsertTable(tableName, null, props); + // Make sure that the plan creates is of MultiRowDeleteMutationPlan + verifyDeletePlan(delete, DeleteCompiler.MultiRowDeleteMutationPlan.class, props); + executeDelete(delete, props, 1); + String startRowKeyForBaseTable = "1"; + // Make sure that Delete Marker has cell tag for base table. + // We haven't created index table for this test case. + checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue); + } + + /* + Verify whether plan that we create for delete statement is of planClass + */ + private void verifyDeletePlan(String delete, Class planClass, Properties props) + throws SQLException { + try(Connection conn = DriverManager.getConnection(getUrl(), props)) { + conn.setAutoCommit(true); + PhoenixStatement stmt = conn.createStatement().unwrap(PhoenixStatement.class); + SQLParser parser = new SQLParser(delete); + DeleteStatement deleteStmt = (DeleteStatement) parser.parseStatement(); + DeleteCompiler compiler = new DeleteCompiler(stmt, null); + MutationPlan plan = compiler.compile(deleteStmt); + assertEquals(plan.getClass(), planClass); + } + } + private void createAndUpsertTable(String tableName, String indexName, Properties props) + throws SQLException { + String ddl = "CREATE TABLE " + tableName + + " (k INTEGER NOT NULL PRIMARY KEY, v1 VARCHAR, v2 VARCHAR)"; + try(Connection conn = DriverManager.getConnection(getUrl(), props)) { + conn.setAutoCommit(true); + try (Statement statement = conn.createStatement()) { + statement.execute(ddl); + if (indexName != null) { + String indexDdl1 = "CREATE INDEX " + indexName + " ON " + tableName + "(v1,v2)"; + statement.execute(indexDdl1); + } + } + conn.createStatement().execute( + "upsert into " + tableName + " values (1, 'foo', 'foo1')"); + conn.createStatement().execute( + "upsert into " + tableName + " values (2, 'bar', 'bar1')"); Review comment: nit: both these statements can be included with above `try (Statement statement = conn.createStatement())` ########## File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/DeleteIT.java ########## @@ -943,6 +961,163 @@ public void testDeleteFilterWithMultipleIndexes() throws Exception { } } } -} + /* + Tests whether we have cell tags in delete marker for + ClientSelectDeleteMutationPlan. + */ + @Test + public void testDeleteClientDeleteMutationPlan() throws Exception { + String tableName = generateUniqueName(); + String indexName = generateUniqueName(); + String tagValue = "customer-delete"; + String delete = "DELETE FROM " + tableName + " WHERE v1 = 'foo'"; + Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES); + // Add tag "customer-delete" to delete marker. + props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue); + + createAndUpsertTable(tableName, indexName, props); + // Make sure that the plan creates is of ClientSelectDeleteMutationPlan + verifyDeletePlan(delete, DeleteCompiler.ClientSelectDeleteMutationPlan.class, props); + executeDelete(delete, props, 1); + String startRowKeyForBaseTable = "1"; + String startRowKeyForIndexTable = "foo"; + // Make sure that Delete Marker has cell tag for base table + // and has no cell tag for index table. + checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue); + checkTagPresentInDeleteMarker(indexName, startRowKeyForIndexTable, false, null); + } + + /* + Tests whether we have cell tags in delete marker for + ServerSelectDeleteMutationPlan. + */ + @Test + public void testDeleteServerDeleteMutationPlan() throws Exception { + String tableName = generateUniqueName(); + String indexName = generateUniqueName(); + String tagValue = "customer-delete"; + String delete = "DELETE FROM " + tableName; + Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES); + props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue); + + createAndUpsertTable(tableName, indexName, props); + // Make sure that the plan creates is of ServerSelectDeleteMutationPlan + verifyDeletePlan(delete, DeleteCompiler.ServerSelectDeleteMutationPlan.class, props); Review comment: `ServerSelectDeleteMutationPlan` is because DELETE query has no WHERE clause and we have index on base table which will be part of queryPlan? ``` boolean runOnServer = isAutoCommit && !hasPreOrPostProcessing && !table.isTransactional() && !hasClientSideIndexes && allowServerMutations; runOnServer &= queryPlans.get(0).getTableRef().getTable().getType() != PTableType.INDEX; ``` Is this the correct reference? ########## File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/DeleteIT.java ########## @@ -943,6 +961,163 @@ public void testDeleteFilterWithMultipleIndexes() throws Exception { } } } -} + /* + Tests whether we have cell tags in delete marker for + ClientSelectDeleteMutationPlan. + */ + @Test + public void testDeleteClientDeleteMutationPlan() throws Exception { + String tableName = generateUniqueName(); + String indexName = generateUniqueName(); + String tagValue = "customer-delete"; + String delete = "DELETE FROM " + tableName + " WHERE v1 = 'foo'"; + Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES); + // Add tag "customer-delete" to delete marker. + props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue); + + createAndUpsertTable(tableName, indexName, props); + // Make sure that the plan creates is of ClientSelectDeleteMutationPlan + verifyDeletePlan(delete, DeleteCompiler.ClientSelectDeleteMutationPlan.class, props); + executeDelete(delete, props, 1); + String startRowKeyForBaseTable = "1"; + String startRowKeyForIndexTable = "foo"; + // Make sure that Delete Marker has cell tag for base table + // and has no cell tag for index table. + checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue); + checkTagPresentInDeleteMarker(indexName, startRowKeyForIndexTable, false, null); + } + + /* + Tests whether we have cell tags in delete marker for + ServerSelectDeleteMutationPlan. + */ + @Test + public void testDeleteServerDeleteMutationPlan() throws Exception { + String tableName = generateUniqueName(); + String indexName = generateUniqueName(); + String tagValue = "customer-delete"; + String delete = "DELETE FROM " + tableName; + Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES); + props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue); + + createAndUpsertTable(tableName, indexName, props); + // Make sure that the plan creates is of ServerSelectDeleteMutationPlan + verifyDeletePlan(delete, DeleteCompiler.ServerSelectDeleteMutationPlan.class, props); + executeDelete(delete, props, 2); + + String startRowKeyForBaseTable = "1"; + String startRowKeyForIndexTable = "foo"; + // Make sure that Delete Marker has cell tag for base table + // and has no cell tag for index table. + checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue); + checkTagPresentInDeleteMarker(indexName, startRowKeyForIndexTable, false, null); + } + + /* + Tests whether we have cell tags in delete marker for + MultiRowDeleteMutationPlan. + */ + @Test + public void testDeleteMultiRowDeleteMutationPlan() throws Exception { + String tableName = generateUniqueName(); + String tagValue = "customer-delete"; + String delete = "DELETE FROM " + tableName + " WHERE k = 1"; + Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES); + props.setProperty(ConnectionQueryServices.SOURCE_OPERATION_ATTRIB, tagValue); + // Don't create index table. We will use MultiRowDeleteMutationPlan + // if there is no index present for a table. + createAndUpsertTable(tableName, null, props); + // Make sure that the plan creates is of MultiRowDeleteMutationPlan + verifyDeletePlan(delete, DeleteCompiler.MultiRowDeleteMutationPlan.class, props); + executeDelete(delete, props, 1); + String startRowKeyForBaseTable = "1"; + // Make sure that Delete Marker has cell tag for base table. + // We haven't created index table for this test case. + checkTagPresentInDeleteMarker(tableName, startRowKeyForBaseTable, true, tagValue); + } + + /* + Verify whether plan that we create for delete statement is of planClass + */ + private void verifyDeletePlan(String delete, Class planClass, Properties props) + throws SQLException { + try(Connection conn = DriverManager.getConnection(getUrl(), props)) { + conn.setAutoCommit(true); + PhoenixStatement stmt = conn.createStatement().unwrap(PhoenixStatement.class); + SQLParser parser = new SQLParser(delete); + DeleteStatement deleteStmt = (DeleteStatement) parser.parseStatement(); + DeleteCompiler compiler = new DeleteCompiler(stmt, null); + MutationPlan plan = compiler.compile(deleteStmt); + assertEquals(plan.getClass(), planClass); + } + } + private void createAndUpsertTable(String tableName, String indexName, Properties props) + throws SQLException { + String ddl = "CREATE TABLE " + tableName + + " (k INTEGER NOT NULL PRIMARY KEY, v1 VARCHAR, v2 VARCHAR)"; + try(Connection conn = DriverManager.getConnection(getUrl(), props)) { + conn.setAutoCommit(true); + try (Statement statement = conn.createStatement()) { + statement.execute(ddl); + if (indexName != null) { + String indexDdl1 = "CREATE INDEX " + indexName + " ON " + tableName + "(v1,v2)"; + statement.execute(indexDdl1); + } + } + conn.createStatement().execute( + "upsert into " + tableName + " values (1, 'foo', 'foo1')"); + conn.createStatement().execute( + "upsert into " + tableName + " values (2, 'bar', 'bar1')"); + conn.commit(); + } + } + + private void executeDelete(String delete, Properties props, int deleteRowCount) + throws SQLException { + try(Connection conn = DriverManager.getConnection(getUrl(), props)) { + conn.setAutoCommit(true); + try (Statement statement = conn.createStatement()) { + int rs = statement.executeUpdate(delete); + assertEquals( deleteRowCount, rs); + } + } + } + /* + Verify whether we have tags present for base table and not present for + index tables. + */ + private void checkTagPresentInDeleteMarker(String tableName, String startRowKey, + boolean tagPresent, String tagValue) throws IOException { + List<Cell> values = new ArrayList<>(); + TableName table = TableName.valueOf(tableName); + // Scan table with specified startRowKey + for (HRegion region : getUtility().getHBaseCluster().getRegions(table)) { + values.clear(); Review comment: nit: cleaning up entries might not be needed. If `values` has any record, we will be out of this loop. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org