imbajin commented on code in PR #3209: URL: https://github.com/apache/hugegraph/pull/3209#discussion_r4172617809
########## hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/graph/PropertiesDeserializer.java: ########## @@ -0,0 +1,138 @@ +/* + * 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.graph; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; + +import com.fasterxml.jackson.core.JsonFactory; +import com.fasterxml.jackson.core.JsonParser; +import com.fasterxml.jackson.core.JsonToken; +import com.fasterxml.jackson.databind.DeserializationContext; +import com.fasterxml.jackson.databind.JsonDeserializer; +import com.fasterxml.jackson.databind.JsonMappingException; + +/** + * Reads the "properties" object of a vertex or edge body so that a JSON + * fraction keeps every digit: it becomes a BigDecimal instead of a double, + * which is what a DECIMAL property key needs and what every numeric key + * narrows through DataType.valueToNumber as before. Only property values + * are read this way; the rest of the request body (job parameters, schema + * userdata, query options) keeps Jackson's default number types. + */ +public class PropertiesDeserializer extends JsonDeserializer<Map<String, Object>> { + + private static final JsonFactory FACTORY = new JsonFactory(); + + @Override + public Map<String, Object> deserialize(JsonParser parser, + DeserializationContext ctxt) + throws IOException { + JsonToken token = parser.currentToken(); + if (token == JsonToken.VALUE_NULL) { + return null; + } + if (token != JsonToken.START_OBJECT) { + throw JsonMappingException.from(parser, + "Expected an object for 'properties', but got " + token); + } + return readObject(parser, true); + } + + /** + * The properties filter of the list APIs, read by the same rule as a + * request body: exact fractions for the values of the object and the + * members of a top-level array, Jackson's number types inside an OBJECT + * value, so a filter on an OBJECT key equals the stored map. + */ + public static Map<String, Object> parse(String json) throws IOException { + try (JsonParser parser = FACTORY.createParser(json)) { + if (parser.nextToken() != JsonToken.START_OBJECT) { + throw JsonMappingException.from(parser, + "Expected an object for 'properties', but got " + parser.currentToken()); + } + Map<String, Object> properties = readObject(parser, true); + if (parser.nextToken() != null) { + throw JsonMappingException.from(parser, "Trailing content after 'properties'"); + } + return properties; + } + } + + /** + * `exact` holds for the values of the properties object itself and the + * elements of a top-level array (a DECIMAL value or a DECIMAL LIST/SET); + * anything nested deeper is the content of an OBJECT property and keeps + * Jackson's default number types. + */ + private static Map<String, Object> readObject(JsonParser parser, boolean exact) + throws IOException { + Map<String, Object> object = new LinkedHashMap<>(); + while (parser.nextToken() != JsonToken.END_OBJECT) { + String name = parser.currentName(); + parser.nextToken(); + object.put(name, readValue(parser, exact)); + } + return object; + } + + private static List<Object> readArray(JsonParser parser, boolean exact) + throws IOException { + List<Object> array = new ArrayList<>(); + while (parser.nextToken() != JsonToken.END_ARRAY) { + array.add(readValue(parser, exact)); + } + return array; + } + + /** One value at the parser's current token: exact fractions, nested objects and arrays. */ + public static Object readValue(JsonParser parser) throws IOException { + return readValue(parser, true); + } + + private static Object readValue(JsonParser parser, boolean exact) throws IOException { + JsonToken token = parser.currentToken(); + switch (token) { + case START_OBJECT: + return readObject(parser, false); + case START_ARRAY: + // a top-level array is a LIST/SET value: its members are exact; + // an array inside an array or an object is OBJECT content + return readArray(parser, exact && !parser.getParsingContext().getParent().inArray()); + case VALUE_STRING: + return parser.getText(); + case VALUE_NUMBER_INT: + return parser.getNumberValue(); + case VALUE_NUMBER_FLOAT: + // Exact for a property value: the literal's digits, not the nearest double + return exact ? parser.getDecimalValue() : (Object) parser.getDoubleValue(); Review Comment: ⚠️ Blocking: yes. Summary: This parser turns every top-level fraction into BigDecimal before the property type is known, so an existing OBJECT scalar or collection member is emitted and, on TextSerializer, persisted as a JSON string. Please preserve OBJECT's prior numeric type while keeping DECIMAL exact, and add scalar/collection round-trip coverage. Evidence: PropertyKey accepts BigDecimal unchanged for OBJECT; the shared serializer writes strings and TextSerializer reloads OBJECT values through Object.class. ########## hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/type/define/DataType.java: ########## @@ -43,7 +45,14 @@ public enum DataType implements SerialEnum { TEXT(8, "text", String.class), BLOB(9, "blob", Blob.class), DATE(10, "date", Date.class), - UUID(11, "uuid", UUID.class); + UUID(11, "uuid", UUID.class), + /* + * Arbitrary-precision decimal (java.math.BigDecimal). Stored exactly; not a + * "number" in the isNumber() sense because it has no fixed-width, sortable + * encoding, so it can't be a sort key, a range/secondary index field or an + * OLAP range property. + */ + DECIMAL(12, "decimal", BigDecimal.class); Review Comment: 🧹 Blocking: no. Summary: DECIMAL is now a REST data_type, but the OpenAPI description in PropertyKeyAPI still enumerates the old types and omits it. Please update that generated-schema description as well; the paired website documentation does not change this annotation. Evidence: PropertyKeyAPI.JsonPropertyKey.dataType at lines 242–245. ########## hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/business/GraphStoreIterator.java: ########## @@ -255,6 +257,12 @@ private <P extends BaseProperty<?>> List<Graphpb.Property> buildProperties( variant.setType(VariantType.VT_DOUBLE) .setValueDouble((Double) v); break; + case DECIMAL: Review Comment: 🧹 Blocking: no. Summary: This adds Store-side VT_STRING encoding for DECIMAL scalars and LIST/SET payloads, but no Store test exercises GraphStoreIterator or the emitted Variant. Please add exact-payload assertions for scalar and collection values to pin this wire contract. Evidence: the hugegraph-store test sources contain no GraphStoreIterator or DECIMAL payload case. ########## hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/schema/PropertyKey.java: ########## @@ -368,6 +454,10 @@ private <V> V convSingleValue(V value) { @SuppressWarnings("unchecked") V blob = (V) this.dataType().valueToBlob(value); return blob; + } else if (this.dataType().isDecimal()) { + @SuppressWarnings("unchecked") + V decimal = (V) this.dataType().valueToDecimal(value); Review Comment: ⚠️ Blocking: no. Summary: This conversion preserves BigDecimal scale, but SET normalization uses LinkedHashSet and BigDecimal.equals, so [1.0, 1.00] remains two values although DECIMAL conditions compare them numerically. Please deduplicate DECIMAL SET members by numeric value in both core and Struct and add a differing-scale case. Evidence: Cardinality.SET promises distinct values; the new Condition helper treats these decimals as equal. -- 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]
