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()); | ||
| 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()); | ||
| } | ||
|
|
||
| @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.
🧹 Minor. This goes through
registerCommonSerializers(), whichHugeGraphIoRegistryregisters for every GraphSON version andJsonUtilalso uses, so it applies to everyjava.math.BigDecimal, not only DECIMAL property values. Groovy decimal literals are alreadyBigDecimal:g.inject(1.5)or2 * 1.1through/gremlinreturned1.5before this PR and returns"1.5"now (the newHugeGraphSONModuleTestasserts the V1 string). The PR description says existing endpoints don't change.Requested change: add the V1 number-to-string change, and the string in
gx:BigDecimal@value, to the compatibility note and release notes. Or keep numeric output for BigDecimal values that don't come from a DECIMAL property.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.
Right, the PR description was inaccurate there. Added to "Note on compatibility" and for the release notes: through
registerCommonSerializers()the serializer applies to everyjava.math.BigDecimalin a response, so a Groovy literal (g.inject(1.5),2 * 1.1) used to come back as the number1.5and now comes back as"1.5"in V1 and through the REST/gremlinproxy, and as{"@type":"gx:BigDecimal","@value":"1.5"}instead of a number in@valueon V2/V3. The "numbers for BigDecimals that are not DECIMAL property values" variant cannot be done in the serializer, since aBigDecimalcarries no record of where it came from; it would need a wrapper type on property values, which I think is worse than one plain rule, "a BigDecimal is always a string". If the maintainers prefer backward compatibility, the change is one line (writeNumberinstead ofwriteStringin@value), but then the JS/Python clients get a double.