feat(core): add DECIMAL (BigDecimal) property data type #3209
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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); | ||
| } | ||
|
|
||
| @Override | ||
| public ObjectMapper getContext(Class<?> type) { | ||
| return this.mapper; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ | |
|
|
||
| import java.io.File; | ||
| import java.io.IOException; | ||
| import java.math.BigDecimal; | ||
| import java.text.DateFormat; | ||
| import java.text.ParseException; | ||
| import java.text.SimpleDateFormat; | ||
|
|
@@ -183,6 +184,10 @@ public static void registerCommonSerializers(SimpleModule module) { | |
|
|
||
| module.addSerializer(Blob.class, new BlobSerializer()); | ||
| module.addDeserializer(Blob.class, new BlobDeserializer()); | ||
|
|
||
| // Decimals travel as strings: JSON numbers are doubles to most clients | ||
| module.addSerializer(BigDecimal.class, new BigDecimalSerializer()); | ||
|
Contributor
There was a problem hiding this comment. 🧹 Minor. This goes through Requested change: add the V1 number-to-string change, and the string in
Contributor
Author
There was a problem hiding this comment. Right, the PR description was inaccurate there. Added to "Note on compatibility" and for the release notes: through |
||
| module.addDeserializer(BigDecimal.class, new BigDecimalDeserializer()); | ||
| } | ||
|
|
||
| public static void registerIdSerializers(SimpleModule module) { | ||
|
|
@@ -956,4 +961,56 @@ public Blob deserialize(JsonParser jsonParser, | |
| return Blob.wrap(bytes); | ||
| } | ||
| } | ||
|
|
||
| private static class BigDecimalSerializer extends StdSerializer<BigDecimal> { | ||
|
Contributor
There was a problem hiding this comment.
Contributor
Author
There was a problem hiding this comment. Done in 9d5eaab, and thanks for checking this through the real serializers, I had only tested Result: V1 gives Test: new End to end on the lab, dists from both heads, hstore and rocksdb ( |
||
|
|
||
| public BigDecimalSerializer() { | ||
| super(BigDecimal.class); | ||
| } | ||
|
|
||
| @Override | ||
| public void serialize(BigDecimal decimal, JsonGenerator jsonGenerator, | ||
| SerializerProvider provider) throws IOException { | ||
| jsonGenerator.writeString(decimal.toPlainString()); | ||
|
Member
There was a problem hiding this comment. |
||
| } | ||
|
|
||
| @Override | ||
| public void serializeWithType(BigDecimal decimal, | ||
| JsonGenerator jsonGenerator, | ||
| SerializerProvider provider, | ||
| TypeSerializer typeSer) | ||
| throws IOException { | ||
| /* | ||
| * The typed GraphSON mappers (v2/v3) call this variant and | ||
| * StdSerializer does not implement it. Keep the type prefix so | ||
| * that the value stays "gx:BigDecimal", but carry the plain | ||
| * string inside it: a JSON number would be read as a double by | ||
| * most clients, which is what this type exists to avoid. | ||
| */ | ||
| WritableTypeId typeId = typeSer.typeId(decimal, | ||
| JsonToken.VALUE_STRING); | ||
| typeSer.writeTypePrefix(jsonGenerator, typeId); | ||
| this.serialize(decimal, jsonGenerator, provider); | ||
| typeSer.writeTypeSuffix(jsonGenerator, typeId); | ||
| } | ||
| } | ||
|
|
||
| private static class BigDecimalDeserializer extends StdDeserializer<BigDecimal> { | ||
|
|
||
| public BigDecimalDeserializer() { | ||
| super(BigDecimal.class); | ||
| } | ||
|
|
||
| @Override | ||
| public BigDecimal deserialize(JsonParser jsonParser, | ||
| DeserializationContext ctxt) | ||
| throws IOException { | ||
| JsonToken token = jsonParser.getCurrentToken(); | ||
| if (token == JsonToken.VALUE_NUMBER_INT || | ||
| token == JsonToken.VALUE_NUMBER_FLOAT) { | ||
| return jsonParser.getDecimalValue(); | ||
| } | ||
| return new BigDecimal(jsonParser.getText().trim()); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -114,6 +114,12 @@ public IndexLabel build() { | |
| indexLabel.indexType(this.indexType); | ||
| for (String field : this.indexFields) { | ||
| PropertyKey propertyKey = graph.propertyKey(field); | ||
| // Also guarded in checkFields(), but build() is reached directly | ||
| // by the OLAP property-key path, which skips checkFields() | ||
| E.checkArgument(!propertyKey.dataType().isDecimal(), | ||
| "Not allowed to build index on property key " + | ||
| "'%s' whose data type is decimal", | ||
| propertyKey.name()); | ||
| indexLabel.indexField(propertyKey.id()); | ||
| } | ||
| indexLabel.userdata(this.userdata); | ||
|
|
@@ -472,6 +478,9 @@ private void checkFields(Set<Id> propertyIds) { | |
| E.checkArgument(pkey.aggregateType().isIndexable(), | ||
| "The aggregate type %s is not indexable", | ||
| pkey.aggregateType()); | ||
| E.checkArgument(!pkey.dataType().isDecimal(), | ||
|
Contributor
There was a problem hiding this comment.
Contributor
Author
There was a problem hiding this comment. Done in 9d5eaab, both things you asked for: Test: |
||
| "Not allowed to build index on property key " + | ||
| "'%s' whose data type is decimal", pkey.name()); | ||
|
|
||
| if (pkey.cardinality().multiple()) { | ||
| E.checkArgument(fields.size() == 1, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
USE_BIG_DECIMAL_FOR_FLOATSon the shared Jersey mapper changes every untypedObjectin every REST body, not only vertex and edge properties. Combined with theBigDecimalSerializerthis PR adds toregisterCommonSerializers()(whichJsonUtiluses), any fraction that the server re-encodes withJsonUtil.toJson()now comes back as a JSON string, and existing code that expects aNumberrejects it.Two concrete paths:
AlgorithmAPI.post()andComputerAPI.post()takeMap<String, Object> parametersand storeJsonUtil.toJson(input)as the task input.AlgorithmJob.execute()/ComputerJobread it back withJsonUtil.fromJson(input, Map.class), andParameterUtil.parameterDouble()checksvalue instanceof Number. SoPOST /graphs/{graph}/jobs/algorithm/page_rank {"alpha": 0.85}passes the synchronousAlgorithmJob.check()(the value is still aBigDecimalthere) and then the task fails withExpect double value for parameter 'alpha': '0.85'. The same applies toprecisioninAbstractCommAlgorithm/AbstractComputerandalphainPageRankComputer.JsonPropertyKey.userdata(and the vertex/edge/index label equivalents) is aMap<String, Object>, persisted byBinarySerializer/TextSerializerthroughJsonUtil.toJson(schema.userdata()). A DOUBLE key created with"userdata": {"~default_value": 1.5}is stored as"1.5". After a reload from the backend (restart, schema cache miss, another server node),PropertyKey.defaultValue()callsvalidValueOrThrow("1.5"),DataType.valueToNumber()returns null for aString, andHugeElementline 102 throwsInvalid property value '1.5'when filling defaults. Before this commit the value was aDoubleand round-tripped as a number.I reproduced the round trip with plain Jackson 2.9: a mapper with
USE_BIG_DECIMAL_FOR_FLOATSreads{"alpha":0.85}asBigDecimal; a mapper with the sameBigDecimaltowriteString(toPlainString())serializer writes the task input as{"parameters":{"alpha":"0.85",...}}; reading it back givesjava.lang.String,instanceof Number == false. CI is green because no API test sends a fractional algorithm parameter or a fractional default value.Requested change: scope exact-decimal parsing to property values instead of flipping the global mapper, for example a content deserializer on
JsonElement.properties(or convertingBigDecimalback toDoublefor DOUBLE/FLOAT keys only where the key is known), and drop the globalObjectMapperResolver. If the global switch is kept, the job and userdata paths need explicit normalisation plus API tests: an algorithm job with a fractional parameter, and a DOUBLE property key with a fractional~default_valueread after the schema is reloaded.