aihuaxu commented on code in PR #12238:
URL: https://github.com/apache/iceberg/pull/12238#discussion_r1974489717
##########
core/src/main/java/org/apache/iceberg/avro/AvroCustomOrderSchemaVisitor.java:
##########
@@ -27,13 +27,20 @@
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
abstract class AvroCustomOrderSchemaVisitor<T, F> {
+ private static final String METADATA = "metadata";
+
public static <T, F> T visit(Schema schema, AvroCustomOrderSchemaVisitor<T,
F> visitor) {
switch (schema.getType()) {
case RECORD:
// check to make sure this hasn't been visited before
String name = schema.getFullName();
Preconditions.checkState(
!visitor.recordLevels.contains(name), "Cannot process recursive
Avro record %s", name);
+ Preconditions.checkArgument(
Review Comment:
Make sense. I think we don't need `visitor.recordLevels.push(name)` and
`visitor.recordLevels.pop();` for visiting Variant fields since we know they
are not records.
BTW: seems we should change the following
```
visitor.recordLevels.pop();
return visitor.record(schema, names, Iterables.transform(results,
Supplier::get));
```
to
```
visitor.recordLevels.push(name);
...
Iterable<> itFields = Iterables.transform(results, Supplier::get);
visitor.recordLevels.pop();
return visitor.record(schema, names, itFields);
```
in order to fail if there is any recursive nodes in Avro schema.
--
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...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
