[GitHub] [iceberg] yyanyy commented on a change in pull request #2096: Core: add schema id and schemas to table metadata

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

yyanyy commented on a change in pull request #2096:
URL: https://github.com/apache/iceberg/pull/2096#discussion_r560442043



##########
File path: core/src/main/java/org/apache/iceberg/TableMetadataParser.java
##########
@@ -242,6 +255,7 @@ public static TableMetadata read(FileIO io, InputFile file) {
     }
   }
 
+  @SuppressWarnings("checkstyle:CyclomaticComplexity")

Review comment:
       Yeah I imagine there would be some conflict when either of the PRs got merged. I think the main problem with refactoring this is that a lot of the things it parsed should be parsed together/validate each other; e.g. parse of schema array has to come together with current schema id since both will either present at the same time, or derived from a different field, and same goes for sort order/partition spec. Since Java only allows returning a single object per method, we either have to refactor the code a bit and ensure the logic flow doesn't change and is readable, or let the new methods to return things like `tuple<Integer, List<Schema>>` that I'm not sure which way is cleaner. 




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



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org