pvary commented on code in PR #9212:
URL: https://github.com/apache/iceberg/pull/9212#discussion_r1420131007


##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/sink/shuffle/TestDataStatisticsOperator.java:
##########
@@ -119,9 +121,9 @@ public void testProcessElement() throws Exception {
         testHarness = createHarness(this.operator)) {
       StateInitializationContext stateContext = getStateContext();
       operator.initializeState(stateContext);
-      operator.processElement(new 
StreamRecord<>(GenericRowData.of(StringData.fromString("a"))));
-      operator.processElement(new 
StreamRecord<>(GenericRowData.of(StringData.fromString("a"))));
-      operator.processElement(new 
StreamRecord<>(GenericRowData.of(StringData.fromString("b"))));
+      operator.processElement(new StreamRecord<>(genericRowDataA));
+      operator.processElement(new StreamRecord<>(genericRowDataA));

Review Comment:
   Sorry - my midnight review was not clear enough. 😢 
   
   I was trying to suggest to process a different `RowData` object, with the 
same key, like:
   ```
   private final GenericRowData genericRowDataA_1 = 
GenericRowData.of(StringData.fromString("a"), 1);
   private final GenericRowData genericRowDataA_2 = 
GenericRowData.of(StringData.fromString("a"), 2);
   private final GenericRowData genericRowDataB = 
GenericRowData.of(StringData.fromString("b"), 3);
   [..]
   operator.processElement(new StreamRecord<>(genericRowDataA_1));
   operator.processElement(new StreamRecord<>(genericRowDataA_2));
   operator.processElement(new StreamRecord<>(genericRowDataB));
   ```
   
   I know that we have individual test for the correct grouping, but I consider 
this as an e2e tests, and it would be nice to test this out as well.



##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/sink/shuffle/TestDataStatisticsOperator.java:
##########
@@ -119,9 +121,9 @@ public void testProcessElement() throws Exception {
         testHarness = createHarness(this.operator)) {
       StateInitializationContext stateContext = getStateContext();
       operator.initializeState(stateContext);
-      operator.processElement(new 
StreamRecord<>(GenericRowData.of(StringData.fromString("a"))));
-      operator.processElement(new 
StreamRecord<>(GenericRowData.of(StringData.fromString("a"))));
-      operator.processElement(new 
StreamRecord<>(GenericRowData.of(StringData.fromString("b"))));
+      operator.processElement(new StreamRecord<>(genericRowDataA));
+      operator.processElement(new StreamRecord<>(genericRowDataA));

Review Comment:
   Sorry - my midnight review was not clear enough. 😢 
   
   I was trying to suggest to process a different `RowData` object, with the 
same key, like:
   ```
   private final GenericRowData genericRowDataA_1 = 
GenericRowData.of(StringData.fromString("a"), 1);
   private final GenericRowData genericRowDataA_2 = 
GenericRowData.of(StringData.fromString("a"), 2);
   private final GenericRowData genericRowDataB = 
GenericRowData.of(StringData.fromString("b"), 3);
   [..]
   operator.processElement(new StreamRecord<>(genericRowDataA_1));
   operator.processElement(new StreamRecord<>(genericRowDataA_2));
   operator.processElement(new StreamRecord<>(genericRowDataB));
   ```
   
   I know that we have individual test for the correct grouping, but I consider 
this as an e2e tests, and it would be nice to test this out as well.



-- 
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: issues-unsubscr...@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org

Reply via email to