[GitHub] [parquet-mr] matthieun commented on a diff in pull request #988: PARQUET-1711: Break circular dependencies in proto definitions

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

matthieun commented on code in PR #988:
URL: https://github.com/apache/parquet-mr/pull/988#discussion_r951644453


##########
parquet-protobuf/src/main/java/org/apache/parquet/proto/ProtoSchemaConverter.java:
##########
@@ -79,12 +80,20 @@ public MessageType convert(Class<? extends Message> protobufClass) {
   }
 
   /* Iterates over list of fields. **/
-  private <T> GroupBuilder<T> convertFields(GroupBuilder<T> groupBuilder, List<FieldDescriptor> fieldDescriptors) {
+  private <T> GroupBuilder<T> convertFields(GroupBuilder<T> groupBuilder, List<FieldDescriptor> fieldDescriptors, List<String> parentNames) {
     for (FieldDescriptor fieldDescriptor : fieldDescriptors) {
-      groupBuilder =
-          addField(fieldDescriptor, groupBuilder)
+      final String name = fieldDescriptor.getFullName();
+      final List<String> newParentNames = new ArrayList<>(parentNames);
+      newParentNames.add(name);
+      if (parentNames.contains(name)) {

Review Comment:
   The list is mostly used to keep the ordering, so that the dependency chain is printed in order in the warning message. I understand that in case the schema definition is really deep with nested types it might be slower, but overall that list is not growing any bigger than the deepest nesting in the schema.
   If this is still a concern, I am happy to switch to HashSet at the expense of maybe dumming down the log message (printing the nesting chain out of order would not be valuable anyway I think).



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

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