You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/06/16 22:09:41 UTC
[GitHub] [iceberg] dimas-b opened a new pull request, #5067: API: Support composite types in Accessors
dimas-b opened a new pull request, #5067:
URL: https://github.com/apache/iceberg/pull/5067
Positional accessors can be helpful to getting Row values
that are not primitive. Even though there does not appear
to be any use cases for this in the Iceberg codebase itself,
it seems logical to extends Accessors to support this to
allow external tools to use Accessors uniformly across all
column types.
* Update PositionAccessor to use appropriate javaClass in
get operations targeting composite values types (List, Map,
Struct).
* Generate individual positional accessors for Struct fields
to allow getting them as an Object. Previously, nested structs
could only be accessed field-by-field.
Note: Accessing Struct fields nested in Lists and Maps is
not supported.
/cc @nastra , @ajantha-bhat
--
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-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] RussellSpitzer commented on pull request #5067: API: Support composite types in Accessors
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on PR #5067:
URL: https://github.com/apache/iceberg/pull/5067#issuecomment-1167821382
Just waiting on tests now
--
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-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] RussellSpitzer commented on pull request #5067: API: Support composite types in Accessors
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on PR #5067:
URL: https://github.com/apache/iceberg/pull/5067#issuecomment-1168812329
Ok let's update the TypeID classes in another PR and change all the relevant parts of the code which use the switch/if chain to pick out the right subclass.
--
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-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] dimas-b commented on a diff in pull request #5067: API: Support composite types in Accessors
Posted by GitBox <gi...@apache.org>.
dimas-b commented on code in PR #5067:
URL: https://github.com/apache/iceberg/pull/5067#discussion_r907664566
##########
api/src/main/java/org/apache/iceberg/Accessors.java:
##########
@@ -56,14 +56,30 @@ static Map<Integer, Accessor<StructLike>> forSchema(Schema schema) {
}
private static class PositionAccessor implements Accessor<StructLike> {
- private int position;
+ private final int position;
private final Type type;
private final Class<?> javaClass;
PositionAccessor(int pos, Type type) {
this.position = pos;
this.type = type;
- this.javaClass = type.typeId().javaClass();
+
+ switch (type.typeId()) {
+ case MAP:
+ this.javaClass = Map.class;
Review Comment:
Here's an example of type-based `if`s: https://github.com/apache/iceberg/blob/7c14ab6a42a8d2fc87df5d3f33c9956b3e25aed0/api/src/main/java/org/apache/iceberg/types/Comparators.java#L89-L96
@RussellSpitzer : Do you mean to unify all of the code and use `type.typeId().javaClass()` or just limit the scope to `Accessors`?
--
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-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #5067: API: Support composite types in Accessors
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on code in PR #5067:
URL: https://github.com/apache/iceberg/pull/5067#discussion_r907612627
##########
api/src/main/java/org/apache/iceberg/Accessors.java:
##########
@@ -56,14 +56,30 @@ static Map<Integer, Accessor<StructLike>> forSchema(Schema schema) {
}
private static class PositionAccessor implements Accessor<StructLike> {
- private int position;
+ private final int position;
private final Type type;
private final Class<?> javaClass;
PositionAccessor(int pos, Type type) {
this.position = pos;
this.type = type;
- this.javaClass = type.typeId().javaClass();
+
+ switch (type.typeId()) {
+ case MAP:
+ this.javaClass = Map.class;
Review Comment:
Currently in the Type.java Enum we have this set as "void.class", is it possible for us to just change it there? Or is there a reason that doing that will break things?
https://github.com/apache/iceberg/blob/ecd35afa436f093656fb66c981a51a56ed687af0/api/src/main/java/org/apache/iceberg/types/Type.java#L43-L45
--
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-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #5067: API: Support composite types in Accessors
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5067:
URL: https://github.com/apache/iceberg/pull/5067#discussion_r907961252
##########
api/src/main/java/org/apache/iceberg/Accessors.java:
##########
@@ -56,14 +56,30 @@ static Map<Integer, Accessor<StructLike>> forSchema(Schema schema) {
}
private static class PositionAccessor implements Accessor<StructLike> {
- private int position;
+ private final int position;
private final Type type;
private final Class<?> javaClass;
PositionAccessor(int pos, Type type) {
this.position = pos;
this.type = type;
- this.javaClass = type.typeId().javaClass();
+
+ switch (type.typeId()) {
+ case MAP:
+ this.javaClass = Map.class;
Review Comment:
I think it should be fine to update the type ID classes.
--
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-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] RussellSpitzer merged pull request #5067: API: Support composite types in Accessors
Posted by GitBox <gi...@apache.org>.
RussellSpitzer merged PR #5067:
URL: https://github.com/apache/iceberg/pull/5067
--
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-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] dimas-b commented on a diff in pull request #5067: API: Support composite types in Accessors
Posted by GitBox <gi...@apache.org>.
dimas-b commented on code in PR #5067:
URL: https://github.com/apache/iceberg/pull/5067#discussion_r901687689
##########
api/src/test/java/org/apache/iceberg/TestAccessors.java:
##########
@@ -0,0 +1,174 @@
+/*
+ * Licensed 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;
+
+import java.math.BigDecimal;
+import java.nio.ByteBuffer;
+import java.util.UUID;
+import org.apache.iceberg.TestHelpers.Row;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.Types;
+import org.junit.Assert;
+import org.junit.Test;
+
+import static org.apache.iceberg.types.Types.NestedField.optional;
+import static org.apache.iceberg.types.Types.NestedField.required;
+
+public class TestAccessors {
+
+ private static Accessor<StructLike> direct(Type type) {
+ Schema schema = new Schema(required(17, "field_" + type.typeId(), type));
+ return schema.accessorForField(17);
+ }
+
+ private static Accessor<StructLike> nested1(Type type) {
+ Schema schema = new Schema(required(11, "struct1", Types.StructType.of(
+ Types.NestedField.required(17, "field_" + type.typeId(), type))));
+ return schema.accessorForField(17);
+ }
+
+ private static Accessor<StructLike> nested2(Type type) {
+ Schema schema = new Schema(required(11, "s", Types.StructType.of(
+ Types.NestedField.required(22, "s2", Types.StructType.of(
+ Types.NestedField.required(17, "field_" + type.typeId(), type))))));
+ return schema.accessorForField(17);
+ }
+
+ private static Accessor<StructLike> nested3(Type type) {
+ Schema schema = new Schema(required(11, "s", Types.StructType.of(
+ Types.NestedField.required(22, "s2", Types.StructType.of(
+ Types.NestedField.required(33, "s3", Types.StructType.of(
+ Types.NestedField.required(17, "field_" + type.typeId(), type))))))));
+ return schema.accessorForField(17);
+ }
+
+ private static Accessor<StructLike> nested3optional(Type type) {
+ Schema schema = new Schema(optional(11, "s", Types.StructType.of(
+ Types.NestedField.optional(22, "s2", Types.StructType.of(
+ Types.NestedField.optional(33, "s3", Types.StructType.of(
+ Types.NestedField.optional(17, "field_" + type.typeId(), type))))))));
+ return schema.accessorForField(17);
+ }
+
+ private static Accessor<StructLike> nested4(Type type) {
+ Schema schema = new Schema(required(11, "s", Types.StructType.of(
+ Types.NestedField.required(22, "s2", Types.StructType.of(
+ Types.NestedField.required(33, "s3", Types.StructType.of(
+ Types.NestedField.required(44, "s4", Types.StructType.of(
+ Types.NestedField.required(17, "field_" + type.typeId(), type))))))))));
+ return schema.accessorForField(17);
+ }
+
+ private void assertAccessorReturns(Type type, Object value) {
+ Assert.assertEquals(value, direct(type).get(Row.of(value)));
+
+ Assert.assertEquals(value, nested1(type).get(Row.of(Row.of(value))));
+ Assert.assertEquals(value, nested2(type).get(Row.of(Row.of(Row.of(value)))));
+ Assert.assertEquals(value, nested3(type).get(Row.of(Row.of(Row.of(Row.of(value))))));
+ Assert.assertEquals(value, nested4(type).get(Row.of(Row.of(Row.of(Row.of(Row.of(value)))))));
+
+ Assert.assertEquals(value, nested3optional(type).get(Row.of(Row.of(Row.of(Row.of(value))))));
+ }
+
+ @Test
+ public void testBoolean() {
+ assertAccessorReturns(Types.IntegerType.get(), 123);
Review Comment:
oops... fixing
--
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-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] nastra commented on a diff in pull request #5067: API: Support composite types in Accessors
Posted by GitBox <gi...@apache.org>.
nastra commented on code in PR #5067:
URL: https://github.com/apache/iceberg/pull/5067#discussion_r901348454
##########
api/src/test/java/org/apache/iceberg/TestAccessors.java:
##########
@@ -0,0 +1,174 @@
+/*
+ * Licensed 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;
+
+import java.math.BigDecimal;
+import java.nio.ByteBuffer;
+import java.util.UUID;
+import org.apache.iceberg.TestHelpers.Row;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.Types;
+import org.junit.Assert;
+import org.junit.Test;
+
+import static org.apache.iceberg.types.Types.NestedField.optional;
+import static org.apache.iceberg.types.Types.NestedField.required;
+
+public class TestAccessors {
+
+ private static Accessor<StructLike> direct(Type type) {
+ Schema schema = new Schema(required(17, "field_" + type.typeId(), type));
+ return schema.accessorForField(17);
+ }
+
+ private static Accessor<StructLike> nested1(Type type) {
+ Schema schema = new Schema(required(11, "struct1", Types.StructType.of(
+ Types.NestedField.required(17, "field_" + type.typeId(), type))));
+ return schema.accessorForField(17);
+ }
+
+ private static Accessor<StructLike> nested2(Type type) {
+ Schema schema = new Schema(required(11, "s", Types.StructType.of(
+ Types.NestedField.required(22, "s2", Types.StructType.of(
+ Types.NestedField.required(17, "field_" + type.typeId(), type))))));
+ return schema.accessorForField(17);
+ }
+
+ private static Accessor<StructLike> nested3(Type type) {
+ Schema schema = new Schema(required(11, "s", Types.StructType.of(
+ Types.NestedField.required(22, "s2", Types.StructType.of(
+ Types.NestedField.required(33, "s3", Types.StructType.of(
+ Types.NestedField.required(17, "field_" + type.typeId(), type))))))));
+ return schema.accessorForField(17);
+ }
+
+ private static Accessor<StructLike> nested3optional(Type type) {
+ Schema schema = new Schema(optional(11, "s", Types.StructType.of(
+ Types.NestedField.optional(22, "s2", Types.StructType.of(
+ Types.NestedField.optional(33, "s3", Types.StructType.of(
+ Types.NestedField.optional(17, "field_" + type.typeId(), type))))))));
+ return schema.accessorForField(17);
+ }
+
+ private static Accessor<StructLike> nested4(Type type) {
+ Schema schema = new Schema(required(11, "s", Types.StructType.of(
+ Types.NestedField.required(22, "s2", Types.StructType.of(
+ Types.NestedField.required(33, "s3", Types.StructType.of(
+ Types.NestedField.required(44, "s4", Types.StructType.of(
+ Types.NestedField.required(17, "field_" + type.typeId(), type))))))))));
+ return schema.accessorForField(17);
+ }
+
+ private void assertAccessorReturns(Type type, Object value) {
+ Assert.assertEquals(value, direct(type).get(Row.of(value)));
+
+ Assert.assertEquals(value, nested1(type).get(Row.of(Row.of(value))));
+ Assert.assertEquals(value, nested2(type).get(Row.of(Row.of(Row.of(value)))));
+ Assert.assertEquals(value, nested3(type).get(Row.of(Row.of(Row.of(Row.of(value))))));
+ Assert.assertEquals(value, nested4(type).get(Row.of(Row.of(Row.of(Row.of(Row.of(value)))))));
+
+ Assert.assertEquals(value, nested3optional(type).get(Row.of(Row.of(Row.of(Row.of(value))))));
+ }
+
+ @Test
+ public void testBoolean() {
+ assertAccessorReturns(Types.IntegerType.get(), 123);
Review Comment:
should probably test a boolean here
--
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-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] dimas-b commented on a diff in pull request #5067: API: Support composite types in Accessors
Posted by GitBox <gi...@apache.org>.
dimas-b commented on code in PR #5067:
URL: https://github.com/apache/iceberg/pull/5067#discussion_r907636938
##########
api/src/main/java/org/apache/iceberg/Accessors.java:
##########
@@ -56,14 +56,30 @@ static Map<Integer, Accessor<StructLike>> forSchema(Schema schema) {
}
private static class PositionAccessor implements Accessor<StructLike> {
- private int position;
+ private final int position;
private final Type type;
private final Class<?> javaClass;
PositionAccessor(int pos, Type type) {
this.position = pos;
this.type = type;
- this.javaClass = type.typeId().javaClass();
+
+ switch (type.typeId()) {
+ case MAP:
+ this.javaClass = Map.class;
Review Comment:
Yeah, I saw other places where current code did a custom java type choosing, so I did not want to change the enum without a good reason. I'll look more into this presently.
--
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-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] dimas-b commented on a diff in pull request #5067: API: Support composite types in Accessors
Posted by GitBox <gi...@apache.org>.
dimas-b commented on code in PR #5067:
URL: https://github.com/apache/iceberg/pull/5067#discussion_r907664566
##########
api/src/main/java/org/apache/iceberg/Accessors.java:
##########
@@ -56,14 +56,30 @@ static Map<Integer, Accessor<StructLike>> forSchema(Schema schema) {
}
private static class PositionAccessor implements Accessor<StructLike> {
- private int position;
+ private final int position;
private final Type type;
private final Class<?> javaClass;
PositionAccessor(int pos, Type type) {
this.position = pos;
this.type = type;
- this.javaClass = type.typeId().javaClass();
+
+ switch (type.typeId()) {
+ case MAP:
+ this.javaClass = Map.class;
Review Comment:
Here's an example of type-based `if`s: https://github.com/apache/iceberg/blob/7c14ab6a42a8d2fc87df5d3f33c9956b3e25aed0/api/src/main/java/org/apache/iceberg/types/Comparators.java#L89-L96
@RussellSpitzer : Do you mean to unify all of the code and use `type.typeId().javaClass()` or just limit the scope to `Accessors` in this PR?
--
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-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #5067: API: Support composite types in Accessors
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on code in PR #5067:
URL: https://github.com/apache/iceberg/pull/5067#discussion_r899811526
##########
api/src/test/java/org/apache/iceberg/TestAccessors.java:
##########
@@ -0,0 +1,174 @@
+/*
+ * Licensed 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;
+
+import java.math.BigDecimal;
+import java.nio.ByteBuffer;
+import java.util.UUID;
+import org.apache.iceberg.TestHelpers.Row;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.Types;
+import org.junit.Assert;
+import org.junit.Test;
+
+import static org.apache.iceberg.types.Types.NestedField.optional;
+import static org.apache.iceberg.types.Types.NestedField.required;
+
+public class TestAccessors {
+
+ private static Accessor<StructLike> direct(Type type) {
+ Schema schema = new Schema(required(17, "field_" + type.typeId(), type));
+ return schema.accessorForField(17);
+ }
+
+ private static Accessor<StructLike> nested1(Type type) {
+ Schema schema = new Schema(required(11, "struct1", Types.StructType.of(
+ Types.NestedField.required(17, "field_" + type.typeId(), type))));
+ return schema.accessorForField(17);
+ }
+
+ private static Accessor<StructLike> nested2(Type type) {
+ Schema schema = new Schema(required(11, "s", Types.StructType.of(
+ Types.NestedField.required(22, "s2", Types.StructType.of(
+ Types.NestedField.required(17, "field_" + type.typeId(), type))))));
+ return schema.accessorForField(17);
+ }
+
+ private static Accessor<StructLike> nested3(Type type) {
+ Schema schema = new Schema(required(11, "s", Types.StructType.of(
+ Types.NestedField.required(22, "s2", Types.StructType.of(
+ Types.NestedField.required(33, "s3", Types.StructType.of(
+ Types.NestedField.required(17, "field_" + type.typeId(), type))))))));
+ return schema.accessorForField(17);
+ }
+
+ private static Accessor<StructLike> nested3optional(Type type) {
+ Schema schema = new Schema(optional(11, "s", Types.StructType.of(
+ Types.NestedField.optional(22, "s2", Types.StructType.of(
+ Types.NestedField.optional(33, "s3", Types.StructType.of(
+ Types.NestedField.optional(17, "field_" + type.typeId(), type))))))));
+ return schema.accessorForField(17);
+ }
+
+ private static Accessor<StructLike> nested4(Type type) {
+ Schema schema = new Schema(required(11, "s", Types.StructType.of(
+ Types.NestedField.required(22, "s2", Types.StructType.of(
+ Types.NestedField.required(33, "s3", Types.StructType.of(
+ Types.NestedField.required(44, "s4", Types.StructType.of(
+ Types.NestedField.required(17, "field_" + type.typeId(), type))))))))));
+ return schema.accessorForField(17);
+ }
+
+ private void assertAccessorReturns(Type type, Object value) {
+ Assert.assertEquals(value, direct(type).get(Row.of(value)));
+
+ Assert.assertEquals(value, nested1(type).get(Row.of(Row.of(value))));
+ Assert.assertEquals(value, nested2(type).get(Row.of(Row.of(Row.of(value)))));
+ Assert.assertEquals(value, nested3(type).get(Row.of(Row.of(Row.of(Row.of(value))))));
+ Assert.assertEquals(value, nested4(type).get(Row.of(Row.of(Row.of(Row.of(Row.of(value)))))));
+
+ Assert.assertEquals(value, nested3optional(type).get(Row.of(Row.of(Row.of(Row.of(value))))));
Review Comment:
Only covers struct type. We can cover map and list type also?
--
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-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] dimas-b commented on a diff in pull request #5067: API: Support composite types in Accessors
Posted by GitBox <gi...@apache.org>.
dimas-b commented on code in PR #5067:
URL: https://github.com/apache/iceberg/pull/5067#discussion_r907727228
##########
api/src/main/java/org/apache/iceberg/Accessors.java:
##########
@@ -56,14 +56,30 @@ static Map<Integer, Accessor<StructLike>> forSchema(Schema schema) {
}
private static class PositionAccessor implements Accessor<StructLike> {
- private int position;
+ private final int position;
private final Type type;
private final Class<?> javaClass;
PositionAccessor(int pos, Type type) {
this.position = pos;
this.type = type;
- this.javaClass = type.typeId().javaClass();
+
+ switch (type.typeId()) {
+ case MAP:
+ this.javaClass = Map.class;
Review Comment:
Ok, I added a test case for empty structs and rebased.
Putting specific java types into `TypeID` does not look too difficult, but there is some non-trivial logic about that in `ManifestFileUtil` and `BaseDataReader`. I can certain handle that in a follow-up PR. It seems reasonable to put specific java types into `TypeID`.
--
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-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #5067: API: Support composite types in Accessors
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on code in PR #5067:
URL: https://github.com/apache/iceberg/pull/5067#discussion_r907682409
##########
api/src/main/java/org/apache/iceberg/Accessors.java:
##########
@@ -56,14 +56,30 @@ static Map<Integer, Accessor<StructLike>> forSchema(Schema schema) {
}
private static class PositionAccessor implements Accessor<StructLike> {
- private int position;
+ private final int position;
private final Type type;
private final Class<?> javaClass;
PositionAccessor(int pos, Type type) {
this.position = pos;
this.type = type;
- this.javaClass = type.typeId().javaClass();
+
+ switch (type.typeId()) {
+ case MAP:
+ this.javaClass = Map.class;
Review Comment:
We can save that for another PR I just couldn't understand why it was the way it was
--
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-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] dimas-b commented on pull request #5067: API: Support composite types in Accessors
Posted by GitBox <gi...@apache.org>.
dimas-b commented on PR #5067:
URL: https://github.com/apache/iceberg/pull/5067#issuecomment-1169368773
Thanks for the reviews! Here's the follow-up PR: https://github.com/apache/iceberg/pull/5154
--
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-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] dimas-b commented on a diff in pull request #5067: API: Support composite types in Accessors
Posted by GitBox <gi...@apache.org>.
dimas-b commented on code in PR #5067:
URL: https://github.com/apache/iceberg/pull/5067#discussion_r908452264
##########
api/src/main/java/org/apache/iceberg/Accessors.java:
##########
@@ -56,14 +56,30 @@ static Map<Integer, Accessor<StructLike>> forSchema(Schema schema) {
}
private static class PositionAccessor implements Accessor<StructLike> {
- private int position;
+ private final int position;
private final Type type;
private final Class<?> javaClass;
PositionAccessor(int pos, Type type) {
this.position = pos;
this.type = type;
- this.javaClass = type.typeId().javaClass();
+
+ switch (type.typeId()) {
+ case MAP:
+ this.javaClass = Map.class;
Review Comment:
will do in a follow up PR (did not get the time yesterday :) )
--
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-unsubscribe@iceberg.apache.org
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