Copilot commented on code in PR #6794:
URL: https://github.com/apache/hive/pull/6794#discussion_r4160878646
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/rewrite/sql/COWWithClauseBuilder.java:
##########
@@ -50,6 +53,8 @@ public void appendWith(MultiInsertSqlGenerator sqlGenerator,
String sourceName,
sqlGenerator.removeLastChar();
addSourceColumnsForRowLineage(isRowLineageSupported, sqlGenerator,
rowLineagePrefix, sqlGenerator.conf);
sqlGenerator.append(", row_number() OVER (partition by
").append(filePathCol).append(") rn");
+ sqlGenerator.append(", count(*) OVER (partition by
").append(filePathCol).append(") ")
+ .append(matchedRowCountCol);
Review Comment:
This builder is also called by `CopyOnWriteDeleteRewriter`, so the new count
window and negative count marker change standalone CoW DELETE reporting and
execution plans as well. That user-facing behavior is outside the stated
UPDATE/MERGE scope and has no operation-status assertion. Either gate count
encoding to the intended operations, or explicitly document the DELETE change
and add a DELETE row-count regression.
##########
iceberg/iceberg-handler/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergCRUD.java:
##########
@@ -441,6 +444,67 @@ public void testUpdateStatementUnpartitioned() throws
TException, InterruptedExc
HiveIcebergTestUtils.valueForRow(HiveIcebergStorageHandlerTestUtils.CUSTOMER_SCHEMA,
objects), 0);
}
+ @Test
+ public void testCopyOnWriteUpdateReportsOnlyMatchedRowCount() throws
IOException {
+ Assume.assumeTrue(formatVersion == 2);
+
+ TableIdentifier identifier = TableIdentifier.of("default",
"cow_update_count");
+ shell.executeStatement("CREATE EXTERNAL TABLE " + identifier + " (a int, b
string) " +
+ "STORED BY ICEBERG " +
+ testTables.locationForCreateTableSQL(identifier) +
+ "TBLPROPERTIES ('" + InputFormatConfig.TABLE_SCHEMA + "'='" +
+ SchemaParser.toJson(new Schema(
+ optional(1, "a", Types.IntegerType.get()),
+ optional(2, "b", Types.StringType.get()))) + "', " +
+ "'" + InputFormatConfig.PARTITION_SPEC + "'='" +
+ PartitionSpecParser.toJson(PartitionSpec.unpartitioned()) + "', " +
+ "'write.update.mode'='copy-on-write', " +
+ "'" + InputFormatConfig.EXTERNAL_TABLE_PURGE + "'='TRUE', " +
+ "'" + InputFormatConfig.CATALOG_NAME + "'='" +
testTables.catalogName() + "')");
+
+ shell.executeStatement("INSERT INTO " + identifier +
+ " VALUES (1, 'one'), (2, 'two'), (3, 'three'), (4, 'four'), (5,
'five')");
+
+ long numModifiedRows = shell.executeStatementAndGetNumModifiedRows(
+ "UPDATE " + identifier + " SET b = 'changed' WHERE a IN (2, 4)");
+
+ Assert.assertEquals(2, numModifiedRows);
+ }
+
+ @Test
+ public void testCopyOnWriteMergeReportsMatchedRowCount() throws IOException {
+ Assume.assumeTrue(formatVersion == 2);
+
+ TableIdentifier identifier = TableIdentifier.of("default",
"cow_merge_count");
+ shell.executeStatement("CREATE EXTERNAL TABLE " + identifier + " (a int, b
string) " +
+ "STORED BY ICEBERG " +
+ testTables.locationForCreateTableSQL(identifier) +
+ "TBLPROPERTIES ('" + InputFormatConfig.TABLE_SCHEMA + "'='" +
+ SchemaParser.toJson(new Schema(
+ optional(1, "a", Types.IntegerType.get()),
+ optional(2, "b", Types.StringType.get()))) + "', " +
+ "'" + InputFormatConfig.PARTITION_SPEC + "'='" +
+ PartitionSpecParser.toJson(PartitionSpec.unpartitioned()) + "', " +
+ "'write.merge.mode'='copy-on-write', " +
+ "'" + InputFormatConfig.EXTERNAL_TABLE_PURGE + "'='TRUE', " +
+ "'" + InputFormatConfig.CATALOG_NAME + "'='" +
testTables.catalogName() + "')");
+
+ shell.executeStatement("CREATE TABLE cow_merge_count_source (a int, b
string)");
+
+ shell.executeStatement("INSERT INTO " + identifier +
+ " VALUES (1, 'a'), (2, 'b'), (111, 'del')");
+ shell.executeStatement("INSERT INTO cow_merge_count_source " +
+ "VALUES (1, 'a'), (2, 'b'), (3, 'c'), (4, 'd'), (111, 'del')");
+
+ long numModifiedRows = shell.executeStatementAndGetNumModifiedRows(
+ "MERGE INTO " + identifier + " AS t USING cow_merge_count_source src
ON t.a = src.a " +
+ "WHEN MATCHED AND t.a = 111 THEN DELETE " +
+ "WHEN MATCHED THEN UPDATE SET b = 'merged' " +
+ "WHEN NOT MATCHED THEN INSERT VALUES (src.a, src.b)");
+
+ Assert.assertEquals(3, numModifiedRows);
Review Comment:
This assertion establishes a different user-facing contract from both the PR
description and the new interface: the statement performs 2 updates, 1 delete,
and 2 inserts; the description says to report the 2 matched updates,
`AffectedRowsProvidingRecordWriter` promises logical affected rows (which
suggests 5), while this test locks in 3 matched target rows. Please define one
`numModifiedRows` semantic and align the generator/writer, interface
documentation, PR description, and test expectation.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
