PHOENIX-300 Add support for TRUNCATE TABLE #1409
Conversation
ragarkar
commented
Mar 24, 2022
ragarkar
commented
- Added support for TRUNCATE TABLE statement in phoenix.
- Added an integration test to test the feature.
|
Please do not use private JIRA IDs in the Apache project. |
|
There are a few issues to be addressed:
|
|
You are preserving the splits during truncate, which is good, but you may also want to consider exposing an option that lets users remove the existing split points. (not for salted tables, of course.) |
|
Thanks for looking into these changes. How about we provide an extension to the TRUNCATE TABLE statement to preserve the splits? We can have an optional clause something like: TRUNCATE TABLE PRESERVE SPLITS; |
|
That sounds good, @ragarkar . |
|
Or rather make preserving splits the default, and add an option to NOT preserve them. |
| TableName hbaseTableName = SchemaUtil.getPhysicalTableName(SchemaUtil.getTableName(schemaName, tableName).getBytes(), isNamespaceMapped); | ||
| try { | ||
| Admin admin = getAdmin(); | ||
| admin.disableTable(hbaseTableName); |
There was a problem hiding this comment.
Truncate table automatically disables the table we need not do it explicitly.
| try { | ||
| Admin admin = getAdmin(); | ||
| admin.disableTable(hbaseTableName); | ||
| admin.truncateTable(hbaseTableName, true); |
There was a problem hiding this comment.
Have you checked the truncate table always waiting for the table to get created and enabled? If not we might need to add retries in case if required.
| try { | ||
| Admin admin = getAdmin(); | ||
| admin.disableTable(hbaseTableName); | ||
| admin.truncateTable(hbaseTableName, true); |
There was a problem hiding this comment.
After the truncate table the regions cache in hbase is not longer valid so would be better to call
ConnectionQueryServicesImpl#clearTableRegionCache after that so that further queries fetch new locations of table regions.
| ? connection.getSchema() : statement.getTableName().getSchemaName(); | ||
| String tableName = statement.getTableName().getTableName(); | ||
| boolean isNamespaceMapped = SchemaUtil.isNamespaceMappingEnabled(statement.getTableType(), connection.getQueryServices().getProps()); | ||
| boolean wasAutoCommit = connection.getAutoCommit(); |
There was a problem hiding this comment.
Since we are not doing any meta update changes here changing getting autocommit status and setting back is not much impact so better to remove it.
| import org.junit.experimental.categories.Category; | ||
|
|
||
| @Category(ParallelStatsDisabledTest.class) | ||
| public class TruncateTableIT extends ParallelStatsDisabledIT { |
There was a problem hiding this comment.
You can add a test case creating tables with split keys which is missing.
There was a problem hiding this comment.
You can add the code to check the number of regions after truncate also.
| conn.close(); | ||
|
|
||
| conn = DriverManager.getConnection(getUrl(), props); | ||
| conn.setAutoCommit(true); |
There was a problem hiding this comment.
This is by default not required.
| stmt.execute(); | ||
| conn.close(); | ||
|
|
||
| conn = DriverManager.getConnection(getUrl(), props); |
There was a problem hiding this comment.
Need not recreate the connection every time. Can make use the same connection and close it at the end of test case.
|
|
||
| @Test | ||
| public void testTruncateTableNotExist() throws Exception { | ||
| Properties props = new Properties(); |
There was a problem hiding this comment.
Need not pass empty properties, can call directly DriverManager.getConnection(getUrl())
There was a problem hiding this comment.
Nice feature @ragarkar . Thanks for the patch, this has been wanted by the community for a long time. Some thoughts below on polishing it and adding some extra safety
| @Test | ||
| public void testTruncateTableNotExist() throws Exception { | ||
| Properties props = new Properties(); | ||
| Connection conn = DriverManager.getConnection(getUrl(), props); |
There was a problem hiding this comment.
Please create the connection using try-with-resources to make sure that it gets closed at the end of the test.
| @Test | ||
| public void testTruncateTable() throws SQLException { | ||
| Properties props = new Properties(); | ||
| Connection conn = DriverManager.getConnection(getUrl(), props); |
There was a problem hiding this comment.
Best to create the connection using try-with-resources
| rs = conn.createStatement().executeQuery("SELECT COUNT(*) FROM " + tableName); | ||
| assertTrue(rs.next()); | ||
| assertEquals(0, rs.getInt(1)); | ||
| conn.createStatement().execute("DROP TABLE " + tableName); |
There was a problem hiding this comment.
Consider dropping the table in a finally block
| conn.createStatement().execute("TRUNCATE TABLE " + schemaName + "." + tableName); | ||
| fail(); | ||
| } catch (SQLException e) { | ||
| conn.createStatement().execute("DROP TABLE " + tableName); |
There was a problem hiding this comment.
Good to assert on the actual SQLException you're expecting.
| @Test | ||
| public void testTruncateTableWithExplicitSchema() throws SQLException { | ||
| Properties props = new Properties(); | ||
| props.setProperty(QueryServices.IS_NAMESPACE_MAPPING_ENABLED, Boolean.toString(true)); |
There was a problem hiding this comment.
A given Phoenix cluster can only have Namespace mapping enabled or disabled; it can't be enabled for some tables and disabled for others. This usually means either having to shut down the minicluster and spin it back up with namespace enabled, or putting this is in a separate IT test.
| // Parse a truncate table statement. | ||
| truncate_table_node returns [TruncateTableStatement ret] | ||
| : TRUNCATE TABLE t=from_table_name | ||
| {ret = factory.truncateTable(t, PTableType.TABLE);} |
There was a problem hiding this comment.
What happens if someone tries to truncate a view? Looks like we're assuming that from_table_name is a table, but there's nothing in the grammar to enforce that, is there?
| throw e; | ||
| } | ||
| try { | ||
| connection.getQueryServices().truncateTable(schemaName, tableName, isNamespaceMapped); |
There was a problem hiding this comment.
There should be a check in here to make sure that we don't try to truncate a SYSTEM table, and if we do, throw a SQLException with a new SQLExceptionCode explaining we don't allow that.
There was a problem hiding this comment.
Likewise, I don't think you should be able to truncate an index directly (but an index should get truncated if its base table gets truncated, as I think @chrajeshbabu pointed out.)
ragarkar
changed the title
