Fix bug when DELETE ACID block is a DictionaryBlock #9354
Conversation
| @@ -116,28 +117,31 @@ public HiveUpdatablePageSource( | |||
| @Override | |||
| public void deleteRows(Block rowIds) | |||
| { | |||
| List<Block> blocks = rowIds.getChildren(); | |||
| checkArgument(blocks.size() == 3, "The rowId block for DELETE should have 3 children, but has %s", blocks.size()); | |||
There was a problem hiding this comment.
Should this check survive in the form of columnarRow.getFieldCount()==3 inside deleteRowsInternal?
| List<Block> blocks = rowIds.getChildren(); | ||
| ColumnarRow columnarRow = ColumnarRow.toColumnarRow(rowIds); | ||
| for (int position = 0; position < positionCount; position++) { | ||
| // Throw an exception if there are any null rows |
There was a problem hiding this comment.
redundant
| // Throw an exception if there are any null rows |
| ColumnarRow columnarRow = ColumnarRow.toColumnarRow(rowIds); | ||
| for (int position = 0; position < positionCount; position++) { | ||
| // Throw an exception if there are any null rows | ||
| checkArgument(!columnarRow.isNull(position), "In the deleteRows block, found null position %s", position); |
There was a problem hiding this comment.
deleteRows is not a variable or anything else known
| checkArgument(!columnarRow.isNull(position), "In the deleteRows block, found null position %s", position); | |
| checkArgument(!columnarRow.isNull(position), "In the delete rowIds, found null row at position %s", position); |
| @@ -832,6 +832,28 @@ public void testDeleteAllRowsInPartition() | |||
| }); | |||
| } | |||
|
|
|||
| @Test(groups = HIVE_TRANSACTIONAL, timeOut = TEST_TIMEOUT) | |||
| public void testDeleteAllRowsUnpartitioned() | |||
There was a problem hiding this comment.
i think the important aspect is "delete after delete".
assuming it's correct, let's reflect this in a test name: testDeleteAfterDelete
There was a problem hiding this comment.
Good idea; renamed.
| log.info("About to delete all rows"); | ||
| onTrino().executeQuery("DELETE FROM " + tableName); | ||
|
|
||
| verifySelectForTrinoAndHive("SELECT COUNT(*) FROM " + tableName, "TRUE", row(0)); |
| onTrino().executeQuery(format("DELETE FROM %s WHERE id = 2", tableName)); | ||
|
|
||
| log.info("About to verify"); | ||
| verifySelectForTrinoAndHive("SELECT * FROM " + tableName, "TRUE", row(1), row(3)); |
There was a problem hiding this comment.
verification of this stage is a bit redundant (we already have some tests for delete), especially if we run this on Hive too. remove, or keep only on Trino.
There was a problem hiding this comment.
Changed to test only on Trino.
| verifySelectForTrinoAndHive("SELECT * FROM " + tableName, "TRUE", row(1), row(3)); | ||
|
|
||
| log.info("About to delete all rows"); | ||
| onTrino().executeQuery("DELETE FROM " + tableName); |
There was a problem hiding this comment.
Was the bug triggered for DELETE without any predicate, or for a delete with a predicate too?
we should test both cases
There was a problem hiding this comment.
Good idea. I added a test that differs in this way:
// A predicate sufficient to fool statistics-based optimization
onTrino().executeQuery(format("DELETE FROM %s WHERE id != 2" + tableName));
| withTemporaryTable("delete_all_rows", true, false, NONE, tableName -> { | ||
| onTrino().executeQuery(format("CREATE TABLE %s (id INT) WITH (transactional = true)", tableName)); | ||
|
|
||
| log.info("About to insert"); |
There was a problem hiding this comment.
There was a problem hiding this comment.
All logging removed from this method.
|
cc @losipiuk |
djsstarburst
force-pushed
the
david.stryker/fix-delete-all
branch
from
3fac599
to
6a9ca50
Compare
|
Thanks for the prompt, close review, @findepi. I force pushed an update making all your suggested changes. BTW, separating the WHERE clause out as a separate argument to the |
Before this commit, Block.getChildren() was used to take apart the DELETE ACID rowId block. That is always the wrong thing to do. Fixed by using ColumnarRow to access the elementa of the ACID rowId block. This commit also adds a comment warning developers not to use Block.getChildren() so others don't make the same mistake of calling the method.
djsstarburst
force-pushed
the
david.stryker/fix-delete-all
branch
from
6a9ca50
to
4d85258
Compare
Replace use of Block.getChildren in HiveUpdatablePageSource and HiveUpdateProcessor with ColumnarRow methods.
|
CI #8432 |
|
Merged as 87785b4, thanks! |
This PR consists of two commits:
Before the first commit,
Block.getChildren()was used to take apart theDELETE ACID rowId block. That is always the wrong thing to do.
Fixed by using
ColumnarRowto access the elements of the ACID rowId block.This first commit also adds a comment warning developers not to use
Block.getChildren()so others don't make the same mistake ofcalling the method.
The second commit replaces use of Block.getChildren in
HiveUpdateProcessor with ColumnarRow methods.