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 2021/06/08 11:33:19 UTC
[GitHub] [iceberg] openinx opened a new pull request #2683: Tests: Add unit tests for InternalRecordWrapper, RowDataWrapper, InternalRowWrapper
openinx opened a new pull request #2683:
URL: https://github.com/apache/iceberg/pull/2683
Add unit tests to address the introduced `InternalRecordWrapper`, `RowDataWrapper`, `InternalRowWrapper`. The intention of writing this test is : I find that the `InternalRecordWrapper` did not handle the nullable value correct ( I mean we will cast a `null` to the target value, that is incorrect), so I think preparing an unit tests for this is necessary.
--
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.
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 change in pull request #2683: Tests: Add unit tests for InternalRecordWrapper, RowDataWrapper, InternalRowWrapper
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2683:
URL: https://github.com/apache/iceberg/pull/2683#discussion_r650443371
##########
File path: data/src/main/java/org/apache/iceberg/data/InternalRecordWrapper.java
##########
@@ -80,10 +80,15 @@ public int size() {
@Override
public <T> T get(int pos, Class<T> javaClass) {
- if (transforms[pos] != null) {
- return javaClass.cast(transforms[pos].apply(wrapped.get(pos, Object.class)));
+ Object value = wrapped.get(pos, Object.class);
Review comment:
I don't think this should change the behavior of the else case where the java class is passed to `wrapped.get`. For that case, the right thing to do is to delegate the operation to `wrapped`. The reason to use `Object.class` is when we have a transform function and know that the java class passed in won't work.
##########
File path: data/src/main/java/org/apache/iceberg/data/InternalRecordWrapper.java
##########
@@ -80,10 +80,15 @@ public int size() {
@Override
public <T> T get(int pos, Class<T> javaClass) {
- if (transforms[pos] != null) {
- return javaClass.cast(transforms[pos].apply(wrapped.get(pos, Object.class)));
+ Object value = wrapped.get(pos, Object.class);
+
+ if (value == null) {
+ return null;
Review comment:
Why is it incorrect to call `javaClass.cast(null)`?
##########
File path: data/src/test/java/org/apache/iceberg/RecordWrapperTest.java
##########
@@ -0,0 +1,120 @@
+/*
+ * 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;
+
+import org.apache.iceberg.types.Types;
+import org.apache.iceberg.util.StructLikeWrapper;
+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 abstract class RecordWrapperTest {
+
+ private static final Types.StructType PRIMITIVE_WITHOUT_TIME = Types.StructType.of(
+ required(100, "id", Types.LongType.get()),
+ optional(101, "data", Types.StringType.get()),
+ required(102, "b", Types.BooleanType.get()),
+ optional(103, "i", Types.IntegerType.get()),
+ required(104, "l", Types.LongType.get()),
+ optional(105, "f", Types.FloatType.get()),
+ required(106, "d", Types.DoubleType.get()),
+ optional(107, "date", Types.DateType.get()),
+ required(108, "ts_tz", Types.TimestampType.withZone()),
+ required(110, "s", Types.StringType.get()),
+ required(112, "fixed", Types.FixedType.ofLength(7)),
+ optional(113, "bytes", Types.BinaryType.get()),
+ required(114, "dec_9_0", Types.DecimalType.of(9, 0)),
+ required(115, "dec_11_2", Types.DecimalType.of(11, 2)),
+ required(116, "dec_38_10", Types.DecimalType.of(38, 10))// maximum precision
+ );
+
+ private static final Types.StructType TIMESTAMP_WITHOUT_ZONE = Types.StructType.of(
+ required(101, "ts0", Types.TimestampType.withoutZone()),
+ required(102, "ts1", Types.TimestampType.withoutZone())
+ );
+
+ private static final Types.StructType TIME = Types.StructType.of(
+ required(100, "time0", Types.TimeType.get()),
+ optional(101, "time1", Types.TimeType.get())
+ );
+
+ @Test
+ public void testSimpleStructWithoutTime() {
+ generateAndValidate(new Schema(PRIMITIVE_WITHOUT_TIME.fields()));
+ }
+
+ @Test
+ public void testTimestampWithoutZone() {
+ generateAndValidate(new Schema(TIMESTAMP_WITHOUT_ZONE.fields()));
+ }
+
+ @Test
+ public void testTime() {
+ generateAndValidate(new Schema(TIME.fields()), (message, expectedWrapper, actualWrapper) -> {
+ for (int pos = 0; pos < TIME.fields().size(); pos++) {
+ Object expected = expectedWrapper.get().get(pos, Object.class);
+ Object actual = actualWrapper.get().get(pos, Object.class);
+ if (expected == actual) {
+ return;
+ }
+
+ if (expected == null || actual == null) {
+ Assert.fail(String.format("The expected value is %s but actual value is %s", expected, actual));
+ }
+
+ int expectedMilliseconds = (int) ((long) expected / 1000_000);
+ int actualMilliseconds = (int) ((long) actual / 1000_000);
+ Assert.assertEquals(message, expectedMilliseconds, actualMilliseconds);
Review comment:
I don't think that this is valid. Millisecond truncation should never be allowed, except in Flink where it is unavoidable. In the data module, this is incorrect.
--
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.
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 change in pull request #2683: Tests: Add unit tests for InternalRecordWrapper, RowDataWrapper, InternalRowWrapper
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2683:
URL: https://github.com/apache/iceberg/pull/2683#discussion_r654571257
##########
File path: data/src/main/java/org/apache/iceberg/data/InternalRecordWrapper.java
##########
@@ -81,7 +81,13 @@ public int size() {
@Override
public <T> T get(int pos, Class<T> javaClass) {
if (transforms[pos] != null) {
- return javaClass.cast(transforms[pos].apply(wrapped.get(pos, Object.class)));
+ Object value = wrapped.get(pos, Object.class);
+ if (value == null) {
+ // transforms function don't allow to handle null values, so just return null here.
Review comment:
Thanks for adding a comment to clarify why you're making this change!
--
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.
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 #2683: Tests: Add unit tests for InternalRecordWrapper, RowDataWrapper, InternalRowWrapper
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #2683:
URL: https://github.com/apache/iceberg/pull/2683
--
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.
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