SebastianGruza commented on code in PR #3209:
URL: https://github.com/apache/hugegraph/pull/3209#discussion_r4165456634
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/serializer/BinarySerializer.java:
##########
@@ -1237,8 +1237,8 @@ private void readUserdata(SchemaElement schema) {
byte[] userdataBytes = column(HugeKeys.USER_DATA);
String userdataStr = StringEncoding.decode(userdataBytes);
@SuppressWarnings("unchecked")
- Map<String, Object> userdata = JsonUtil.fromJson(userdataStr,
- Map.class);
+ Map<String, Object> userdata = JsonUtil.fromJsonExact(userdataStr,
Review Comment:
Right, thanks for checking both paths on the shaded Jackson. Both
`readUserdata()` methods (Binary and Text) are back to
`JsonUtil.fromJson(userdataStr, Map.class)`; a DECIMAL default is stored as a
string and `normalizeDefaultValue()` restores it exactly. Tests
`testUserdataTypesSurviveAReload` in `TextSerializerTest` and
`BinarySerializerTest`: a DECIMAL key with a default and a vertex label with
`rate: 0.85`, two round trips through the serializer, the first still the exact
`BigDecimal`, the second still a `Double`.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/API.java:
##########
@@ -222,14 +225,41 @@ protected static void checkUpdatingBody(Collection<?
extends Checkable> bodies)
}
@SuppressWarnings("unchecked")
Review Comment:
Done: `@SuppressWarnings("unchecked")` is back directly above
`parseProperties()`, and the `normalizeDefaultValue()` Javadoc moved onto that
method in `PropertyKey`.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/graph/PropertiesDeserializer.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * 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;
+ }
+
+ /** One value at the parser's current token: exact fractions, nested
objects and arrays. */
+ public 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. Exact parsing now applies only to values at the top level of
`properties` and to the members of top-level arrays; the contents of a nested
object (an OBJECT value) are read with Jackson's types (a fraction is a
`Double`). Unit case in `PropertiesDeserializerTest` plus an API case in
`VertexApiTest`: an OBJECT key `meta` with `{"ratio":0.25,"n":3}`, a number and
not a string in the POST and GET responses.
That API case found a master bug on the way: no map-valued OBJECT property
could be returned through REST at all (`Failed to serialize property`,
confirmed with master code on the lab). Cause in
`HugeGraphSONModule.writePropertiesField()`:
`provider.findValueSerializer(val.getClass())` returns an uncontextualized
`MapSerializer` with `keySerializer == null`, and the exception was rethrown
without its cause. Fixed in 371275c02 with `provider.defaultSerializeField(key,
val, generator)` and the cause kept on the `HugeException`; test
`testObjectMapPropertyInVertexJson` in `HugeGraphSONModuleTest`. If you prefer
that as a separate PR, I will split it out.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/schema/PropertyKey.java:
##########
@@ -116,7 +117,70 @@ public PropertyKey properties(Id... properties) {
public void defineDefaultValue(Object value) {
// TODO add a field default_value
- this.userdata().put(Userdata.DEFAULT_VALUE, value);
+ this.userdata().put(Userdata.DEFAULT_VALUE,
this.normalizeDefaultValue(value));
+ }
+
+ /**
+ * A DECIMAL key keeps its default value in userdata as the exact
+ * BigDecimal (a JSON fraction arrives as BigDecimal, a string is parsed),
+ * both when the key is defined through the API and when it is read back
+ * from the backend, so the value serializes the same way on every path.
+ * Every other data type keeps the raw value the user sent, as before:
+ * {@link #defaultValue()} converts it lazily when it is applied; only a
+ * BigDecimal (which exists solely because the API reads the default
+ * exactly) becomes the Double the default parser produced on master.
+ */
+ /** A BigDecimal from the exact parser as the Double the default parser
gave. */
+ private static Object undoExact(Object value) {
+ if (value instanceof BigDecimal) {
+ return ((BigDecimal) value).doubleValue();
+ }
+ if (value instanceof Collection) {
+ List<Object> values = new ArrayList<>(((Collection<?>)
value).size());
+ for (Object member : (Collection<?>) value) {
+ values.add(undoExact(member));
+ }
+ return value instanceof Set ? new LinkedHashSet<>(values) : values;
+ }
+ return value;
+ }
+
+ @Override
+ public void userdata(String key, Object value) {
+ if (Userdata.DEFAULT_VALUE.equals(key)) {
+ value = this.normalizeDefaultValue(value);
+ }
+ super.userdata(key, value);
+ }
+
+ @Override
+ public void userdata(Userdata userdata) {
+ E.checkArgumentNotNull(userdata, "userdata");
+ for (Map.Entry<String, Object> e : userdata.entrySet()) {
+ this.userdata(e.getKey(), e.getValue());
+ }
+ }
+
+ private Object normalizeDefaultValue(Object value) {
+ if (value == null) {
+ return value;
+ }
+ if (this.dataType != DataType.DECIMAL) {
+ return undoExact(value);
+ }
+ Object raw = value;
+ if (this.cardinality == Cardinality.SET && value instanceof Collection
&&
+ !(value instanceof Set)) {
+ raw = new LinkedHashSet<>((Collection<?>) value);
+ }
+ try {
+ Object valid = this.validValue(raw);
+ return valid != null ? valid : value;
+ } catch (RuntimeException e) {
Review Comment:
Right. `normalizeDefaultValue(value, lenient)`: the bulk path
`userdata(Userdata)` (builder create/append) is strict and throws through
`validValueOrThrow()`, the per-entry path `userdata(String, Object)` (storage
reload) is lenient and keeps the raw value. Tests in `PropertyKeyUserdataTest`:
create rejects `"not-a-number"` and `1E+999999999`, the per-entry reload keeps
the raw value.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/query/Condition.java:
##########
@@ -280,6 +281,29 @@ public boolean test(Object first, Object second) {
return this.tester.apply(first, second);
}
+ /**
+ * Membership by value once a BigDecimal is involved: its equals() is
+ * scale sensitive (1.0 vs 1.00), while EQ compares decimals by value,
+ * so IN/NOT_IN do the same. Other numbers keep contains() semantics
+ * (an Integer 1 is not a Double 1.0 here, as before).
+ */
+ private static boolean collectionContains(Collection<?> values, Object
value) {
Review Comment:
Right. CONTAINS in both copies (core and struct) goes through
`collectionContains()`, which compares by value only when a `BigDecimal` is
involved. Differing-scale cases (`1.00` vs `1.0`) in the core and struct
`ConditionTest`.
--
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]
