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/04/08 00:58:09 UTC
[GitHub] [iceberg] rzhang10 opened a new pull request, #4525: API: Add default value core api and schema serialization
rzhang10 opened a new pull request, #4525:
URL: https://github.com/apache/iceberg/pull/4525
This is the first PR to implement the spec pr #4301
It augments the `NestedField` API with default values and adds logic to `SchemaParser` for serializing default values with schema.
The default value is represented in memory as `JsonNode` and validated against their corresponding Iceberg schema types.
This PR will be the basis of the later PRs to support default value r/w semantics in each data type X compute engine.
--
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] rzhang10 commented on a diff in pull request #4525: API: Add default value core api and schema serialization
Posted by GitBox <gi...@apache.org>.
rzhang10 commented on code in PR #4525:
URL: https://github.com/apache/iceberg/pull/4525#discussion_r847670673
##########
api/src/main/java/org/apache/iceberg/types/Types.java:
##########
@@ -524,6 +563,91 @@ public boolean equals(Object o) {
public int hashCode() {
return Objects.hash(NestedField.class, id, isOptional, name, type);
}
+
+ private static JsonNode validateDefault(String name, Type type, JsonNode defaultValue) {
+ if (defaultValue != null && !isValidDefault(type, defaultValue)) {
+ throw new ValidationException("Invalid default value for field %s: %s not a %s", name, defaultValue, type);
+ }
+ return defaultValue;
+ }
+
+ @SuppressWarnings("checkstyle:CyclomaticComplexity")
+ private static boolean isValidDefault(Type type, JsonNode defaultValue) {
Review Comment:
I feel using a `SchemaVisitor` here would be an overkill, since this logic is only validating a `NestField`, it's technically only part of the schema, and even if it's a complex type, we only recursively validate downward, but not upward. Also I feel readability-wise, this recursive implementation is clean enough? it clearly shows the logic is to validate each type of default value case-by-case.
To me, I feel `SchemaVisitor` is mostly used to do holistic schema manipulations/transformations.
--
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 #4525: API: Add default value core api and schema serialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4525:
URL: https://github.com/apache/iceberg/pull/4525#discussion_r863325993
##########
api/src/main/java/org/apache/iceberg/types/Types.java:
##########
@@ -415,43 +415,64 @@ public int hashCode() {
public static class NestedField implements Serializable {
public static NestedField optional(int id, String name, Type type) {
- return new NestedField(true, id, name, type, null);
+ return new NestedField(true, id, name, type, null, null, null);
}
public static NestedField optional(int id, String name, Type type, String doc) {
- return new NestedField(true, id, name, type, doc);
+ return new NestedField(true, id, name, type, doc, null, null);
+ }
+
+ public static NestedField optional(int id, String name, Type type, String doc,
+ Object initialDefault, Object writeDefault) {
+ return new NestedField(true, id, name, type, doc, initialDefault, writeDefault);
}
public static NestedField required(int id, String name, Type type) {
- return new NestedField(false, id, name, type, null);
+ return new NestedField(false, id, name, type, null, null, null);
}
public static NestedField required(int id, String name, Type type, String doc) {
- return new NestedField(false, id, name, type, doc);
+ return new NestedField(false, id, name, type, doc, null, null);
+ }
+
+ public static NestedField required(int id, String name, Type type, String doc,
+ Object initialDefault, Object writeDefault) {
+ return new NestedField(false, id, name, type, doc, initialDefault, writeDefault);
}
public static NestedField of(int id, boolean isOptional, String name, Type type) {
- return new NestedField(isOptional, id, name, type, null);
+ return new NestedField(isOptional, id, name, type, null, null, null);
}
public static NestedField of(int id, boolean isOptional, String name, Type type, String doc) {
- return new NestedField(isOptional, id, name, type, doc);
+ return new NestedField(isOptional, id, name, type, doc, null, null);
+ }
+
+ public static NestedField of(int id, boolean isOptional, String name, Type type, String doc,
+ Object initialDefault, Object writeDefault) {
+ return new NestedField(isOptional, id, name, type, doc, initialDefault, writeDefault);
}
private final boolean isOptional;
private final int id;
private final String name;
private final Type type;
private final String doc;
+ private final Object initialDefault;
+ private final Object writeDefault;
+
- private NestedField(boolean isOptional, int id, String name, Type type, String doc) {
+ private NestedField(boolean isOptional, int id, String name, Type type, String doc,
+ Object initialDefault, Object writeDefault) {
Review Comment:
Style: In Iceberg always start all argument lines at the same point, either all indented after the method declaration (aligned with `boolean isOptional, ...`) or all starting at the same continuation indent level. This mixes the two.
--
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] rzhang10 commented on a diff in pull request #4525: API: Add default value core api and schema serialization
Posted by GitBox <gi...@apache.org>.
rzhang10 commented on code in PR #4525:
URL: https://github.com/apache/iceberg/pull/4525#discussion_r867267616
##########
core/src/main/java/org/apache/iceberg/DefaultValueParser.java:
##########
@@ -0,0 +1,253 @@
+/*
+ * 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 com.fasterxml.jackson.core.JsonFactory;
+import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.SerializerProvider;
+import com.fasterxml.jackson.databind.module.SimpleModule;
+import com.fasterxml.jackson.databind.ser.std.ByteBufferSerializer;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.time.LocalDateTime;
+import java.time.ZoneOffset;
+import java.time.format.DateTimeFormatter;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.stream.Collectors;
+import java.util.stream.StreamSupport;
+import org.apache.iceberg.exceptions.ValidationException;
+import org.apache.iceberg.expressions.Literal;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.relocated.com.google.common.io.BaseEncoding;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.Types;
+
+public class DefaultValueParser {
Review Comment:
The reason I put them together is that, the default value serialization and deserialization will depends on the APIs to access the default value stored inside the schema. So in order for the `jsonRoundTrip` test to work, it must depends on the methods introduced in the API..
Do you think we can first create a standalone PR with only API changes (I think that PR should be very minimum and fast for approval and merge), then I can rebase the changes and submit subsequent PRs? (The spark reader implementation PRs will also surely depends on the new API 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] rzhang10 commented on a diff in pull request #4525: API: Add default value core api and schema serialization
Posted by GitBox <gi...@apache.org>.
rzhang10 commented on code in PR #4525:
URL: https://github.com/apache/iceberg/pull/4525#discussion_r867273314
##########
api/src/main/java/org/apache/iceberg/expressions/Literals.java:
##########
@@ -419,6 +420,9 @@ static class TimeLiteral extends ComparableLiteral<Long> {
public <T> Literal<T> to(Type type) {
if (type.typeId() == Type.TypeID.TIME) {
return (Literal<T>) this;
+ } else if (type.typeId() == Type.TypeID.STRING) {
+ return (Literal<T>) new StringLiteral(LocalTime.ofNanoOfDay(value() * 1000)
+ .format(DateTimeFormatter.ISO_LOCAL_TIME));
Review Comment:
Yeah since previously I remember you pointed me to the `Literals` code so I thought I might be able to utilize the existing literals conversion logic there.
My changes basically add the conversion from the 3 time/date-related type to string literal. Since I noticed the existing string literal code has the conversion to those date/time types, so I thought it might be a plus to add my current changes that can make the conversions more symmetric and enable people to do round-trip conversions?
WDYT?
--
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 #4525: API: Add default value core api and schema serialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #4525:
URL: https://github.com/apache/iceberg/pull/4525#issuecomment-1095496964
@rzhang10, it's okay to add the library to the API module if that's what is needed, although I'd like to see if we actually need it there or if we can convert values to/from JSON in core.
For the in-memory representation, we do need to use the common representation and not JsonNode. We also cannot leak Jackson classes in the API module.
--
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 #4525: API: Add default value core api and schema serialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4525:
URL: https://github.com/apache/iceberg/pull/4525#discussion_r863325248
##########
api/src/main/java/org/apache/iceberg/expressions/Literal.java:
##########
@@ -71,6 +71,18 @@ static Literal<BigDecimal> of(BigDecimal value) {
return new Literals.DecimalLiteral(value);
}
+ static Literal<Integer> ofDateLiteral(int value) {
Review Comment:
I don't think you should need any new literal constructors. You can always convert using `to` and the type.
--
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] rzhang10 commented on a diff in pull request #4525: API: Add default value core api and schema serialization
Posted by GitBox <gi...@apache.org>.
rzhang10 commented on code in PR #4525:
URL: https://github.com/apache/iceberg/pull/4525#discussion_r867272143
##########
core/src/main/java/org/apache/iceberg/DefaultValueParser.java:
##########
@@ -0,0 +1,253 @@
+/*
+ * 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 com.fasterxml.jackson.core.JsonFactory;
+import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.SerializerProvider;
+import com.fasterxml.jackson.databind.module.SimpleModule;
+import com.fasterxml.jackson.databind.ser.std.ByteBufferSerializer;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.time.LocalDateTime;
+import java.time.ZoneOffset;
+import java.time.format.DateTimeFormatter;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.stream.Collectors;
+import java.util.stream.StreamSupport;
+import org.apache.iceberg.exceptions.ValidationException;
+import org.apache.iceberg.expressions.Literal;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.relocated.com.google.common.io.BaseEncoding;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.Types;
+
+public class DefaultValueParser {
+ private DefaultValueParser() {
+ }
+
+ private static final JsonFactory FACTORY = new JsonFactory();
+ private static final ObjectMapper MAPPER;
+
+ static {
+ MAPPER = new ObjectMapper(FACTORY);
+ SimpleModule customModule = new SimpleModule();
+ customModule.addSerializer(ByteBuffer.class, new HexStringCustomByteBufferSerializer());
+ MAPPER.registerModule(customModule);
Review Comment:
I use this mainly to help serialize the java in-mem `fixed` and `binary` type defaults which are represented by java `ByteBuffer` to our custom json representation that looks like `0x111f`, you can see the customized serialized logic in `HexStringCustomByteBufferSerializer`.
--
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
Re: [PR] API: Add default value core api and schema serialization [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #4525: API: Add default value core api and schema serialization
URL: https://github.com/apache/iceberg/pull/4525
--
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 #4525: API: Add default value core api and schema serialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4525:
URL: https://github.com/apache/iceberg/pull/4525#discussion_r846841623
##########
api/src/test/java/org/apache/iceberg/types/TestTypeDefaultValuesValidation.java:
##########
@@ -0,0 +1,89 @@
+/*
+ * 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 com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import java.util.Arrays;
+import java.util.Collection;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import static org.apache.iceberg.types.Types.NestedField.of;
+import static org.apache.iceberg.types.Types.NestedField.optional;
+import static org.apache.iceberg.types.Types.NestedField.required;
+
+@RunWith(Parameterized.class)
+public class TestTypeDefaultValuesValidation {
+ private final Type type;
+ private final JsonNode defaultValue;
+
+ public TestTypeDefaultValuesValidation(Type type, JsonNode defaultValue) {
+ this.type = type;
+ this.defaultValue = defaultValue;
+ }
+
+ @Parameterized.Parameters
+ public static Collection<Object[]> data() {
+ return Arrays.asList(new Object[][] {
+ {Types.BooleanType.get(), parseJsonStringToJsonNode("true")},
+ {Types.IntegerType.get(), parseJsonStringToJsonNode("1")},
+ {Types.LongType.get(), parseJsonStringToJsonNode("9999999")},
+ {Types.FloatType.get(), parseJsonStringToJsonNode("1.23")},
+ {Types.DoubleType.get(), parseJsonStringToJsonNode("123.456")},
+ {Types.DateType.get(), parseJsonStringToJsonNode("3650")},
+ {Types.TimeType.get(), parseJsonStringToJsonNode("36000000000")},
+ {Types.TimestampType.withoutZone(), parseJsonStringToJsonNode("1649374911000000")},
Review Comment:
Looking at this again, I think that date/time fields should be converted to (strict) ISO-8601 representations. That way they are readable in JSON.
--
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] rzhang10 commented on a diff in pull request #4525: API: Add default value core api and schema serialization
Posted by GitBox <gi...@apache.org>.
rzhang10 commented on code in PR #4525:
URL: https://github.com/apache/iceberg/pull/4525#discussion_r867266873
##########
core/src/main/java/org/apache/iceberg/DefaultValueParser.java:
##########
@@ -0,0 +1,253 @@
+/*
+ * 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 com.fasterxml.jackson.core.JsonFactory;
+import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.SerializerProvider;
+import com.fasterxml.jackson.databind.module.SimpleModule;
+import com.fasterxml.jackson.databind.ser.std.ByteBufferSerializer;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.time.LocalDateTime;
+import java.time.ZoneOffset;
+import java.time.format.DateTimeFormatter;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.stream.Collectors;
+import java.util.stream.StreamSupport;
+import org.apache.iceberg.exceptions.ValidationException;
+import org.apache.iceberg.expressions.Literal;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.relocated.com.google.common.io.BaseEncoding;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.Types;
+
+public class DefaultValueParser {
+ private DefaultValueParser() {
+ }
+
+ private static final JsonFactory FACTORY = new JsonFactory();
+ private static final ObjectMapper MAPPER;
+
+ static {
+ MAPPER = new ObjectMapper(FACTORY);
+ SimpleModule customModule = new SimpleModule();
+ customModule.addSerializer(ByteBuffer.class, new HexStringCustomByteBufferSerializer());
+ MAPPER.registerModule(customModule);
+ }
+
+ public static ObjectMapper mapper() {
+ return MAPPER;
+ }
+
+ public static Object parseDefaultFromJson(Type type, JsonNode jsonNode) {
+ if (jsonNode == null) {
+ return null;
+ }
+
+ switch (type.typeId()) {
+ case BOOLEAN:
+ return jsonNode.booleanValue();
Review Comment:
There's another dedicated method which does validation work `isValidDefault` which does this similar check already, and it should always be called before calling the current method `parseDefaultFromJson` which does the actual parsing.
--
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 #4525: API: Add default value core api and schema serialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4525:
URL: https://github.com/apache/iceberg/pull/4525#discussion_r846841468
##########
build.gradle:
##########
@@ -193,6 +193,7 @@ project(':iceberg-bundled-guava') {
project(':iceberg-api') {
dependencies {
implementation project(path: ':iceberg-bundled-guava', configuration: 'shadow')
+ implementation "com.fasterxml.jackson.core:jackson-databind"
Review Comment:
Why is an additional Jackson library needed? I think we should be able to add default value serialization just like the other parsers.
--
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] rzhang10 commented on a diff in pull request #4525: API: Add default value core api and schema serialization
Posted by GitBox <gi...@apache.org>.
rzhang10 commented on code in PR #4525:
URL: https://github.com/apache/iceberg/pull/4525#discussion_r867274300
##########
core/src/main/java/org/apache/iceberg/DefaultValueParser.java:
##########
@@ -0,0 +1,253 @@
+/*
+ * 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 com.fasterxml.jackson.core.JsonFactory;
+import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.SerializerProvider;
+import com.fasterxml.jackson.databind.module.SimpleModule;
+import com.fasterxml.jackson.databind.ser.std.ByteBufferSerializer;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.time.LocalDateTime;
+import java.time.ZoneOffset;
+import java.time.format.DateTimeFormatter;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.stream.Collectors;
+import java.util.stream.StreamSupport;
+import org.apache.iceberg.exceptions.ValidationException;
+import org.apache.iceberg.expressions.Literal;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.relocated.com.google.common.io.BaseEncoding;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.Types;
+
+public class DefaultValueParser {
Review Comment:
The reason I'm asking is even if I split all my later PRs, if the API changes are not checked in, those PRs won't compile themselves, or else I have to include the same API code changes in all the split PRs respectively.
--
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
Re: [PR] API: Add default value core api and schema serialization [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4525:
URL: https://github.com/apache/iceberg/pull/4525#issuecomment-2290114427
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
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 #4525: API: Add default value core api and schema serialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4525:
URL: https://github.com/apache/iceberg/pull/4525#discussion_r876056384
##########
api/src/main/java/org/apache/iceberg/expressions/Literals.java:
##########
@@ -419,6 +420,9 @@ static class TimeLiteral extends ComparableLiteral<Long> {
public <T> Literal<T> to(Type type) {
if (type.typeId() == Type.TypeID.TIME) {
return (Literal<T>) this;
+ } else if (type.typeId() == Type.TypeID.STRING) {
+ return (Literal<T>) new StringLiteral(LocalTime.ofNanoOfDay(value() * 1000)
+ .format(DateTimeFormatter.ISO_LOCAL_TIME));
Review Comment:
I don't think this conversion should be done using literals. You can see how we're doing it here, but there is no need to change the `Literals` API
--
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 #4525: API: Add default value core api and schema serialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4525:
URL: https://github.com/apache/iceberg/pull/4525#discussion_r863328159
##########
core/src/test/java/org/apache/iceberg/TestDefaultValuesValidationAndParsing.java:
##########
@@ -0,0 +1,99 @@
+/*
+ * 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 com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import java.util.Arrays;
+import java.util.Collection;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.Types;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import static org.apache.iceberg.types.Types.NestedField.optional;
+import static org.apache.iceberg.types.Types.NestedField.required;
+
+@RunWith(Parameterized.class)
+public class TestDefaultValuesValidationAndParsing {
+
+ private final Type type;
+ private final JsonNode defaultValue;
+
+ public TestDefaultValuesValidationAndParsing(Type type, JsonNode defaultValue) {
+ this.type = type;
+ this.defaultValue = defaultValue;
+ }
+
+ @Parameterized.Parameters
+ public static Collection<Object[]> data() {
+ return Arrays.asList(new Object[][] {
+ {Types.BooleanType.get(), stringToJsonNode("true")},
+ {Types.IntegerType.get(), stringToJsonNode("1")},
+ {Types.LongType.get(), stringToJsonNode("9999999")},
+ {Types.FloatType.get(), stringToJsonNode("1.23")},
+ {Types.DoubleType.get(), stringToJsonNode("123.456")},
+ {Types.DateType.get(), stringToJsonNode("\"2007-12-03\"")},
+ {Types.TimeType.get(), stringToJsonNode("\"10:15:30\"")},
+ {Types.TimestampType.withoutZone(), stringToJsonNode("\"2007-12-03T10:15:30\"")},
+ {Types.TimestampType.withZone(), stringToJsonNode("\"2007-12-03T10:15:30+01:00\"")},
+ {Types.StringType.get(), stringToJsonNode("\"foo\"")},
+ {Types.UUIDType.get(), stringToJsonNode("\"eb26bdb1-a1d8-4aa6-990e-da940875492c\"")},
+ {Types.FixedType.ofLength(2), stringToJsonNode("\"0x111f\"")},
+ {Types.BinaryType.get(), stringToJsonNode("\"0x0000ff\"")},
+ {Types.DecimalType.of(9, 2), stringToJsonNode("123.45")},
+ {Types.ListType.ofOptional(1, Types.IntegerType.get()), stringToJsonNode("[1, 2, 3]")},
+ {Types.MapType.ofOptional(1, 2, Types.IntegerType.get(), Types.StringType.get()),
+ stringToJsonNode("[[1,2], [\"foo\", \"bar\"]]")},
+ {Types.StructType.of(
+ required(1, "f1", Types.IntegerType.get(), "doc"),
+ optional(2, "f2", Types.StringType.get(), "doc")),
+ stringToJsonNode("{\"1\": 1, \"2\": \"bar\"}")}
+ });
+ }
+
+ private static JsonNode stringToJsonNode(String json) {
+ try {
+ ObjectMapper mapper = new ObjectMapper();
+ return mapper.readTree(json);
+ } catch (JsonProcessingException e) {
+ System.out.println("Failed to parse: " + json + "; reason: " + e.getMessage());
Review Comment:
Please remove print statements.
--
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 #4525: API: Add default value core api and schema serialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4525:
URL: https://github.com/apache/iceberg/pull/4525#discussion_r846841694
##########
api/src/main/java/org/apache/iceberg/types/Types.java:
##########
@@ -524,6 +563,91 @@ public boolean equals(Object o) {
public int hashCode() {
return Objects.hash(NestedField.class, id, isOptional, name, type);
}
+
+ private static JsonNode validateDefault(String name, Type type, JsonNode defaultValue) {
+ if (defaultValue != null && !isValidDefault(type, defaultValue)) {
+ throw new ValidationException("Invalid default value for field %s: %s not a %s", name, defaultValue, type);
+ }
+ return defaultValue;
+ }
+
+ @SuppressWarnings("checkstyle:CyclomaticComplexity")
+ private static boolean isValidDefault(Type type, JsonNode defaultValue) {
Review Comment:
Iceberg uses visitors to separate the logic for schema traversal from the logic for individual decisions. Can you please use a visitor?
--
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 #4525: API: Add default value core api and schema serialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4525:
URL: https://github.com/apache/iceberg/pull/4525#discussion_r863325599
##########
api/src/main/java/org/apache/iceberg/expressions/Literals.java:
##########
@@ -419,6 +420,9 @@ static class TimeLiteral extends ComparableLiteral<Long> {
public <T> Literal<T> to(Type type) {
if (type.typeId() == Type.TypeID.TIME) {
return (Literal<T>) this;
+ } else if (type.typeId() == Type.TypeID.STRING) {
+ return (Literal<T>) new StringLiteral(LocalTime.ofNanoOfDay(value() * 1000)
+ .format(DateTimeFormatter.ISO_LOCAL_TIME));
Review Comment:
I don't think this conversion should be done using literals. Instead, use the methods in `DateTimeUtil` and add any new methods there. This shouldn't require changes to the public API in expressions.
--
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 #4525: API: Add default value core api and schema serialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4525:
URL: https://github.com/apache/iceberg/pull/4525#discussion_r876058555
##########
core/src/main/java/org/apache/iceberg/DefaultValueParser.java:
##########
@@ -0,0 +1,253 @@
+/*
+ * 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 com.fasterxml.jackson.core.JsonFactory;
+import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.SerializerProvider;
+import com.fasterxml.jackson.databind.module.SimpleModule;
+import com.fasterxml.jackson.databind.ser.std.ByteBufferSerializer;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.time.LocalDateTime;
+import java.time.ZoneOffset;
+import java.time.format.DateTimeFormatter;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.stream.Collectors;
+import java.util.stream.StreamSupport;
+import org.apache.iceberg.exceptions.ValidationException;
+import org.apache.iceberg.expressions.Literal;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.relocated.com.google.common.io.BaseEncoding;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.Types;
+
+public class DefaultValueParser {
+ private DefaultValueParser() {
+ }
+
+ private static final JsonFactory FACTORY = new JsonFactory();
+ private static final ObjectMapper MAPPER;
+
+ static {
+ MAPPER = new ObjectMapper(FACTORY);
+ SimpleModule customModule = new SimpleModule();
+ customModule.addSerializer(ByteBuffer.class, new HexStringCustomByteBufferSerializer());
+ MAPPER.registerModule(customModule);
+ }
+
+ public static ObjectMapper mapper() {
+ return MAPPER;
+ }
+
+ public static Object parseDefaultFromJson(Type type, JsonNode jsonNode) {
+ if (jsonNode == null) {
+ return null;
+ }
+
+ switch (type.typeId()) {
+ case BOOLEAN:
+ return jsonNode.booleanValue();
Review Comment:
Let's match the approach that the other parsers take so we have consistency in the code.
--
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 #4525: API: Add default value core api and schema serialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4525:
URL: https://github.com/apache/iceberg/pull/4525#discussion_r876057391
##########
api/src/main/java/org/apache/iceberg/types/Types.java:
##########
@@ -415,43 +415,64 @@ public int hashCode() {
public static class NestedField implements Serializable {
public static NestedField optional(int id, String name, Type type) {
- return new NestedField(true, id, name, type, null);
+ return new NestedField(true, id, name, type, null, null, null);
}
public static NestedField optional(int id, String name, Type type, String doc) {
- return new NestedField(true, id, name, type, doc);
+ return new NestedField(true, id, name, type, doc, null, null);
+ }
+
+ public static NestedField optional(int id, String name, Type type, String doc,
+ Object initialDefault, Object writeDefault) {
+ return new NestedField(true, id, name, type, doc, initialDefault, writeDefault);
}
public static NestedField required(int id, String name, Type type) {
- return new NestedField(false, id, name, type, null);
+ return new NestedField(false, id, name, type, null, null, null);
}
public static NestedField required(int id, String name, Type type, String doc) {
- return new NestedField(false, id, name, type, doc);
+ return new NestedField(false, id, name, type, doc, null, null);
+ }
+
+ public static NestedField required(int id, String name, Type type, String doc,
+ Object initialDefault, Object writeDefault) {
+ return new NestedField(false, id, name, type, doc, initialDefault, writeDefault);
}
public static NestedField of(int id, boolean isOptional, String name, Type type) {
- return new NestedField(isOptional, id, name, type, null);
+ return new NestedField(isOptional, id, name, type, null, null, null);
}
public static NestedField of(int id, boolean isOptional, String name, Type type, String doc) {
- return new NestedField(isOptional, id, name, type, doc);
+ return new NestedField(isOptional, id, name, type, doc, null, null);
+ }
+
+ public static NestedField of(int id, boolean isOptional, String name, Type type, String doc,
+ Object initialDefault, Object writeDefault) {
+ return new NestedField(isOptional, id, name, type, doc, initialDefault, writeDefault);
}
private final boolean isOptional;
private final int id;
private final String name;
private final Type type;
private final String doc;
+ private final Object initialDefault;
+ private final Object writeDefault;
+
- private NestedField(boolean isOptional, int id, String name, Type type, String doc) {
+ private NestedField(boolean isOptional, int id, String name, Type type, String doc,
+ Object initialDefault, Object writeDefault) {
Review Comment:
@rzhang10, please don't resolve threads that are not updated yet.
--
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 #4525: API: Add default value core api and schema serialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4525:
URL: https://github.com/apache/iceberg/pull/4525#discussion_r876056384
##########
api/src/main/java/org/apache/iceberg/expressions/Literals.java:
##########
@@ -419,6 +420,9 @@ static class TimeLiteral extends ComparableLiteral<Long> {
public <T> Literal<T> to(Type type) {
if (type.typeId() == Type.TypeID.TIME) {
return (Literal<T>) this;
+ } else if (type.typeId() == Type.TypeID.STRING) {
+ return (Literal<T>) new StringLiteral(LocalTime.ofNanoOfDay(value() * 1000)
+ .format(DateTimeFormatter.ISO_LOCAL_TIME));
Review Comment:
I don't think this conversion should be done using literals. You can see how we're doing it here, but there is no need to change the `Literals` API. And there is certainly no need to add extra conversions to `Literals`. These are purposely limited.
--
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 #4525: API: Add default value core api and schema serialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4525:
URL: https://github.com/apache/iceberg/pull/4525#discussion_r846842293
##########
api/src/main/java/org/apache/iceberg/types/Types.java:
##########
@@ -492,6 +523,14 @@ public String doc() {
return doc;
}
+ public JsonNode initialDefaultValue() {
Review Comment:
This API should not use Jackson classes. Instead, it should use the standard Iceberg in-memory representation:
* boolean - Java primitive boolean
* int - Java primitive int
* long - Java primitive long
* float - Java primitive float
* double - Java primitive double
* date - Java primitive int
* time - Java primitive long
* timestamp/timestamptz - Java primitive long
* string - Java CharSequence
* uuid - Java UUID
* binary - Java ByteBuffer
* fixed(L) - Java ByteBuffer
* decimal(P, S) - Java BigDecimal
* list - Java Collection
* struct - StructLike
* map - Java Map
--
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 #4525: API: Add default value core api and schema serialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4525:
URL: https://github.com/apache/iceberg/pull/4525#discussion_r863327981
##########
core/src/main/java/org/apache/iceberg/DefaultValueParser.java:
##########
@@ -0,0 +1,253 @@
+/*
+ * 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 com.fasterxml.jackson.core.JsonFactory;
+import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.SerializerProvider;
+import com.fasterxml.jackson.databind.module.SimpleModule;
+import com.fasterxml.jackson.databind.ser.std.ByteBufferSerializer;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.time.LocalDateTime;
+import java.time.ZoneOffset;
+import java.time.format.DateTimeFormatter;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.stream.Collectors;
+import java.util.stream.StreamSupport;
+import org.apache.iceberg.exceptions.ValidationException;
+import org.apache.iceberg.expressions.Literal;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.relocated.com.google.common.io.BaseEncoding;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.Types;
+
+public class DefaultValueParser {
+ private DefaultValueParser() {
+ }
+
+ private static final JsonFactory FACTORY = new JsonFactory();
+ private static final ObjectMapper MAPPER;
+
+ static {
+ MAPPER = new ObjectMapper(FACTORY);
+ SimpleModule customModule = new SimpleModule();
+ customModule.addSerializer(ByteBuffer.class, new HexStringCustomByteBufferSerializer());
+ MAPPER.registerModule(customModule);
+ }
+
+ public static ObjectMapper mapper() {
+ return MAPPER;
+ }
+
+ public static Object parseDefaultFromJson(Type type, JsonNode jsonNode) {
+ if (jsonNode == null) {
+ return null;
+ }
+
+ switch (type.typeId()) {
+ case BOOLEAN:
+ return jsonNode.booleanValue();
+ case INTEGER:
+ return jsonNode.intValue();
+ case LONG:
+ return jsonNode.longValue();
+ case FLOAT:
+ return jsonNode.floatValue();
+ case DOUBLE:
+ return jsonNode.doubleValue();
+ case DECIMAL:
+ return jsonNode.decimalValue();
+ case STRING:
+ case UUID:
+ return jsonNode.textValue();
+ case DATE:
+ case TIME:
+ case TIMESTAMP:
+ return Literal.of(jsonNode.textValue()).to(type).value();
+ case FIXED:
+ byte[] fixedBytes = BaseEncoding.base16().decode(jsonNode.textValue().toUpperCase(Locale.ROOT).replaceFirst(
+ "^0X",
+ ""));
+ return ByteBuffer.allocate(((Types.FixedType) type).length()).put(fixedBytes);
+ case BINARY:
+ byte[] binaryBytes = BaseEncoding.base16().decode(jsonNode.textValue().toUpperCase(Locale.ROOT).replaceFirst(
+ "^0X", ""));
+ return ByteBuffer.wrap(binaryBytes);
+ case LIST:
+ List<Object> defaultList = Lists.newArrayList();
+ for (JsonNode element : jsonNode) {
+ defaultList.add(parseDefaultFromJson(type.asListType().elementType(), element));
+ }
+ return defaultList;
+ case MAP:
+ Map<Object, Object> defaultMap = Maps.newHashMap();
+ List<JsonNode> keysAndValues = StreamSupport
+ .stream(jsonNode.spliterator(), false)
+ .collect(Collectors.toList());
+ JsonNode keys = keysAndValues.get(0);
+ JsonNode values = keysAndValues.get(1);
+
+ List<JsonNode> keyList = Lists.newArrayList(keys.iterator());
+ List<JsonNode> valueList = Lists.newArrayList(values.iterator());
+
+ for (int i = 0; i < keyList.size(); i++) {
+ defaultMap.put(
+ parseDefaultFromJson(type.asMapType().keyType(), keyList.get(i)),
+ parseDefaultFromJson(type.asMapType().valueType(), valueList.get(i)));
+ }
+ return defaultMap;
+ case STRUCT:
+ Map<Integer, Object> defaultStruct = Maps.newHashMap();
+ for (Types.NestedField subField : type.asStructType().fields()) {
+ String fieldIdAsString = String.valueOf(subField.fieldId());
+ Object value = jsonNode.has(fieldIdAsString) ? parseDefaultFromJson(
+ subField.type(),
+ jsonNode.get(fieldIdAsString)) : null;
+ if (value != null) {
+ defaultStruct.put(subField.fieldId(), value);
+ }
+ }
+ return defaultStruct;
+ default:
+ return null;
+ }
+ }
+
+ public static JsonNode validateDefault(String name, Type type, JsonNode defaultValue) {
+ if (defaultValue != null && !isValidDefault(type, defaultValue)) {
+ throw new ValidationException("Invalid default value for field %s: %s not a %s", name, defaultValue, type);
+ }
+ return defaultValue;
+ }
+
+ @SuppressWarnings("checkstyle:CyclomaticComplexity")
Review Comment:
We generally don't suppress this warning unless we are trying to avoid major code changes. Since this is all new, it is best to follow the recommendation.
--
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 #4525: API: Add default value core api and schema serialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4525:
URL: https://github.com/apache/iceberg/pull/4525#discussion_r863327773
##########
core/src/main/java/org/apache/iceberg/DefaultValueParser.java:
##########
@@ -0,0 +1,253 @@
+/*
+ * 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 com.fasterxml.jackson.core.JsonFactory;
+import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.SerializerProvider;
+import com.fasterxml.jackson.databind.module.SimpleModule;
+import com.fasterxml.jackson.databind.ser.std.ByteBufferSerializer;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.time.LocalDateTime;
+import java.time.ZoneOffset;
+import java.time.format.DateTimeFormatter;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.stream.Collectors;
+import java.util.stream.StreamSupport;
+import org.apache.iceberg.exceptions.ValidationException;
+import org.apache.iceberg.expressions.Literal;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.relocated.com.google.common.io.BaseEncoding;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.Types;
+
+public class DefaultValueParser {
+ private DefaultValueParser() {
+ }
+
+ private static final JsonFactory FACTORY = new JsonFactory();
+ private static final ObjectMapper MAPPER;
+
+ static {
+ MAPPER = new ObjectMapper(FACTORY);
+ SimpleModule customModule = new SimpleModule();
+ customModule.addSerializer(ByteBuffer.class, new HexStringCustomByteBufferSerializer());
+ MAPPER.registerModule(customModule);
Review Comment:
The majority of the Iceberg parsers don't use `ObjectMapper`. Why is it needed 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] rdblue commented on a diff in pull request #4525: API: Add default value core api and schema serialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4525:
URL: https://github.com/apache/iceberg/pull/4525#discussion_r863327646
##########
core/src/main/java/org/apache/iceberg/util/JsonUtil.java:
##########
@@ -115,6 +115,17 @@ public static String getStringOrNull(String property, JsonNode node) {
return pNode.asText();
}
+ public static JsonNode getChildNodeOrNull(String property, JsonNode node) {
+ if (!node.has(property)) {
+ return null;
+ }
+ JsonNode pNode = node.get(property);
Review Comment:
Style: control flow blocks should be separated from the next statement by an empty newline.
--
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] rzhang10 commented on a diff in pull request #4525: API: Add default value core api and schema serialization
Posted by GitBox <gi...@apache.org>.
rzhang10 commented on code in PR #4525:
URL: https://github.com/apache/iceberg/pull/4525#discussion_r867273801
##########
api/src/main/java/org/apache/iceberg/expressions/Literal.java:
##########
@@ -71,6 +71,18 @@ static Literal<BigDecimal> of(BigDecimal value) {
return new Literals.DecimalLiteral(value);
}
+ static Literal<Integer> ofDateLiteral(int value) {
Review Comment:
See my below comment.
--
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
Re: [PR] API: Add default value core api and schema serialization [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4525:
URL: https://github.com/apache/iceberg/pull/4525#issuecomment-2274584244
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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 #4525: API: Add default value core api and schema serialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4525:
URL: https://github.com/apache/iceberg/pull/4525#discussion_r846841553
##########
core/src/main/java/org/apache/iceberg/SchemaParser.java:
##########
@@ -92,6 +94,14 @@ private static void toJson(Types.StructType struct, Integer schemaId, Set<Intege
if (field.doc() != null) {
generator.writeStringField(DOC, field.doc());
}
+ if (field.initialDefaultValue() != null) {
+ generator.writeFieldName(INITIAL_DEFAULT);
+ generator.writeRawValue(field.initialDefaultValue().toString());
+ }
+ if (field.writeDefaultValue() != null) {
+ generator.writeFieldName(WRITE_DEFAULT);
+ generator.writeRawValue(field.writeDefaultValue().toString());
Review Comment:
Instead of using `toString` that produces JSON, I think that this should expose a `ValueParser` that converts values from the internal Iceberg representation into JSON values.
--
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] rzhang10 commented on pull request #4525: API: Add default value core api and schema serialization
Posted by GitBox <gi...@apache.org>.
rzhang10 commented on PR #4525:
URL: https://github.com/apache/iceberg/pull/4525#issuecomment-1095460067
@rdblue
I see your major concern in the reviews are regarding using Jackson in Iceberg-api module.
Let me put my thoughts here in one place to answer all the questions above, the core problem is whether we should represent the default value in memory in `JsonNode` or Java primitives.
The reason I chose Jackson is that:
- Jackson has more robust json validation utils, which come in handy here.
- Jackson `JsonNoe` is also `java.io.Serializable`.
- Jackson is also used in the Iceberg-core module as a dependency. I believe whenever iceberg is used as a library, iceberg-api and iceberg-core will both be dependent upon. So I wouldn't believe introducing Jackson in the API module will (additionally) pollute downstream dependencies.
If we take the java primitive approach, I think my current code could be adapted this way:
Maintain the current json validation logic, then add a `ValueParser` with `toJson` and `fromJson` to convert between json and their respective java primitive representations. The structure of the current code will pretty much stay the same.
Let me know your thoughts and I'm open to further discussions too.
--
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 #4525: API: Add default value core api and schema serialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4525:
URL: https://github.com/apache/iceberg/pull/4525#discussion_r863326106
##########
api/src/main/java/org/apache/iceberg/types/Types.java:
##########
@@ -415,43 +415,64 @@ public int hashCode() {
public static class NestedField implements Serializable {
public static NestedField optional(int id, String name, Type type) {
- return new NestedField(true, id, name, type, null);
+ return new NestedField(true, id, name, type, null, null, null);
}
public static NestedField optional(int id, String name, Type type, String doc) {
- return new NestedField(true, id, name, type, doc);
+ return new NestedField(true, id, name, type, doc, null, null);
+ }
+
+ public static NestedField optional(int id, String name, Type type, String doc,
+ Object initialDefault, Object writeDefault) {
+ return new NestedField(true, id, name, type, doc, initialDefault, writeDefault);
}
public static NestedField required(int id, String name, Type type) {
- return new NestedField(false, id, name, type, null);
+ return new NestedField(false, id, name, type, null, null, null);
}
public static NestedField required(int id, String name, Type type, String doc) {
- return new NestedField(false, id, name, type, doc);
+ return new NestedField(false, id, name, type, doc, null, null);
+ }
+
+ public static NestedField required(int id, String name, Type type, String doc,
+ Object initialDefault, Object writeDefault) {
+ return new NestedField(false, id, name, type, doc, initialDefault, writeDefault);
}
public static NestedField of(int id, boolean isOptional, String name, Type type) {
- return new NestedField(isOptional, id, name, type, null);
+ return new NestedField(isOptional, id, name, type, null, null, null);
}
public static NestedField of(int id, boolean isOptional, String name, Type type, String doc) {
- return new NestedField(isOptional, id, name, type, doc);
+ return new NestedField(isOptional, id, name, type, doc, null, null);
+ }
+
+ public static NestedField of(int id, boolean isOptional, String name, Type type, String doc,
+ Object initialDefault, Object writeDefault) {
+ return new NestedField(isOptional, id, name, type, doc, initialDefault, writeDefault);
}
private final boolean isOptional;
private final int id;
private final String name;
private final Type type;
private final String doc;
+ private final Object initialDefault;
+ private final Object writeDefault;
+
Review Comment:
Style: avoid adding unnecessary newlines. One newline to separate blocks is sufficient.
--
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 #4525: API: Add default value core api and schema serialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4525:
URL: https://github.com/apache/iceberg/pull/4525#discussion_r863326742
##########
api/src/main/java/org/apache/iceberg/types/Types.java:
##########
@@ -473,7 +494,11 @@ public NestedField asRequired() {
if (!isOptional) {
return this;
}
- return new NestedField(false, id, name, type, doc);
+ return new NestedField(false, id, name, type, doc, initialDefault, writeDefault);
+ }
+
+ public NestedField updateWriteDefault(Object newWriteDefault) {
Review Comment:
There's precedent for returning a slightly modified version of the field with the `asRequired` and `asOptional` methods, but this method name doesn't make it clear to me that the original field isn't being modified. I think `withWriteDefault` is slightly better, although not great.
--
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 #4525: API: Add default value core api and schema serialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4525:
URL: https://github.com/apache/iceberg/pull/4525#discussion_r863326807
##########
api/src/main/java/org/apache/iceberg/types/Types.java:
##########
@@ -492,6 +517,14 @@ public String doc() {
return doc;
}
+ public Object initialDefaultValue() {
Review Comment:
Do we need `Value` here? It seems redundant. Maybe just `initialDefault` and `writeDefault`.
--
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] rzhang10 commented on a diff in pull request #4525: API: Add default value core api and schema serialization
Posted by GitBox <gi...@apache.org>.
rzhang10 commented on code in PR #4525:
URL: https://github.com/apache/iceberg/pull/4525#discussion_r847769422
##########
api/src/main/java/org/apache/iceberg/types/Types.java:
##########
@@ -492,6 +523,14 @@ public String doc() {
return doc;
}
+ public JsonNode initialDefaultValue() {
Review Comment:
> * list - Java Collection
I think using Java `List` is more correct, as List cares about the order.
> * struct - StructLike
I see this comment:
```
/**
* Interface for accessing data by position in a schema.
* <p>
* This interface supports accessing data in top-level fields, not in nested fields.
*/
```
in that interface, also I see the API exposes is getting fields by position, which doesn't suit our casing of accessing fields by field id.
I think using a native Java map would be better.
--
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 #4525: API: Add default value core api and schema serialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4525:
URL: https://github.com/apache/iceberg/pull/4525#discussion_r846842357
##########
api/src/main/java/org/apache/iceberg/types/Types.java:
##########
@@ -524,6 +563,91 @@ public boolean equals(Object o) {
public int hashCode() {
return Objects.hash(NestedField.class, id, isOptional, name, type);
}
+
+ private static JsonNode validateDefault(String name, Type type, JsonNode defaultValue) {
Review Comment:
I think this logic should be moved to a `ValueParser` with `toJson` and `fromJson` methods, like the others. We don't want to leak any Jackson classes in the API.
--
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 #4525: API: Add default value core api and schema serialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4525:
URL: https://github.com/apache/iceberg/pull/4525#discussion_r863327544
##########
core/src/main/java/org/apache/iceberg/DefaultValueParser.java:
##########
@@ -0,0 +1,253 @@
+/*
+ * 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 com.fasterxml.jackson.core.JsonFactory;
+import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.SerializerProvider;
+import com.fasterxml.jackson.databind.module.SimpleModule;
+import com.fasterxml.jackson.databind.ser.std.ByteBufferSerializer;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.time.LocalDateTime;
+import java.time.ZoneOffset;
+import java.time.format.DateTimeFormatter;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.stream.Collectors;
+import java.util.stream.StreamSupport;
+import org.apache.iceberg.exceptions.ValidationException;
+import org.apache.iceberg.expressions.Literal;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.relocated.com.google.common.io.BaseEncoding;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.Types;
+
+public class DefaultValueParser {
+ private DefaultValueParser() {
+ }
+
+ private static final JsonFactory FACTORY = new JsonFactory();
+ private static final ObjectMapper MAPPER;
+
+ static {
+ MAPPER = new ObjectMapper(FACTORY);
+ SimpleModule customModule = new SimpleModule();
+ customModule.addSerializer(ByteBuffer.class, new HexStringCustomByteBufferSerializer());
+ MAPPER.registerModule(customModule);
+ }
+
+ public static ObjectMapper mapper() {
+ return MAPPER;
+ }
+
+ public static Object parseDefaultFromJson(Type type, JsonNode jsonNode) {
+ if (jsonNode == null) {
+ return null;
+ }
+
+ switch (type.typeId()) {
+ case BOOLEAN:
+ return jsonNode.booleanValue();
Review Comment:
I think this should use the same pattern and checks that are used in other JSON parsers. Those use `JsonUtil` and validate the type. For example:
```java
public static int getInt(String property, JsonNode node) {
Preconditions.checkArgument(node.has(property), "Cannot parse missing int %s", property);
JsonNode pNode = node.get(property);
Preconditions.checkArgument(pNode != null && !pNode.isNull() && pNode.isNumber(),
"Cannot parse %s to an integer value: %s", property, pNode);
return pNode.asInt();
}
```
That checks that the node actually is an integer. We want to be similarly strict 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] rdblue commented on a diff in pull request #4525: API: Add default value core api and schema serialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4525:
URL: https://github.com/apache/iceberg/pull/4525#discussion_r863327057
##########
core/src/main/java/org/apache/iceberg/DefaultValueParser.java:
##########
@@ -0,0 +1,253 @@
+/*
+ * 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 com.fasterxml.jackson.core.JsonFactory;
+import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.SerializerProvider;
+import com.fasterxml.jackson.databind.module.SimpleModule;
+import com.fasterxml.jackson.databind.ser.std.ByteBufferSerializer;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.time.LocalDateTime;
+import java.time.ZoneOffset;
+import java.time.format.DateTimeFormatter;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.stream.Collectors;
+import java.util.stream.StreamSupport;
+import org.apache.iceberg.exceptions.ValidationException;
+import org.apache.iceberg.expressions.Literal;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.relocated.com.google.common.io.BaseEncoding;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.Types;
+
+public class DefaultValueParser {
Review Comment:
I think this class could be in a separate PR to start with. The representation and tests are fairly well-defined. Can you separate it out?
--
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 #4525: API: Add default value core api and schema serialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4525:
URL: https://github.com/apache/iceberg/pull/4525#discussion_r863326344
##########
api/src/main/java/org/apache/iceberg/types/Types.java:
##########
@@ -473,7 +494,11 @@ public NestedField asRequired() {
if (!isOptional) {
return this;
}
- return new NestedField(false, id, name, type, doc);
+ return new NestedField(false, id, name, type, doc, initialDefault, writeDefault);
+ }
+
+ public NestedField updateWriteDefault(Object newWriteDefault) {
Review Comment:
I don't think there are other methods like this. Is it worth adding a new pattern for just this? Why not add `updateDoc`? I think I'd rather copy where needed.
--
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] rzhang10 commented on a diff in pull request #4525: API: Add default value core api and schema serialization
Posted by GitBox <gi...@apache.org>.
rzhang10 commented on code in PR #4525:
URL: https://github.com/apache/iceberg/pull/4525#discussion_r867740082
##########
core/src/main/java/org/apache/iceberg/DefaultValueParser.java:
##########
@@ -0,0 +1,253 @@
+/*
+ * 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 com.fasterxml.jackson.core.JsonFactory;
+import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.SerializerProvider;
+import com.fasterxml.jackson.databind.module.SimpleModule;
+import com.fasterxml.jackson.databind.ser.std.ByteBufferSerializer;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.time.LocalDateTime;
+import java.time.ZoneOffset;
+import java.time.format.DateTimeFormatter;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.stream.Collectors;
+import java.util.stream.StreamSupport;
+import org.apache.iceberg.exceptions.ValidationException;
+import org.apache.iceberg.expressions.Literal;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.relocated.com.google.common.io.BaseEncoding;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.Types;
+
+public class DefaultValueParser {
Review Comment:
Anyways, I have created a minimum API-change-only PR: https://github.com/apache/iceberg/pull/4732 , which took into account all your above style comments. I think this PR should be quite straightforward to be merged. And I plan to submit all subsequent split PRs by rebasing this after this has been merged.
--
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