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/28 22:57:53 UTC
[GitHub] [iceberg] dimas-b opened a new pull request, #5154: API: Add java type to composite TypeID enums
dimas-b opened a new pull request, #5154:
URL: https://github.com/apache/iceberg/pull/5154
Previously TypeID.STRUCT, LIST and MAP exposed Void as their
java type. It seems reasonable to expose more specific java
types according to existing code patterns in Accessors and
Comparators and simplify their if/switch statements.
ManifestFileUtil has non-trivial logic around the value returned
from TypeID.javaClass() but it works only on PrimitiveType, so it
is not affected by this change.
BaseDataReader (for Spark) also has non-trivial logic for processing
TypeID.javaClass() in its convertConstant() method, but this change
appears to actually improve the processing of values in that case.
PartitionSpec appears to be unaffected by this change as it would
normally use TypeID.javaClass() only for primitive values. In any
case this change should not break its processing logic.
--
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 merged pull request #5154: API: Add java type to composite TypeID enums
Posted by GitBox <gi...@apache.org>.
rdblue merged PR #5154:
URL: 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] nastra commented on a diff in pull request #5154: API: Add java type to composite TypeID enums
Posted by GitBox <gi...@apache.org>.
nastra commented on code in PR #5154:
URL: https://github.com/apache/iceberg/pull/5154#discussion_r910651749
##########
api/src/test/java/org/apache/iceberg/types/TestComparators.java:
##########
@@ -0,0 +1,151 @@
+/*
+ * 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.math.BigDecimal;
+import java.nio.ByteBuffer;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.UUID;
+import org.apache.iceberg.TestHelpers;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class TestComparators {
+
+ private <T> void assertComparesCorrectly(Comparator<T> cmp, T less, T greater) {
+ Assert.assertEquals(0, cmp.compare(greater, greater));
+ Assert.assertEquals(0, cmp.compare(less, less));
+ Assert.assertEquals(-1, Integer.signum(cmp.compare(less, greater)));
+ Assert.assertEquals(1, Integer.signum(cmp.compare(greater, less)));
+ }
+
+ @Test
+ public void testBoolean() {
+ assertComparesCorrectly(Comparators.forType(Types.BooleanType.get()), false, true);
+ }
+
+ @Test
+ public void testInt() {
+ assertComparesCorrectly(Comparators.forType(Types.IntegerType.get()), 0, 1);
Review Comment:
nit: should we also have comparisons where both values are the same across all the test methods?
--
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 pull request #5154: API: Add java type to composite TypeID enums
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #5154:
URL: https://github.com/apache/iceberg/pull/5154#issuecomment-1173164391
Thanks, @dimas-b!
--
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 #5154: API: Add java type to composite TypeID enums
Posted by GitBox <gi...@apache.org>.
dimas-b commented on code in PR #5154:
URL: https://github.com/apache/iceberg/pull/5154#discussion_r912960170
##########
api/src/test/java/org/apache/iceberg/types/TestComparators.java:
##########
@@ -0,0 +1,151 @@
+/*
+ * 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.math.BigDecimal;
+import java.nio.ByteBuffer;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.UUID;
+import org.apache.iceberg.TestHelpers;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class TestComparators {
+
+ private <T> void assertComparesCorrectly(Comparator<T> cmp, T less, T greater) {
+ Assert.assertEquals(0, cmp.compare(greater, greater));
+ Assert.assertEquals(0, cmp.compare(less, less));
+ Assert.assertEquals(-1, Integer.signum(cmp.compare(less, greater)));
+ Assert.assertEquals(1, Integer.signum(cmp.compare(greater, less)));
+ }
+
+ @Test
+ public void testBoolean() {
+ assertComparesCorrectly(Comparators.forType(Types.BooleanType.get()), false, true);
+ }
+
+ @Test
+ public void testInt() {
+ assertComparesCorrectly(Comparators.forType(Types.IntegerType.get()), 0, 1);
Review Comment:
`assertComparesCorrectly` does that already... or did you mean something else?
--
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