SebastianGruza commented on code in PR #3209: URL: https://github.com/apache/hugegraph/pull/3209#discussion_r4145642127
########## hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/graph/PropertiesDeserializer.java: ########## @@ -0,0 +1,102 @@ +/* + * 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.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 Review Comment: Right, thanks. Done in the first of your forms: `~default_value` stays a number and arrives exactly. `user_data` in `PropertyKeyAPI.JsonPropertyKey` carries `@JsonDeserialize(using = UserdataDeserializer.class)`: the same token walk as `PropertiesDeserializer`, a fraction as `BigDecimal`, everything else as before, wrapped in `Userdata` (with the `~create_time` normalization). The same gap existed on the way back from the backend: `readUserdata` in `BinarySerializer` and `TextSerializer` parsed the JSON with the default mapper, so after a restart an exact default would have come back as a double; both now use `JsonUtil.fromJsonExact`, and `PropertyKey` normalizes `~default_value` to the key's runtime type on every userdata write path (`userdata(key, value)` and `userdata(Userdata)`, through `validValue`): a DOUBLE key keeps and returns `1.5` as a number, a DECIMAL key keeps the exact `BigDecimal` and returns it the way property values are returned. The first E2E run caught that wi thout this normalization a DOUBLE default would have come back from GET as a string. Tests: `PropertiesDeserializerTest.testPropertyKeyUserdataIsExact` (`0.1234567890123456789` as BigDecimal, a string and an int in userdata untouched, no userdata = null), `JsonUtilTest` for `fromJsonExact`, `VertexApiTest`: a DECIMAL key `fee` with `"~default_value": 0.1234567890123456789`, GET of the key returns every digit, a vertex created without `fee` gets the exact default. ########## hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/graph/PropertiesDeserializer.java: ########## @@ -0,0 +1,102 @@ +/* + * 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.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>> { + + @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); + } + + private static Map<String, Object> readObject(JsonParser parser) + 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)); + } + return object; + } + + private static List<Object> readArray(JsonParser parser) + throws IOException { + List<Object> array = new ArrayList<>(); + while (parser.nextToken() != JsonToken.END_ARRAY) { + array.add(readValue(parser)); + } + return array; + } + + private static Object readValue(JsonParser parser) throws IOException { + JsonToken token = parser.currentToken(); + switch (token) { + case START_OBJECT: + return readObject(parser); + case START_ARRAY: + return readArray(parser); + case VALUE_STRING: + return parser.getText(); + case VALUE_NUMBER_INT: + return parser.getNumberValue(); + case VALUE_NUMBER_FLOAT: + // Exact: the literal's digits, not the nearest double + return parser.getDecimalValue(); Review Comment: Right. `API.parseProperties` reads the parameter through `JsonUtil.fromJsonExact` (a fraction as BigDecimal), and `VertexAPI.list` / `EdgeAPI.list` normalize each plain value to its property key's type before building the traversal (`API.normalizeProperties`: `PropertyKey.validValue`; `P.*` predicates, collections and unknown keys are left as they are). DECIMAL keeps every digit, DOUBLE gets a double as before, INT an integral literal as before. `VertexApiTest`: the same edge filter as in round 3, but with a number literal instead of a string: the 39-digit amount hits, the same value with the last digit changed misses (on hstore through the pushdown). For what it is worth, the same test run by mistake against a build of the round-3 head reproduced exactly the miss you describe. ########## hugegraph-struct/src/main/java/org/apache/hugegraph/query/serializer/QueryAdapter.java: ########## @@ -48,8 +50,13 @@ public class QueryAdapter extends AbstractSerializerAdapter<Condition> { .build(); static boolean isPrimitive(Class clz) { + // Values whose class must travel with them: Gson reads an untyped Review Comment: Right. `RelationType.IN` and `NOT_IN` in both `Condition` copies (core and struct) go through `collectionContains`: `contains` first, then for numbers a comparison by value through `NumericUtil.compareNumber`, the same rule EQ uses; `1.0` is within `(1.00, 2.5)`, `"1.0"` is not. The value comparison applies only once a `BigDecimal` is on either side: the existing `testConditionIn` requires that an Integer 1 still does not match a Double 1.0, and that stays. Tests: `ConditionTest.testConditionInMatchesNumbersByValue` in unit/core and in struct. ########## 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: Done: apache/hugegraph-doc#504 adds `decimal` to the data type list in `restful-api/propertykey.md` (en and cn, with the 128/128 bounds and the note that the value travels as a JSON number with every digit, so a client must not parse it as a double) and `asDecimal() | BigDecimal` to the table in `hugegraph-client.md` (en and cn). Linked from the description, the TODO is gone. -- 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]
