Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.apache.hugegraph.config.ServerOptions;
import org.apache.hugegraph.define.Checkable;
import org.apache.hugegraph.define.UpdateStrategy;
import org.apache.hugegraph.schema.PropertyKey;
import org.apache.hugegraph.metrics.MetricsUtil;
import org.apache.hugegraph.server.RestServer;
import org.apache.hugegraph.structure.HugeElement;
Expand Down Expand Up @@ -108,6 +109,18 @@ protected abstract static class JsonElement implements Checkable {

protected void updateExistElement(JsonElement oldElement, JsonElement newElement,
Map<String, UpdateStrategy> strategies) {
this.updateExistElement(null, oldElement, newElement, strategies);
}

/**
* Combine two JSON elements of the same id within one batch request. With
* a graph the raw JSON values are first normalised to the property key's
* data type (a decimal or a date arrives as a string), so the strategy
* sees typed values on both sides.
*/
protected void updateExistElement(HugeGraph g, JsonElement oldElement,
JsonElement newElement,
Map<String, UpdateStrategy> strategies) {
if (oldElement == null) {
return;
}
Expand All @@ -118,9 +131,15 @@ protected void updateExistElement(JsonElement oldElement, JsonElement newElement
UpdateStrategy updateStrategy = kv.getValue();
if (oldElement.properties.get(key) != null &&
newElement.properties.get(key) != null) {
Object value = updateStrategy.checkAndUpdateProperty(
oldElement.properties.get(key),
newElement.properties.get(key));
Object oldValue = oldElement.properties.get(key);
Object newValue = newElement.properties.get(key);
if (g != null) {
PropertyKey propertyKey = g.propertyKey(key);
oldValue = propertyKey.validValueOrThrow(oldValue);
newValue = propertyKey.validValueOrThrow(newValue);
}
Object value = updateStrategy.checkAndUpdateProperty(oldValue,
newValue);
newElement.properties.put(key, value);
} else if (oldElement.properties.get(key) != null &&
newElement.properties.get(key) == null) {
Expand All @@ -142,10 +161,13 @@ protected void updateExistElement(HugeGraph g, Element oldElement, JsonElement n
UpdateStrategy updateStrategy = kv.getValue();
if (oldElement.property(key).isPresent() &&
newElement.properties.get(key) != null) {
PropertyKey propertyKey = g.propertyKey(key);
// The stored value is typed; normalise the JSON one to match
Object newValue = propertyKey.validValueOrThrow(
newElement.properties.get(key));
Object value = updateStrategy.checkAndUpdateProperty(
oldElement.property(key).value(),
newElement.properties.get(key));
value = g.propertyKey(key).validValueOrThrow(value);
oldElement.property(key).value(), newValue);
value = propertyKey.validValueOrThrow(value);
newElement.properties.put(key, value);
} else if (oldElement.property(key).isPresent() &&
newElement.properties.get(key) == null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ public String update(@Context HugeConfig config,
Id newEdgeId = getEdgeId(graph(manager, graphSpace, graph),
newEdge);
JsonEdge oldEdge = map.get(newEdgeId);
this.updateExistElement(oldEdge, newEdge, req.updateStrategies);
this.updateExistElement(g, oldEdge, newEdge, req.updateStrategies);
map.put(newEdgeId, newEdge);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ public String update(@Context HugeConfig config,
req.jsonVertices.forEach(newVertex -> {
Id newVertexId = getVertexId(g, newVertex);
JsonVertex oldVertex = map.get(newVertexId);
this.updateExistElement(oldVertex, newVertex, req.updateStrategies);
this.updateExistElement(g, oldVertex, newVertex, req.updateStrategies);
map.put(newVertexId, newVertex);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,8 @@ private enum ValueType {
FLOAT(DataType.FLOAT),
DOUBLE(DataType.DOUBLE),
DATE(DataType.DATE),
UUID(DataType.UUID);
UUID(DataType.UUID),
DECIMAL(DataType.DECIMAL);

private final DataType dataType;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -947,6 +947,11 @@ private static boolean numberEquals(Object number1, Object number2) {
// Otherwise convert to BigDecimal to make two numbers comparable
Number n1 = NumericUtil.convertToNumber(number1);
Number n2 = NumericUtil.convertToNumber(number2);
if (n1 instanceof BigDecimal || n2 instanceof BigDecimal) {
// Exact: a decimal must not be squeezed through a double
return new BigDecimal(n1.toString())
.compareTo(new BigDecimal(n2.toString())) == 0;
}
BigDecimal b1 = BigDecimal.valueOf(n1.doubleValue());
BigDecimal b2 = BigDecimal.valueOf(n2.doubleValue());
return b1.compareTo(b2) == 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@

package org.apache.hugegraph.backend.serializer;

import java.math.BigDecimal;
import java.math.BigInteger;
import java.io.OutputStream;
import java.nio.ByteBuffer;
import java.util.Arrays;
Expand Down Expand Up @@ -660,6 +662,13 @@ public void writeProperty(DataType dataType, Object value) {
this.writeLong(uuid.getMostSignificantBits());
this.writeLong(uuid.getLeastSignificantBits());
break;
case DECIMAL:
// unscaled two's-complement bytes + scale: exact for any
// precision, 33 bytes for a 78-digit (uint256) value
BigDecimal decimal = (BigDecimal) value;
this.writeBytes(decimal.unscaledValue().toByteArray());
this.writeVInt(decimal.scale());
break;
default:
// TODO: replace Kryo with Fury (https://github.com/apache/fury)
this.writeBytes(KryoUtil.toKryoWithType(value));
Expand Down Expand Up @@ -693,6 +702,9 @@ public Object readProperty(DataType dataType) {
return Blob.wrap(this.readBigBytes());
case UUID:
return new UUID(this.readLong(), this.readLong());
case DECIMAL:
BigInteger unscaled = new BigInteger(this.readBytes());
return new BigDecimal(unscaled, this.readVInt());
default:
// TODO: replace Kryo with Fury (https://github.com/apache/fury)
return KryoUtil.fromKryoWithType(this.readBytes());
Expand Down Expand Up @@ -872,7 +884,7 @@ public BinaryId parseOlapId(HugeType type, boolean isOlap) {
}
// Parse id from bytes
int start = this.buffer.position();
// OLAP {PropertyKey}{VertexId}
// OLAP {PropertyKey}{VertexId}
if (isOlap) {
// Read olap property id first
Id pkId = this.readId();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -956,4 +961,56 @@ public Blob deserialize(JsonParser jsonParser,
return Blob.wrap(bytes);
}
}

private static class BigDecimalSerializer extends StdSerializer<BigDecimal> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

‼️ BigDecimalSerializer only overrides serialize(), but this module is registered into the typed GraphSON mappers used by gremlin-server (GraphSONMessageSerializerV2d0/V3d0 in gremlin-server.yaml, and V3d0 also answers application/json). Jackson then calls serializeWithType(), which StdSerializer does not implement. I checked this at a28554e by building a ResponseMessage with new BigDecimal("1.5") and serializing it through each serializer configured with ioRegistries: [HugeGraphIoRegistry]. V1d0 returns "1.5". V2d0 and V3d0 both fail with InvalidDefinitionException: Type id handling not implemented for type java.math.BigDecimal (by serializer of type ...HugeGraphSONModule$BigDecimalSerializer). Without the registry the same serializers emit {"@type":"gx:BigDecimal","@value":1.5}. So g.V().values('balance') on a DECIMAL key fails over GraphSON v2/v3, and so does any existing script that returns a BigDecimal, such as a Groovy decimal literal (g.inject(1.5)). That used to work. Requested change: implement serializeWithType (for example via typeSer.writeTypePrefix/writeTypeSuffix, as the other typed serializers in this module do), or limit the string serializer to JsonUtil and leave the TinkerPop gx:BigDecimal handling alone. Add a test that serializes a BigDecimal through GraphSON v2 and v3 with HugeGraphIoRegistry.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 9d5eaab, and thanks for checking this through the real serializers, I had only tested JsonUtil. BigDecimalSerializer now has serializeWithType() in the same shape as IdSerializer in this module: typeSer.typeId(value, VALUE_STRING), prefix, serialize(), suffix. While at it I removed the BigDecimal entry from the module's TYPE_DEFINITIONS: with it the type id came out as hugegraph:BigDecimal, which no client knows; without it the id stays gx:BigDecimal from GraphSONXModule, and our serializer still wins the lookup because the registry is added later.

Result: V1 gives "1.5", V2 and V3 give {"@type":"gx:BigDecimal","@value":"1.5"}. The string in @value is deliberate: a number there is decoded as a double by the JS/Python clients, and this type exists to avoid exactly that; Jackson's default BigDecimal deserializer and gx:BigDecimal in gremlin-python both accept a string. If you would rather keep a number in @value for compatibility with the previous gx:BigDecimal output, it is a one-line change, but then it should be said explicitly in the type's description.

Test: new unit/serializer/HugeGraphSONModuleTest (in UnitTestSuite) builds a ResponseMessage with a BigDecimal, runs it through GraphSONMessageSerializerV1d0/V2d0/V3d0 configured with ioRegistries: [HugeGraphIoRegistry], checks the type prefix and the string in @value, and deserializes the response back to an equal BigDecimal for 1.5, 1E-18 and uint256 max. 3/3 on JDK 11.

End to end on the lab, dists from both heads, hstore and rocksdb (cluster/decimal_e2e.py, group R6 in results/decimal/e2e/ of https://github.com/SebastianGruza/hugegraph-validation): before, every one of the 8 queries through gremlin-server with Accept v2.0 and v3.0 (values() on uint256 max, g.inject(1.5), values() of a default value, sum()) → 500 Type id handling not implemented; after, all 8 return gx:BigDecimal with the exact value, sum() exact to the 18th fraction digit. /gremlin through the REST proxy (application/json, untyped) worked on both heads.


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
Expand Up @@ -368,6 +368,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);
return decimal;
}

if (this.checkDataType(value)) {
Expand Down Expand Up @@ -400,6 +404,8 @@ public interface Builder extends SchemaBuilder<PropertyKey> {

Builder asLong();

Builder asDecimal();

Builder valueSingle();

Builder valueList();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -602,6 +602,10 @@ private void checkSortKeys() {
"The sort key '%s' must be contained in " +
"properties '%s' for edge label '%s'",
key, this.name, this.properties);
PropertyKey propertyKey = this.graph().propertyKey(key);
E.checkArgument(!propertyKey.dataType().isDecimal(),
"The sort key '%s' of edge label '%s' can't " +
"be a decimal property", key, this.name);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ The decimal guard is in checkFields(), which only runs on the user-facing create() path. OLAP property keys build their index through SchemaTransaction.createIndexLabelForOlapPk(), which calls IndexLabelBuilder.build() directly and skips checkFields(). PropertyKeyBuilder.checkOlap() also rejects only OLAP_RANGE for non-numeric types. On RocksDB at a28554e, schema.propertyKey("rank").asDecimal().writeType(WriteType.OLAP_SECONDARY).create() succeeds and creates index label *olap_by_rank type=SECONDARY. That contradicts the rule this PR states (no index of any type on a decimal). The secondary index key is also built from value.toString() (SplicingIdGenerator.concatValues), and for BigDecimal that output depends on scale and can use exponent notation (1E+21), so equal numbers can map to different index keys. Requested change: reject OLAP_SECONDARY (and any OLAP write type that builds an index) for DataType.DECIMAL in PropertyKeyBuilder.checkOlap(), or move the decimal check into build(), and add a core test for it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 9d5eaab, both things you asked for: PropertyKeyBuilder.checkOlap() rejects for DECIMAL every OLAP write type that builds an index (everything but OLAP_COMMON) with "decimal keys can't be indexed", and IndexLabelBuilder.build() carries the same guard as checkFields(), so the rule also holds on the createIndexLabelForOlapPk() path and for any future caller of build(). The toString()/scale point about the secondary index key becomes moot, since such an index can no longer exist.

Test: PropertyKeyCoreTest.testAddOlapPropertyKeyWithDecimalType behind Assume supportsOlapProperties: OLAP_SECONDARY and OLAP_RANGE on a decimal key → NotAllowException, *olap_by_rank does not exist, OLAP_COMMON (no index) still passes. On rocksdb: PropertyKeyCoreTest 25/25, IndexLabelCoreTest 45/45. End to end through REST on the lab (group R3 in results/decimal/e2e/): before, write_type: OLAP_SECONDARY on a decimal key → 202 and the key is created; after → 400 "decimal keys can't be indexed", OLAP_RANGE 400, OLAP_COMMON 202, on hstore and rocksdb.

"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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,12 @@ public PropertyKeyBuilder asDouble() {
return this;
}

@Override
public PropertyKeyBuilder asDecimal() {
this.dataType = DataType.DECIMAL;
return this;
}

@Override
public PropertyKeyBuilder asFloat() {
this.dataType = DataType.FLOAT;
Expand Down Expand Up @@ -427,7 +433,8 @@ private void checkAggregateType() {
}

if (this.aggregateType.isNumber() &&
!this.dataType.isNumber() && !this.dataType.isDate()) {
!this.dataType.isNumber() && !this.dataType.isDecimal() &&
!this.dataType.isDate()) {
throw new NotAllowException(
"Not allowed to set aggregate type '%s' for " +
"property key '%s' with data type '%s'",
Expand All @@ -452,6 +459,16 @@ private void checkOlap() {
"property key '%s'", this.aggregateType, this.name);
}

if (this.dataType.isDecimal() &&
this.writeType != WriteType.OLAP_COMMON) {
// OLAP_SECONDARY / OLAP_RANGE build an index label on the key,
// and no index of any type is allowed on a decimal
throw new NotAllowException(
"Not allowed to set write type to %s for property key " +
"'%s' with data type '%s': decimal keys can't be indexed",
this.writeType, this.name, this.dataType);
}

if (this.writeType == WriteType.OLAP_RANGE &&
!this.dataType.isNumber() && !this.dataType.isDate()) {
throw new NotAllowException(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@

package org.apache.hugegraph.type.define;

import java.math.BigDecimal;
import java.math.BigInteger;
import java.nio.ByteBuffer;
import java.util.Date;
import java.util.List;
Expand All @@ -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);

private final byte code;
private final String name;
Expand Down Expand Up @@ -103,6 +112,10 @@ public boolean isUUID() {
return this == DataType.UUID;
}

public boolean isDecimal() {
return this == DataType.DECIMAL;
}

public <V> Number valueToNumber(V value) {
if (!(this.isNumber() && value instanceof Number) &&
!JsonUtil.isInfinityOrNaN(value)) {
Expand Down Expand Up @@ -143,6 +156,42 @@ public <V> Number valueToNumber(V value) {
return number;
}

/**
* Convert a value to BigDecimal: BigDecimal as is, any other Number and a
* decimal string through their exact decimal representation. Float and
* Double go through Number.toString(), i.e. the shortest string that
* round-trips the binary value, so a client that already holds a lossy
* double gets that double, exactly.
*
* @return the BigDecimal, or null if the value is not a Number or String
* @throws IllegalArgumentException if the string is not a decimal number
*/
public <V> BigDecimal valueToDecimal(V value) {
if (!this.isDecimal()) {
return null;
}
if (value instanceof BigDecimal) {
return (BigDecimal) value;
}
if (value instanceof BigInteger) {
return new BigDecimal((BigInteger) value);
}
if (value instanceof Byte || value instanceof Short ||
value instanceof Integer || value instanceof Long) {
return BigDecimal.valueOf(((Number) value).longValue());
}
if (!(value instanceof Number) && !(value instanceof String)) {
return null;
}
String text = value.toString().trim();
try {
return new BigDecimal(text);
} catch (NumberFormatException e) {
throw new IllegalArgumentException(String.format(
"Can't read '%s' as decimal", value));
}
}

public <V> Date valueToDate(V value) {
if (!this.isDate()) {
return null;
Expand Down
Loading
Loading