[GitHub] [flink] RyanSkraba commented on a change in pull request #19024: [FLINK-26349][AvroParquet][test] add UT for reading reflect records from parquet file created with generic record schema.

[GitBox <gi...@apache.org>]

RyanSkraba commented on a change in pull request #19024:
URL: https://github.com/apache/flink/pull/19024#discussion_r822902236



##########
File path: flink-formats/flink-parquet/src/test/java/org/apache/flink/formats/parquet/avro/AvroParquetRecordFormatTest.java
##########
@@ -309,9 +328,16 @@ private static GenericRecord createUser(String name, int favoriteNumber, String
     }
 
     private void assertUserEquals(GenericRecord user, GenericRecord expected) {
-        assertEquals(user.get("name").toString(), expected.get("name"));
+        assertEquals(user.get("name").toString(), expected.get("name").toString());

Review comment:
       In practice here, the `get("name")` and `get("favoriteColor")` never return null, but according to the schema, the latter is nullable.
   
   It's probably not worth making a huge change here.  In Java 11, there's the `CharSequence.compare(...,...)` that might simplify this!  If you make a comment, though, I commit to fixing it when this module gets migrated to AssertJ :D 

##########
File path: flink-formats/flink-parquet/src/test/java/org/apache/flink/formats/parquet/avro/AvroParquetRecordFormatTest.java
##########
@@ -84,6 +84,7 @@ static void setup() throws IOException {
                         .parse(
                                 "{\"type\": \"record\", "
                                         + "\"name\": \"User\", "
+                                        + "\"namespace\": \"org.apache.flink.formats.parquet.avro.AvroParquetRecordFormatTest\", "

Review comment:
       :+1: This is the right way to point to a POJO by reflection.

##########
File path: flink-formats/flink-parquet/src/test/java/org/apache/flink/formats/parquet/avro/AvroParquetRecordFormatTest.java
##########
@@ -140,6 +141,24 @@ void testCreateReflectReader() throws IOException {
         }
     }
 
+    @Test
+    void testReflectReadFromGenericRecords() throws IOException {
+        StreamFormat.Reader<User> reader =
+                createReader(
+                        AvroParquetReaders.forReflectRecord(User.class),
+                        new Configuration(),
+                        userPath,
+                        0,
+                        userPath.getFileSystem().getFileStatus(userPath).getLen());
+        for (GenericRecord record : userRecords) {
+            User user = reader.read();
+            assertUserEquals(Objects.requireNonNull(user), record);
+        }
+    }
+
+    @Test
+    void testReflectReadFromGenericRecords2() throws IOException {}

Review comment:
       Is this intended?




-- 
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-unsubscribe@flink.apache.org

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