SebastianGruza commented on code in PR #771: URL: https://github.com/apache/hugegraph-toolchain/pull/771#discussion_r4145643430
########## hugegraph-client/src/main/java/org/apache/hugegraph/serializer/BigDecimalSerializer.java: ########## @@ -0,0 +1,48 @@ +/* + * 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.hugegraph.serializer; + +import java.io.IOException; +import java.math.BigDecimal; + +import com.fasterxml.jackson.core.JsonGenerator; +import com.fasterxml.jackson.databind.SerializerProvider; +import com.fasterxml.jackson.databind.ser.std.StdSerializer; + +/** + * Write a BigDecimal as a plain JSON number ("1.10", never "1.1E+2"). + * Jackson's default is the scientific form of BigDecimal.toString(); the + * plain form carries every digit, so a server that reads fractions as + * BigDecimal (apache/hugegraph#3209) stores a DECIMAL value exactly, and a + * server that reads them as double behaves as it always did. The value stays + * a number, so numeric keys (DOUBLE, FLOAT, LONG, INT) accept it too. + */ +public class BigDecimalSerializer extends StdSerializer<BigDecimal> { + + private static final long serialVersionUID = 1L; + + public BigDecimalSerializer() { + super(BigDecimal.class); + } + + @Override + public void serialize(BigDecimal value, JsonGenerator generator, + SerializerProvider provider) throws IOException { + generator.writeNumber(value.toPlainString()); Review Comment: Right. `exactString()`: the plain form only while the value is within the DECIMAL bounds (128 digits, scale within ±128), `toString()` beyond them, i.e. the scientific form: still an exact JSON number, and `1E+999999999` stays eleven characters for the server to reject on its side. The same rule as `HugeGraphSONModule` in apache/hugegraph#3209. `testSerializerDoesNotExpandOutOfBoundsValues`: `1E+3 -> 1000`, `1E-128` plain, `1E+999999999` and `1E-129` scientific, through `JsonUtil.toJson`. ########## hugegraph-client/src/main/java/org/apache/hugegraph/structure/constant/DataType.java: ########## @@ -71,6 +80,66 @@ public boolean isUUID() { return this == DataType.UUID; } + public boolean isDecimal() { + return this == DataType.DECIMAL; + } + + /* + * Bounds for a DECIMAL value, the same as the server applies: at most + * DECIMAL_MAX_PRECISION significant digits and an absolute scale of at + * most DECIMAL_MAX_SCALE. A value such as "1E+999999999" costs a few bytes + * to store and a billion characters on every read, and the direct loaders + * write bytes into storage without the server's check. + */ + public static final int DECIMAL_MAX_PRECISION = 128; + public static final int DECIMAL_MAX_SCALE = 128; + + /** + * Convert a value to BigDecimal the same way the server does: BigDecimal + * as is, integral numbers exactly, any other Number and a decimal string + * through their decimal representation, then checked against the bounds. + * + * @return the BigDecimal, or null if the value can't be a decimal + * @throws IllegalArgumentException if the string is not a decimal number + */ + public <V> BigDecimal valueToDecimal(V value) { + if (!this.isDecimal()) { + return null; + } + BigDecimal decimal; + if (value instanceof BigDecimal) { + decimal = (BigDecimal) value; + } else if (value instanceof BigInteger) { + decimal = new BigDecimal((BigInteger) value); + } else if (value instanceof Byte || value instanceof Short || + value instanceof Integer || value instanceof Long) { + decimal = BigDecimal.valueOf(((Number) value).longValue()); + } else if (!(value instanceof Number) && !(value instanceof String)) { + return null; + } else { + try { + decimal = new BigDecimal(value.toString().trim()); + } catch (NumberFormatException e) { + throw new IllegalArgumentException(String.format( + "Can't read '%s' as decimal", value)); + } + } + return checkDecimalBounds(decimal); + } + + public static BigDecimal checkDecimalBounds(BigDecimal decimal) { + int scale = Math.abs(decimal.scale()); Review Comment: Right, the same slip I fixed on the server side in round 3. Both copies compare the scale to both limits directly. `testDecimalBounds` checks `new BigDecimal(BigInteger.ONE, Integer.MIN_VALUE)` and `MAX_VALUE` on both copies and on `BytesBuffer.writeProperty`. ########## hugegraph-loader/src/main/java/org/apache/hugegraph/loader/util/DataTypeUtil.java: ########## @@ -223,10 +235,23 @@ private static Object parseMultiValues(String key, Object values, DataType dataType, Cardinality cardinality, InputSource source) { - // JSON file should not parse again - if (values instanceof Collection && - checkCollectionDataType(key, (Collection<?>) values, dataType)) { - return values; + if (values instanceof Collection) { + // JSON file should not parse again + if (checkCollectionDataType(key, (Collection<?>) values, dataType)) { Review Comment: Right. The DECIMAL branch of `checkDataType` counts an element as already typed only when it is a `BigDecimal` within the bounds; otherwise the list goes through the element-by-element conversion and `parseDecimal` rejects it like the scalar. `testConvertDecimalList`: `[1E+129]` and `[1E+129, 2]` rejected. ########## hugegraph-loader/src/main/java/org/apache/hugegraph/loader/util/JsonUtil.java: ########## @@ -47,6 +48,15 @@ public final class JsonUtil { private static final Logger LOG = Log.logger(JsonUtil.class); private static final ObjectMapper MAPPER = new ObjectMapper(); + static { + /* + * Read JSON fraction literals as BigDecimal: a DECIMAL column keeps + * every digit (a double would keep 17), the other numeric types are + * narrowed by DataTypeUtil as before. + */ + MAPPER.enable(DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS); Review Comment: Right. `ElementBuilder.isNullValue`: `contains` first, then for numbers a comparison by value through `BigDecimal.compareTo` (both sides are BigDecimal now from the same mapper; Long/Double values from JDBC or CSV match too). New `NullValuesTest` in the loader `UnitTestSuite`: a mapping with `null_values: [1.0, ""]` parsed through `JsonUtil`, a JSON row `1.00` matches, `1.5` does not, `""` matches, `1L` and `1.0d` match, `"1.0"` does not. ########## hugegraph-client/src/test/java/org/apache/hugegraph/api/DecimalPropertyApiTest.java: ########## @@ -0,0 +1,225 @@ +/* + * 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.hugegraph.api; + +import java.math.BigDecimal; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; + +import org.apache.hugegraph.exception.ServerException; +import org.apache.hugegraph.structure.constant.DataType; +import org.apache.hugegraph.structure.constant.T; +import org.apache.hugegraph.structure.graph.BatchVertexRequest; +import org.apache.hugegraph.structure.graph.UpdateStrategy; +import org.apache.hugegraph.structure.graph.Vertex; +import org.apache.hugegraph.structure.gremlin.ResultSet; +import org.apache.hugegraph.structure.schema.PropertyKey; +import org.apache.hugegraph.structure.schema.VertexLabel; +import org.apache.hugegraph.testutil.Assert; +import org.apache.hugegraph.testutil.Utils; +import org.junit.After; +import org.junit.Assume; +import org.junit.BeforeClass; +import org.junit.Test; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; + +/** + * DECIMAL property keys: exact values across create/read/batch-SUM. The + * whole class is skipped against a server that has no DECIMAL data type yet. + */ +public class DecimalPropertyApiTest extends BaseApiTest { + + private static final String UINT256_MAX = + "115792089237316195423570985008687907853" + + "269984665640564039457584007913129639935"; + + @BeforeClass + public static void prepareSchema() { + PropertyKey amount = new PropertyKey.BuilderImpl("amount", null) + .asDecimal().build(); + try { + propertyKeyAPI.create(amount); + } catch (ServerException e) { + Assume.assumeTrue("The server has no DECIMAL data type: " + Review Comment: Done: the skip applies only when the message is the enum rejection during deserialization (`DataType`, `"DECIMAL"`, "not one of the values accepted", which is exactly what 1.7.0 returns); any other `ServerException` propagates. Checked against a 1.7.0 server on the lab: 1 skipped with that message. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
