SebastianGruza commented on code in PR #3209: URL: https://github.com/apache/hugegraph/pull/3209#discussion_r4141939356
########## hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/server/ObjectMapperResolver.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.server; + +import com.fasterxml.jackson.databind.DeserializationFeature; +import com.fasterxml.jackson.databind.ObjectMapper; + +import jakarta.ws.rs.ext.ContextResolver; +import jakarta.ws.rs.ext.Provider; + +/** + * The Jackson mapper Jersey uses for REST request bodies. + * + * JSON fraction literals are read as BigDecimal instead of double, so a + * value such as {@code 12345678901234567890.10} reaches a DECIMAL property + * key exactly. Numeric keys are unaffected: DataType.valueToNumber accepts + * any Number and narrows it to the key's type as before. + */ +@Provider +public class ObjectMapperResolver implements ContextResolver<ObjectMapper> { + + private final ObjectMapper mapper; + + public ObjectMapperResolver() { + this.mapper = new ObjectMapper(); + this.mapper.enable(DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS); Review Comment: Right on both counts, thanks: the job's `alpha` and a DOUBLE `~default_value` would have gone through `JsonUtil.toJson` as strings. Done the way you propose as the first option: `ObjectMapperResolver` is gone, Jersey uses its default mapper. Exact fractions are read only by the `properties` field of `BatchAPI.JsonElement` (and through it by `JsonVertex`/`JsonEdge` in the single and batch APIs) via `PropertiesDeserializer`: a manual token walk, `VALUE_NUMBER_FLOAT -> getDecimalValue()`, every other type as before, key order kept. Everything outside `properties` gets Jackson's default types. Tests: `PropertiesDeserializerTest` (exact fractions only inside `properties`, `options.alpha` stays a `Double`, null, an array is rejected), `JsonUtilTest` (`{"alpha":0.85}` round-trips as a number), `VertexApiTest` (a DOUBLE key with `"~default_value": 1.5` comes back from GET as a number). A schema reload through a restart does not fit the API suite; the mechanism that broke it is gone. ########## hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/type/define/DataType.java: ########## @@ -143,6 +156,65 @@ public <V> Number valueToNumber(V value) { return number; } + /** + * Convert a value to BigDecimal: BigDecimal as is, any other Number and a + * decimal string through their exact decimal representation. Float and + * Double go through Number.toString(), i.e. the shortest string that + * round-trips the binary value, so a client that already holds a lossy + * double gets that double, exactly. + * + * @return the BigDecimal, or null if the value is not a Number or String + * @throws IllegalArgumentException if the string is not a decimal number + */ + /* Review Comment: Done: the constants with their bounds comment sit above the Javadoc, and the Javadoc directly above `valueToDecimal()`, as in the struct copy. ########## hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/query/ConditionQuery.java: ########## @@ -947,6 +947,11 @@ private static boolean numberEquals(Object number1, Object number2) { // Otherwise convert to BigDecimal to make two numbers comparable Number n1 = NumericUtil.convertToNumber(number1); Number n2 = NumericUtil.convertToNumber(number2); + if (n1 instanceof BigDecimal || n2 instanceof BigDecimal) { Review Comment: Correct, and this was the most serious gap in the PR. `QueryAdapter` (the core copy and the struct copy) now treats `BigDecimal` and `BigInteger` like the primitive wrappers and `Date`: the value gets a `valuecls`, and on the store side `context.deserialize(value, BigDecimal.class)` reads it from the literal's text, never through a double; lists (`IN`) take the same path from their first element. `ConditionQuery.numberEquals` in the struct copy got the same exact branch as core, and ranges were already exact (`NumericUtil.compareNumber` compares through `new BigDecimal(toString())`). Tests: `StoreSerializerTest.testConditionQueryBytesKeepBigDecimal` (eq, gte, in through `bytes()/fromBytes()`, a double unchanged) and `VertexApiTest`: a vertex filter without an index is refused (`NoIndexException`), so the test takes the path that really reaches the store, an edge query by vertex + label + `properties={"amount": ...}`. The edge with a 39-digit amount is returned for the exact value and not for the same value with the last digit changed; on hstore that goes through the pushdown and the Store's `FilterIterator`, 6/6 with the server and store jars from this commit (lab: 1 PD + 3 Stores). ########## hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/io/HugeGraphSONModule.java: ########## @@ -956,4 +961,56 @@ public Blob deserialize(JsonParser jsonParser, return Blob.wrap(bytes); } } + + private static class BigDecimalSerializer extends StdSerializer<BigDecimal> { + + public BigDecimalSerializer() { + super(BigDecimal.class); + } + + @Override + public void serialize(BigDecimal decimal, JsonGenerator jsonGenerator, + SerializerProvider provider) throws IOException { + jsonGenerator.writeString(decimal.toPlainString()); Review Comment: Done with the second of your options: the serializer writes `toPlainString()` only while the scale is within the DECIMAL bound (±128), and `toString()` beyond it, i.e. the scientific form: just as exact and parseable, but without expanding the exponent into characters. A property value is always within the bound (`PropertyKey` checks it), so nothing changes for graph data; a Gremlin result such as `1E+999999999` goes out as `"1E+999999999"`. The deserializer stays unbounded, since the output side now bounds it. `JsonUtilTest`: `1E+999999999`, `1E-129`, `1E+128` (the boundary, plain). ########## hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/type/define/DataType.java: ########## @@ -143,6 +156,65 @@ public <V> Number valueToNumber(V value) { return number; } + /** + * Convert a value to BigDecimal: BigDecimal as is, any other Number and a + * decimal string through their exact decimal representation. Float and + * Double go through Number.toString(), i.e. the shortest string that + * round-trips the binary value, so a client that already holds a lossy + * double gets that double, exactly. + * + * @return the BigDecimal, or null if the value is not a Number or String + * @throws IllegalArgumentException if the string is not a decimal number + */ + /* + * Bounds for a DECIMAL value: at most DECIMAL_MAX_PRECISION significant + * digits and an absolute scale of at most DECIMAL_MAX_SCALE. uint256 + * with 18 fraction digits is 96 digits, so both fit with room to spare, + * while "1E+999999999" (a few bytes on disk, a billion characters from + * toPlainString() on every read) is rejected before it is stored. + */ + public static final int DECIMAL_MAX_PRECISION = 128; + public static final int DECIMAL_MAX_SCALE = 128; + + 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 { + String text = value.toString().trim(); + try { + decimal = new BigDecimal(text); + } 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: Done in both copies: `scale < -DECIMAL_MAX_SCALE || scale > DECIMAL_MAX_SCALE` instead of `Math.abs`. Tests with `new BigDecimal(BigInteger.ONE, Integer.MIN_VALUE)`, `MIN_VALUE + 1` and `MAX_VALUE` in `DataTypeTest` and in the struct `PropertyKeyTest`. -- 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]
