ggershinsky commented on a change in pull request #950:
URL: https://github.com/apache/parquet-mr/pull/950#discussion_r830823180
##########
File path:
parquet-column/src/main/java/org/apache/parquet/filter2/predicate/SchemaCompatibilityValidator.java
##########
@@ -170,6 +174,24 @@ public Void visit(Not not) {
private <T extends Comparable<T>> void validateColumn(Column<T> column) {
ColumnPath path = column.getColumnPath();
+ HashSet<Type.ID> ids = new HashSet<>();
Review comment:
nit: this can be introduced after the `if`
##########
File path:
parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetOutputFormat.java
##########
@@ -472,7 +488,13 @@ public static boolean
getPageWriteChecksumEnabled(Configuration conf) {
LOG.debug("Parquet properties are:\n{}", props);
WriteContext fileWriteContext = writeSupport.init(conf);
-
+
+ MessageType schema = fileWriteContext.getSchema();
+ HashSet<Type.ID> ids = new HashSet<>();
Review comment:
this variable is not used outside `checkDuplicateId`; is there a way to
hide it?
##########
File path:
parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetFileReader.java
##########
@@ -878,11 +880,92 @@ public String getFile() {
return blocks;
}
- public void setRequestedSchema(MessageType projection) {
+ private boolean uniqueId(GroupType schema, HashSet<Type.ID> ids) {
+ boolean unique = true;
+ List<Type> fields = schema.getFields();
+ for (Type field : fields) {
+ if (field instanceof PrimitiveType) {
+ Type.ID id = field.getId();
+ if (id != null) {
+ if (ids.contains(id)) {
+ return false;
+ }
+ ids.add(id);
+ }
+ }
+
+ if (field instanceof GroupType) {
+ Type.ID id = field.getId();
+ if (id != null) {
+ if (ids.contains(id)) {
+ return false;
+ }
+ ids.add(id);
+ }
+ if (unique) unique = uniqueId(field.asGroupType(), ids);
+ }
+ }
+ return unique;
+ }
+
+ public MessageType setRequestedSchema(MessageType projection) {
paths.clear();
- for (ColumnDescriptor col : projection.getColumns()) {
+ HashSet<Type.ID> ids = new HashSet<>();
Review comment:
this variable is not used outside `uniqueId`; is there a way to hide it?
##########
File path:
parquet-hadoop/src/main/java/org/apache/parquet/hadoop/InternalParquetRecordReader.java
##########
@@ -181,7 +181,7 @@ public void initialize(ParquetFileReader reader,
ParquetReadOptions options) {
this.columnCount = requestedSchema.getPaths().size();
// Setting the projection schema before running any filtering (e.g.
getting filtered record count)
// because projection impacts filtering
- reader.setRequestedSchema(requestedSchema);
+ this.requestedSchema = reader.setRequestedSchema(requestedSchema);
Review comment:
`this.requestedSchema` is set in the line 180; so this line overrides
the class field setting. Maybe you can use a temp local variable (with
appropriate name) in the line 180
--
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: dev-unsubscr...@parquet.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
