afedulov commented on a change in pull request #18911: URL: https://github.com/apache/flink/pull/18911#discussion_r814259791
########## File path: flink-formats/flink-parquet/src/main/java/org/apache/flink/formats/parquet/avro/AvroParquetRecordFormat.java ########## @@ -42,7 +43,17 @@ import static org.apache.flink.util.Preconditions.checkArgument; -/** A reader format that reads individual Avro records from a Parquet stream. */ +/** + * A reader format that reads individual Avro records from a Parquet stream. This class leverages + * {@link ParquetReader} underneath. Developer should make sure the parquet files can be worked with Review comment: The/a developer? issueS Avro (upper case) The sentence is a bit unclear. Maybe remove "Developer" and just say that parquet files need to be in a certain way compatible with the Avro format? ########## File path: flink-formats/flink-parquet/src/main/java/org/apache/flink/formats/parquet/avro/AvroParquetRecordFormat.java ########## @@ -42,7 +43,17 @@ import static org.apache.flink.util.Preconditions.checkArgument; -/** A reader format that reads individual Avro records from a Parquet stream. */ +/** + * A reader format that reads individual Avro records from a Parquet stream. This class leverages + * {@link ParquetReader} underneath. Developer should make sure the parquet files can be worked with + * provided avro schema and take care of any further compatibility issue. + * + * <p>It is recommended to use the factory class {@link AvroParquetReaders} which is capable to Review comment: Suggestion: ``` For instantiation, it is recommended to use the factory class {@link AvroParquetReaders}. It is capable of creating versions of {@link AvroParquetRecordFormat} that can work with {@link GenericRecord GenericRecords}, {@link org.apache.avro.specific.SpecificRecord SpecificRecords}, or {@link org.apache.avro.reflect.ReflectData reflect records}. ``` But honestly, I would remove the second sentence, because this is that the user will see in the API of the AvroParquetReaders class. ########## File path: flink-formats/flink-parquet/src/main/java/org/apache/flink/formats/parquet/avro/AvroParquetReaders.java ########## @@ -30,9 +31,10 @@ import org.apache.avro.specific.SpecificRecordBase; /** - * Convenience builder to create {@link AvroParquetRecordFormat} instances for the different Avro + * Convenience factory to create {@link AvroParquetRecordFormat} instances for the different Avro Review comment: ```suggestion * A convenience factory to create {@link AvroParquetRecordFormat} instances for the different kinds of Avro records. ``` When I read "a convenience factory", I think about a full-blown factory pattern. This seems to be more of a holder for convenience static factory methods. But it is up to you, maybe it is a conventional naming that I do not know of. -- 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...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org