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
