[GitHub] [flink] AHeise commented on a change in pull request #15156: [FLINK-21393] [formats] Implement ParquetAvroInputFormat

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

AHeise commented on a change in pull request #15156:
URL: https://github.com/apache/flink/pull/15156#discussion_r636816618



##########
File path: flink-formats/flink-avro/src/main/java/org/apache/flink/formats/avro/AvroRowSerializationSchema.java
##########
@@ -157,7 +157,7 @@ public int hashCode() {
 
     // --------------------------------------------------------------------------------------------
 
-    private GenericRecord convertRowToAvroRecord(Schema schema, Row row) {
+    public GenericRecord convertRowToAvroRecord(Schema schema, Row row) {

Review comment:
       I think we should factor that to some utility class instead. It feels unnatural that the `ParquetInputFormat` needs to have an instance of `AvroRowSerializationSchema` and we need to expose this previously private method across packages. However, if we need state for this method, I can be persuaded to keep as is.

##########
File path: flink-formats/flink-parquet/pom.xml
##########
@@ -45,6 +45,12 @@ under the License.
 			<scope>provided</scope>
 		</dependency>
 
+		<dependency>
+			<groupId>org.apache.flink</groupId>

Review comment:
       I was a bit worried that all users that use parquet now also get the Avro dependencies, but as far as I can see they already have all avro dependencies % flink-avro (128 kb) already. Can you double-check if this is true?

##########
File path: flink-formats/flink-parquet/src/test/java/org/apache/flink/formats/parquet/ParquetAvroInputFormatTest.java
##########
@@ -0,0 +1,111 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.formats.parquet;
+
+import org.apache.flink.api.java.tuple.Tuple3;
+import org.apache.flink.core.fs.FileInputSplit;
+import org.apache.flink.core.fs.Path;
+import org.apache.flink.formats.parquet.utils.TestUtil;
+import org.apache.flink.types.Row;
+
+import org.apache.avro.AvroRuntimeException;
+import org.apache.avro.generic.GenericRecord;
+import org.apache.avro.specific.SpecificRecord;
+import org.apache.parquet.schema.MessageType;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.List;
+
+import static org.junit.Assert.assertArrayEquals;
+import static org.junit.Assert.assertEquals;
+
+/** Test cases for reading Parquet files and convert parquet records to Avro GenericRecords. */
+@RunWith(Parameterized.class)

Review comment:
       Why is this parameterized? Is this test then even executed on azure?

##########
File path: flink-formats/flink-parquet/src/test/java/org/apache/flink/formats/parquet/ParquetAvroInputFormatTest.java
##########
@@ -0,0 +1,111 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.formats.parquet;
+
+import org.apache.flink.api.java.tuple.Tuple3;
+import org.apache.flink.core.fs.FileInputSplit;
+import org.apache.flink.core.fs.Path;
+import org.apache.flink.formats.parquet.utils.TestUtil;
+import org.apache.flink.types.Row;
+
+import org.apache.avro.AvroRuntimeException;
+import org.apache.avro.generic.GenericRecord;
+import org.apache.avro.specific.SpecificRecord;
+import org.apache.parquet.schema.MessageType;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.List;
+
+import static org.junit.Assert.assertArrayEquals;
+import static org.junit.Assert.assertEquals;
+
+/** Test cases for reading Parquet files and convert parquet records to Avro GenericRecords. */
+@RunWith(Parameterized.class)
+public class ParquetAvroInputFormatTest extends TestUtil {

Review comment:
       Please change to static imports. I know this is how the other tests are doing it, but it's bad practice. If it's indeed a larger change, please leave as-is.




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

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