zhengxiang378928908-code commented on code in PR #18602:
URL: https://github.com/apache/iceberg/pull/18602#discussion_r4348393893


##########
flink/v2.3/flink/src/main/java/org/apache/iceberg/flink/sink/dynamic/DynamicRecordProcessor.java:
##########
@@ -160,14 +160,19 @@ public void collect(DynamicRecord inputData) {
             isForward);
       } else {
         // Shuffled records with immediateUpdate=false go to the update side 
output
+        // The schema, the spec and the row used for the key must describe the 
same layout: the
+        // spec resolves partition source fields by ID in the schema, and the 
schema reads them by
+        // position from the row. Use the table's layout only when both the 
table schema and spec
+        // are known, otherwise stay with the record's own schema, spec and 
row.
+        boolean keyByTable = foundSchema.resolvedTableSchema() != null && 
foundSpec != null;
         int writerKey =
-            hashKeyGenerator.generateKey(
-                data,
-                foundSchema.resolvedTableSchema() != null
-                    ? foundSchema.resolvedTableSchema()
-                    : data.schema(),
-                foundSpec != null ? foundSpec : data.spec(),
-                data.rowData());
+            keyByTable

Review Comment:
   Good point, done in 26500608b. The table schema and spec are only both known 
here when just the branch is missing, so the extra case wasn't worth a second 
conversion of the row. The 322 tests in `org.apache.iceberg.flink.sink.dynamic` 
still run with no failures (2 skipped).
   
   One small difference, for the record: when a record goes to a new branch and 
needs data conversion, the update path now keys it with the record's types, 
while `emit()` keys later records with the table's types. With equality-field 
distribution, an `int` key column written to a `long` table column hashes 
differently for negative values (`Integer.hashCode` vs `Long.hashCode`), so in 
upsert mode rows with the same key values may go to different writers, 
depending on the write parallelism. The same can already happen when a schema 
or spec update is needed. I haven't reproduced this end to end and didn't keep 
a special case for it.
   



-- 
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]

Reply via email to