From 4e1cc58c2a73129a5d590c01e1e0040755e58248 Mon Sep 17 00:00:00 2001 From: David Stryker Date: Sat, 18 Sep 2021 17:22:19 -0700 Subject: [PATCH] Fix UPDATE with multiple identical values UPDATE failed when more than one column was updated to the same value. The root cause was confusion in LocalExecutionPlanner.createColumnValueAndRowIdChannels(). This commit fixes that confusion and removes an comment declaring incorrectly that the code depended on order of columns in the page - - instead, the code is looking up the channel by Symbol. This commit adds the test contained in the original bug report, as well as a test for identical update values for multiple columns. --- .../sql/planner/LocalExecutionPlanner.java | 12 ++-- .../hive/TestHiveTransactionalTable.java | 62 +++++++++++++++++++ 2 files changed, 66 insertions(+), 8 deletions(-) diff --git a/core/trino-main/src/main/java/io/trino/sql/planner/LocalExecutionPlanner.java b/core/trino-main/src/main/java/io/trino/sql/planner/LocalExecutionPlanner.java index 043164f8ec81..2ac240de80e2 100644 --- a/core/trino-main/src/main/java/io/trino/sql/planner/LocalExecutionPlanner.java +++ b/core/trino-main/src/main/java/io/trino/sql/planner/LocalExecutionPlanner.java @@ -3049,16 +3049,12 @@ private List createColumnValueAndRowIdChannels(List outputSymbo { Integer[] columnValueAndRowIdChannels = new Integer[columnValueAndRowIdSymbols.size()]; int symbolCounter = 0; - // This depends on the outputSymbols being ordered as the blocks of the - // resulting page are ordered. - for (Symbol symbol : outputSymbols) { - int index = columnValueAndRowIdSymbols.indexOf(symbol); - if (index >= 0) { - columnValueAndRowIdChannels[index] = symbolCounter; - } + for (Symbol symbol : columnValueAndRowIdSymbols) { + int index = outputSymbols.indexOf(symbol); + verify(index >= 0, "Could not find symbol %s in the outputSymbols %s", symbol, outputSymbols); + columnValueAndRowIdChannels[symbolCounter] = index; symbolCounter++; } - checkArgument(symbolCounter == columnValueAndRowIdSymbols.size(), "symbolCounter %s should be columnValueAndRowIdChannels.size() %s", symbolCounter); return Arrays.asList(columnValueAndRowIdChannels); } diff --git a/testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveTransactionalTable.java b/testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveTransactionalTable.java index 2e5a8c419dd3..16e6be07a275 100644 --- a/testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveTransactionalTable.java +++ b/testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveTransactionalTable.java @@ -34,6 +34,7 @@ import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; +import java.sql.Date; import java.time.Instant; import java.time.temporal.ChronoUnit; import java.util.Collections; @@ -1432,6 +1433,67 @@ public void testAcidUpdateWithSubqueryAssignment() }); } + @Test(groups = HIVE_TRANSACTIONAL, timeOut = TEST_TIMEOUT) + public void testAcidUpdateDuplicateUpdateValue() + { + withTemporaryTable("test_update_bug", true, false, NONE, tableName -> { + onTrino().executeQuery( + format("CREATE TABLE %s (", tableName) + + " yyyy integer," + + " week_number integer," + + " week_start_date date," + + " week_end_date date," + + " genre_summary_status smallint," + + " shop_summary_status smallint)" + + " WITH (transactional = true)"); + + onTrino().executeQuery(format("INSERT INTO %s", tableName) + + "(yyyy, week_number, week_start_date, week_end_date, genre_summary_status, shop_summary_status) VALUES" + + " (2021, 20, DATE '2021-09-10', DATE '2021-09-10', 20, 20)" + + ", (2021, 30, DATE '2021-09-09', DATE '2021-09-09', 30, 30)" + + ", (2021, 999, DATE '2018-12-24', DATE '2018-12-24', 999, 999)" + + ", (2021, 30, DATE '2021-09-09', DATE '2021-09-10', 30, 30)"); + + onTrino().executeQuery(format("UPDATE %s", tableName) + + " SET genre_summary_status = 1, shop_summary_status = 1" + + " WHERE week_start_date = DATE '2021-09-19' - (INTERVAL '08' DAY)" + + " OR week_start_date = DATE '2021-09-19' - (INTERVAL '09' DAY)"); + + assertThat(onTrino().executeQuery("SELECT * FROM " + tableName)) + .contains( + row(2021, 20, Date.valueOf("2021-09-10"), Date.valueOf("2021-09-10"), 1, 1), + row(2021, 30, Date.valueOf("2021-09-09"), Date.valueOf("2021-09-09"), 30, 30), + row(2021, 999, Date.valueOf("2018-12-24"), Date.valueOf("2018-12-24"), 999, 999), + row(2021, 30, Date.valueOf("2021-09-09"), Date.valueOf("2021-09-10"), 30, 30)); + }); + } + + @Test(groups = HIVE_TRANSACTIONAL, timeOut = TEST_TIMEOUT) + public void testAcidUpdateMultipleDuplicateValues() + { + withTemporaryTable("test_update_multiple", true, false, NONE, tableName -> { + onTrino().executeQuery( + format("CREATE TABLE %s (c1 int, c2 int, c3 int, c4 int, c5 int, c6 int) WITH (transactional = true)", tableName)); + + log.info("Inserting into table"); + onTrino().executeQuery(format("INSERT INTO %s (c1, c2, c3, c4, c5, c6) VALUES (1, 2, 3, 4, 5, 6)", tableName)); + + log.info("Performing first update"); + onTrino().executeQuery(format("UPDATE %s SET c1 = 2, c2 = 4, c3 = 2, c4 = 4, c5 = 4, c6 = 2", tableName)); + + log.info("Checking first results"); + assertThat(onTrino().executeQuery("SELECT * FROM " + tableName)).contains(row(2, 4, 2, 4, 4, 2)); + + log.info("Performing second update"); + onTrino().executeQuery(format("UPDATE %s SET c1 = c1 + c2, c2 = c3 + c4, c3 = c1 + c2, c4 = c4 + c3, c5 = c3 + c4, c6 = c4 + c3", tableName)); + + log.info("Checking second results"); + assertThat(onTrino().executeQuery("SELECT * FROM " + tableName)).contains(row(6, 6, 6, 6, 6, 6)); + + log.info("Finished"); + }); + } + @Flaky(issue = ERROR_COMMITTING_WRITE_TO_HIVE_ISSUE, match = ERROR_COMMITTING_WRITE_TO_HIVE_MATCH) @Test(groups = HIVE_TRANSACTIONAL, timeOut = TEST_TIMEOUT) public void testInsertDeleteUpdateWithTrinoAndHive()