rdblue commented on a change in pull request #4318: URL: https://github.com/apache/iceberg/pull/4318#discussion_r835953524
########## File path: python/src/iceberg/table/schema.py ########## @@ -0,0 +1,346 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +import sys +from typing import Iterable, Tuple + +if sys.version_info >= (3, 8): # pragma: no cover + from functools import singledispatchmethod + from typing import Protocol +else: # pragma: no cover + from typing_extensions import Protocol # type: ignore + from singledispatch import singledispatchmethod # type: ignore + +from iceberg.types import IcebergType, ListType, MapType, NestedField, StructType + + +class SchemaVisitor(Protocol): Review comment: I think this should be as close to Java as possible so that visitor implementations can be translated directly. A lot of thinking and bug fixing has gone into those and it's a big risk if the translation over to python isn't straightforward. Rewriting logic in the visitors to handle a different structure could easily cause translation errors and at a minimum will probably require more effort than we want to review. While I like how simple this is, we're probably going to run into problems. The Java visitor has before/after callbacks that aren't supported with this structure using `@singledispatch`. This structure also requires each visitor to recursively call `visit`, which mixes the logic to traverse a schema with the logic to produce some result. Last, using a `result` produced by the visitor instead of returning some value is an unnecessary change for translation. I think we should go with the structure that I originally posted on the [partition spec PR](https://github.com/apache/iceberg/pull/3407/commits/e09d2616b8dae311a25fb13ec7bcebff96ada548). -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
