imbajin commented on code in PR #771:
URL:
https://github.com/apache/hugegraph-toolchain/pull/771#discussion_r4144177254
##########
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:
⚠️ This new collection fast path returns an already typed BigDecimal list,
but the DECIMAL `checkDataType` branch only checks the class and never applies
the 128/128 bound used by scalar `parseDecimal`. Thus `[1E+129]` passes loader
conversion while the same scalar is rejected. Validate every DECIMAL element
before returning the collection.
##########
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:
🧹 This assumption catches every `ServerException` while creating the DECIMAL
key and skips the whole class, including failures from a server that supports
DECIMAL. A regression or unrelated server error can therefore make the API
tests appear skipped. Skip only the known unsupported-type response and rethrow
other errors.
##########
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:
⚠️ This shared request serializer materializes `value.toPlainString()` for
every BigDecimal without checking the DECIMAL bounds or output length. `new
BigDecimal("1E+999999999")` needs roughly a billion plain characters and can
exhaust the client heap before the server rejects it. Use a bounded or
scientific JSON representation, or reject out-of-range values before expansion.
##########
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:
⚠️ This mapper change also parses numeric `null_values` configuration and
JSON rows as BigDecimal. `ElementBuilder.retainField()` uses `Set.contains`, so
`null_values: [1.0]` no longer matches a JSON field `1.00` (`BigDecimal.equals`
is scale-sensitive), although the previous Double values matched. Normalize
numeric null sentinels or compare them numerically.
##########
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:
⚠️ `Math.abs(Integer.MIN_VALUE)` remains negative, so this check accepts
`new BigDecimal(BigInteger.ONE, Integer.MIN_VALUE)`; the direct
`BytesBuffer.writeProperty` path can then persist a DECIMAL outside the stated
±128 scale bound. Compare scale directly to both limits here and in the
duplicate check in `serializer/direct/struct/DataType`.
--
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]
