-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Core: Add v4 manifest reader #16958
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Core: Add v4 manifest reader #16958
Changes from all commits
b70b31d
e45c65e
0853492
f1821b3
d872a76
67579d4
c78b664
cb918d6
765b5e7
08f5888
a951e11
f0925d6
f4a75b2
b798682
9656a4d
607628d
7a11c80
a807c9a
b188369
638e1d8
3c6be36
f6bf7ef
c5d2018
1d2b21f
47ecda8
d244e63
ae84803
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,107 @@ | ||
| /* | ||
| * 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. | ||
| */ | ||
| package org.apache.iceberg.types; | ||
|
|
||
| import java.util.List; | ||
| import java.util.Map; | ||
| import org.apache.iceberg.Schema; | ||
| import org.apache.iceberg.relocated.com.google.common.collect.Lists; | ||
|
|
||
| class ReplaceTypeById extends TypeUtil.SchemaVisitor<Type> { | ||
| private final Map<Integer, Type> replacementsById; | ||
|
|
||
| ReplaceTypeById(Map<Integer, Type> replacementsById) { | ||
| this.replacementsById = replacementsById; | ||
| } | ||
|
|
||
| @Override | ||
| public Type schema(Schema schema, Type structResult) { | ||
| return structResult; | ||
| } | ||
|
|
||
| @Override | ||
| public Type struct(Types.StructType struct, List<Type> fieldResults) { | ||
| List<Types.NestedField> fields = struct.fields(); | ||
| List<Types.NestedField> newFields = Lists.newArrayListWithExpectedSize(fields.size()); | ||
| boolean hasChanged = false; | ||
|
|
||
| for (int i = 0; i < fields.size(); i += 1) { | ||
| Type fieldReplacement = fieldResults.get(i); | ||
| Types.NestedField field = fields.get(i); | ||
| if (field.type() != fieldReplacement) { | ||
| hasChanged = true; | ||
| newFields.add(Types.NestedField.from(field).ofType(fieldReplacement).build()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we need to handle initial/write defaults here when we're changing the type? What happens if the original type is int with some defaults but we change the type to string? I think we should have a test for this scenario
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I checked, and defaults are already handled correctly because it turns out |
||
| } else { | ||
| newFields.add(field); | ||
| } | ||
| } | ||
|
|
||
| if (hasChanged) { | ||
| return Types.StructType.of(newFields); | ||
| } | ||
|
|
||
| return struct; | ||
| } | ||
|
|
||
| @Override | ||
| public Type field(Types.NestedField field, Type fieldResult) { | ||
| return replacementsById.getOrDefault(field.fieldId(), fieldResult); | ||
| } | ||
|
|
||
| @Override | ||
| public Type list(Types.ListType list, Type elementResult) { | ||
| Type elementReplacement = replacementsById.getOrDefault(list.elementId(), elementResult); | ||
| if (list.elementType() != elementReplacement) { | ||
| if (list.isElementRequired()) { | ||
| return Types.ListType.ofRequired(list.elementId(), elementReplacement); | ||
| } else { | ||
| return Types.ListType.ofOptional(list.elementId(), elementReplacement); | ||
| } | ||
| } | ||
|
|
||
| return list; | ||
| } | ||
|
|
||
| @Override | ||
| public Type map(Types.MapType map, Type keyResult, Type valueResult) { | ||
|
anoopj marked this conversation as resolved.
|
||
| Type keyReplacement = replacementsById.getOrDefault(map.keyId(), keyResult); | ||
| Type valueReplacement = replacementsById.getOrDefault(map.valueId(), valueResult); | ||
| if (map.keyType() != keyReplacement || map.valueType() != valueReplacement) { | ||
| if (map.isValueRequired()) { | ||
| return Types.MapType.ofRequired( | ||
| map.keyId(), map.valueId(), keyReplacement, valueReplacement); | ||
| } else { | ||
| return Types.MapType.ofOptional( | ||
| map.keyId(), map.valueId(), keyReplacement, valueReplacement); | ||
| } | ||
| } | ||
|
|
||
| return map; | ||
| } | ||
|
|
||
| @Override | ||
| public Type variant(Types.VariantType variant) { | ||
| return variant; | ||
| } | ||
|
|
||
| @Override | ||
| public Type primitive(Type.PrimitiveType primitive) { | ||
| return primitive; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: I think it might be worth pulling this out into a separate PR in order to reduce complexity in this one and harden the impl + have more extensive tests (especially around default value handling)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are correct. That would have been a better choice. Keeping it here because the code has been reviewed. Will add the tests.