feat(core): add DECIMAL (BigDecimal) property data type - #3209
SebastianGruza wants to merge 8 commits into
Conversation
A new DataType.DECIMAL(12) stores arbitrary-precision decimals exactly: unscaled two's-complement bytes plus scale in BytesBuffer (server and struct copies), a string on the JSON wire (both a string and a number literal are accepted on input), exact equality in ConditionQuery, and the store-side row decoder maps it to a string variant. It is deliberately not a "number" in the DataType.isNumber() sense: there is no fixed-width sortable encoding, so a decimal property key can't be a sort key, an index field of any type, or an OLAP range property; the schema builders reject those explicitly. SUM/MAX/MIN aggregate types and the batch-update SUM/BIGGER/SMALLER strategies, which already compute in BigDecimal, keep the full precision (e.g. uint256 token balances). Tests: DataTypeTest, BytesBufferTest, JsonUtilTest, PropertyKeyCoreTest, IndexLabelCoreTest, EdgeLabelCoreTest, VertexCoreTest, VertexApiTest (batch update with SUM/BIGGER on 2^256-1), struct PropertyKeyTest. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3209 +/- ##
============================================
+ Coverage 37.86% 41.85% +3.99%
- Complexity 6586 7408 +822
============================================
Files 800 797 -3
Lines 68985 69360 +375
Branches 9172 9302 +130
============================================
+ Hits 26120 29034 +2914
+ Misses 39796 37003 -2793
- Partials 3069 3323 +254 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking issues remain in the struct-side DECIMAL schema integration; targeted exact-head tests passed, but this review is not an approval.
| case UUID: | ||
| builder.append(".asUUID()"); | ||
| break; | ||
| case DECIMAL: |
There was a problem hiding this comment.
There was a problem hiding this comment.
Done in 9d5eaab: valueToDecimal() ported into the struct DataType unchanged from the server copy, a decimal branch in struct PropertyKey.convSingleValue(), and asDecimal() on the struct PropertyKey.Builder. Tests in struct PropertyKeyTest: conversion from a string, Long/Integer, BigInteger, exponent notation and uint256 max, "1,5" and a Date rejected; a default value from userdata as a string, as an integral literal and as a list under LIST cardinality, all coming back as BigDecimal. struct 5/5 on JDK 11.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The new BigDecimal serializer breaks typed GraphSON (v2/v3) for every BigDecimal Gremlin result, and an OLAP_SECONDARY decimal key still gets a secondary index despite the new no-index rule. The struct-side conversion gap is still open. Evidence: GraphSONMessageSerializerV2d0/V3d0 configured with HugeGraphIoRegistry at a28554e fail with "Type id handling not implemented for type java.math.BigDecimal" (same serializers without the registry emit gx:BigDecimal); a RocksDB probe created propertyKey("rank").asDecimal().writeType(OLAP_SECONDARY) and index label *olap_by_rank type=SECONDARY. Latest-head CI is green.
| } | ||
| } | ||
|
|
||
| private static class BigDecimalSerializer extends StdSerializer<BigDecimal> { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| E.checkArgument(pkey.aggregateType().isIndexable(), | ||
| "The aggregate type %s is not indexable", | ||
| pkey.aggregateType()); | ||
| E.checkArgument(!pkey.dataType().isDecimal(), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| case UUID: | ||
| builder.append(".asUUID()"); | ||
| break; | ||
| case DECIMAL: |
There was a problem hiding this comment.
DataType.DECIMAL, but struct PropertyKey.convSingleValue() has no decimal branch (only number/date/uuid/blob), and struct DataType has no valueToDecimal(). For a DECIMAL key, a String or Long value falls through to checkDataType() and returns null. One concrete case is defaultValue(): userdata is reloaded from JSON, so a decimal default arrives as a string, and validValueOrThrow(raw) then throws. The server-side PropertyKey in this PR converts these values correctly. Requested change: port valueToDecimal() into the struct DataType and add the decimal branch to struct convSingleValue(), with tests for string, integral and default-value input.
There was a problem hiding this comment.
Done in 9d5eaab, same change as in the thread above: valueToDecimal() in the struct DataType, the decimal branch in convSingleValue(), asDecimal() on the builder, tests for string, integral, BigInteger and default-value input (single and list).
…hSON, OLAP guard - hugegraph-struct: port DataType.valueToDecimal(), add the decimal branch to PropertyKey.convSingleValue() and Builder.asDecimal(); tests for string, integral, BigInteger and default-value input - HugeGraphSONModule: implement BigDecimalSerializer.serializeWithType() for the typed GraphSON v2/v3 mappers; keep TinkerPop's gx:BigDecimal type id, carry the plain string in @value; HugeGraphSONModuleTest round-trips through GraphSONMessageSerializerV1d0/V2d0/V3d0 with HugeGraphIoRegistry - PropertyKeyBuilder.checkOlap(): reject OLAP_SECONDARY/OLAP_RANGE for DECIMAL; IndexLabelBuilder.build(): guard on every path, not only create(); core test for the OLAP path
|
Thanks for the reviews. All three blocking points are handled in 9d5eaab: the DECIMAL conversion in the struct copy (method, branch in On top of that, end to end on live servers (hstore on PD + 3 stores, rocksdb), dists built from One thing for the release notes rather than for this PR: a graph that already contains a DECIMAL property key cannot be opened by a server without this change ( |
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: a DECIMAL value with a huge exponent, such as "1E+999999999", is accepted and stored in a few bytes, and every read then expands it through toPlainString() into a billion-character string, which exhausts server memory. The BigDecimal serializer is also registered globally, so existing BigDecimal Gremlin results change from JSON numbers to strings. Evidence: static read of DataType.valueToDecimal, HugeGraphSONModule.BigDecimalSerializer and GraphStoreIterator at 9d5eaab; a JDK 17 probe where toPlainString() on that value throws OutOfMemoryError at -Xmx512m and returns 1,000,000,000 chars at -Xmx4g. Latest-head CI is green.
| } | ||
| String text = value.toString().trim(); | ||
| try { | ||
| return new BigDecimal(text); |
There was a problem hiding this comment.
"1E+999999999" parses to unscaled 1 with scale -999999999, and BytesBuffer stores it in a few bytes. Every output path then calls toPlainString(): BigDecimalSerializer.serialize() in HugeGraphSONModule.java:974 (REST responses, including the create response, and GraphSON v1/v2/v3) and GraphStoreIterator.java:261 on the store side.
On JDK 17, toPlainString() on that value throws OutOfMemoryError at -Xmx512m (the minimum heap in hugegraph-server.sh). At -Xmx4g it returns a 1,000,000,000 character string in about 1.3 s, before Jackson copies it into the response. So any user who can write a vertex can store an 11-character value that costs gigabytes on every read. "1E-999999999" does the same.
Requested change: reject values whose scale() or precision() is above a documented limit (one that still fits uint256 with 18 fraction digits) in valueToDecimal(), here and in the struct copy, and add a test with "1E+999999999". The validValueOrThrow(value) that BatchAPI already runs after the strategy will then also cover a SUM result that crosses the limit.
There was a problem hiding this comment.
Done in bae56ca, good catch. The bound: at most 128 significant digits and an absolute scale of at most 128 (DataType.DECIMAL_MAX_PRECISION / DECIMAL_MAX_SCALE, the same pair in the struct copy), checked by checkDecimalBounds() at the end of valueToDecimal(). uint256 with 18 fraction digits is 96 digits, so more than 30 digits of headroom remain, and the longest possible toPlainString() is about 256 characters. The message reads Decimal value out of bounds: precision 1, scale -999999999 (at most 128 significant digits and a scale of at most 128 in either direction).
One thing your comment touched on that I had not seen: PropertyKey.convValue() returned the value untouched when its type already matched, so a ready-made BigDecimal (a Gremlin literal in addV().property(), the SUM result in BatchAPI) never reached valueToDecimal() at all. The struct test caught it: validValueOrThrow(new BigDecimal("1E+999999999")) did not throw. Both copies of convValue() now skip the short-circuit for decimals, so validValueOrThrow after the strategy really does cover the SUM result, as you wrote.
Tests: DataTypeTest.testValueToDecimalBounds (1E+999999999, 1E-999999999, 1E+129 and 129 digits rejected; 128 digits, 1E+128, 1E-128 and uint256 with 18 fraction digits accepted; the same bound for a ready-made BigDecimal), struct PropertyKeyTest (the same inputs through validValueOrThrow), PropertyKeyCoreTest (a string and a BigDecimal through validValue). End to end on the lab (rocksdb, cluster/decimal_e2e.py, results/decimal/e2e/after2-rocksdb.log in hugegraph-validation): create with 1E+999999999 → 400 "out of bounds", create with 1E+128 and with 128 nines → 201, batch SUM of 128 nines + 1 (a 129-digit result) → 400 "out of bounds"; 37 PASS, 0 FAIL, 4 N-A.
| module.addDeserializer(Blob.class, new BlobDeserializer()); | ||
|
|
||
| // Decimals travel as strings: JSON numbers are doubles to most clients | ||
| module.addSerializer(BigDecimal.class, new BigDecimalSerializer()); |
There was a problem hiding this comment.
🧹 Minor. This goes through registerCommonSerializers(), which HugeGraphIoRegistry registers for every GraphSON version and JsonUtil also uses, so it applies to every java.math.BigDecimal, not only DECIMAL property values. Groovy decimal literals are already BigDecimal: g.inject(1.5) or 2 * 1.1 through /gremlin returned 1.5 before this PR and returns "1.5" now (the new HugeGraphSONModuleTest asserts 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.
Right, the PR description was inaccurate there. Added to "Note on compatibility" and for the release notes: through registerCommonSerializers() the serializer applies to every java.math.BigDecimal in a response, so a Groovy literal (g.inject(1.5), 2 * 1.1) used to come back as the number 1.5 and now comes back as "1.5" in V1 and through the REST /gremlin proxy, and as {"@type":"gx:BigDecimal","@value":"1.5"} instead of a number in @value on V2/V3. The "numbers for BigDecimals that are not DECIMAL property values" variant cannot be done in the serializer, since a BigDecimal carries 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 (writeNumber instead of writeString in @value), but then the JS/Python clients get a double.
…cimals too - DataType.valueToDecimal() (server and struct): at most 128 significant digits and an absolute scale of 128 (DECIMAL_MAX_PRECISION / DECIMAL_MAX_SCALE); "1E+999999999" is rejected before it is stored instead of costing a billion characters from toPlainString() on every read - PropertyKey.convValue() (server and struct): no short-circuit for a BigDecimal that already has the right type, so a Gremlin literal and the SUM result of a batch update pass the same bounds check - tests: DataTypeTest.testValueToDecimalBounds, struct PropertyKeyTest, PropertyKeyCoreTest
|
Round 2 in bae56ca: DECIMAL values are bounded to 128 significant digits and an absolute scale of 128, checked in |
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: DECIMAL primary-key IDs can collide or fail for valid high-precision values. Evidence: current-head static tracing shows VertexLabelBuilder accepts DECIMAL primary keys, while ConditionQuery/LongEncoding still route the normalized BigDecimal through NumericUtil; a local probe collapsed distinct 18-decimal values to one sortable long and raised ArithmeticException for a uint256 value.
| * encoding, so it can't be a sort key, a range/secondary index field or an | ||
| * OLAP range property. | ||
| */ | ||
| DECIMAL(12, "decimal", BigDecimal.class); |
There was a problem hiding this comment.
DECIMAL is still accepted as a vertex primary key: VertexLabelBuilder.checkPrimaryKeys() only checks that the key belongs to properties, and this PR adds no type guard. Core primary-id generation then passes the normalized BigDecimal through ConditionQuery.concatValues()/LongEncoding; NumericUtil.numberToSortableLong() converts non-integral values through double and throws on large integral values. A local probe showed 1.000000000000000001 and 1.000000000000000002 map to the same sortable long, while the uint256 value advertised by this PR throws ArithmeticException: Overflow. The REST batch path also hashes the raw string before core normalization. Please reject DECIMAL primary keys or add a lossless canonical primary-key encoding, with tests for fractional precision and uint256 values.
There was a problem hiding this comment.
Done in 834ef89, thanks for tracing that path. I reject a decimal as a primary key rather than adding a canonical encoding: it is consistent with this PR's rule (no sort key, no index, no OLAP write type with an index), and a lossless id would also have to settle scale normalisation (are 1.0 and 1 the same key or different ones?) and touch SplicingIdGenerator, which I would rather not do in this PR. The guard sits in VertexLabelBuilder.checkPrimaryKeys(), next to the existing check that the key belongs to the properties, in the same shape as the decimal sort-key rejection in EdgeLabelBuilder: The primary key 'balance' of vertex label 'account' can't be a decimal property. It covers a composite key that has a decimal among its fields too. The REST batch path you mention (hashing the raw string before core normalisation) no longer matters, since such a label cannot exist.
Test: VertexLabelCoreTest.testAddVertexLabelWithDecimalPrimaryKey: a single decimal primary key and a composite (name, balance) key are rejected with that message, the label does not exist afterwards, and a decimal as a plain property next to a text primary key passes. VertexLabelCoreTest 53/53 on rocksdb, 123/123 together with PropertyKeyCoreTest and EdgeLabelCoreTest. End to end: a new R2 check in cluster/decimal_e2e.py (POST /schema/vertexlabels with primary_keys: [balance] → 400 The primary key 'd2g_balance' of vertex label 'd2g_badpk' can't be a decimal property); on rocksdb with a dist from this head 38 PASS, 0 FAIL, 4 N-A, log results/decimal/e2e/after3-rocksdb.log in https://github.com/SebastianGruza/hugegraph-validation. The limits table in the PR description and in the docs now lists the primary key as well.
A primary key becomes part of the vertex id through LongEncoding and NumericUtil.numberToSortableLong(), which goes through a double for fractions (1.000000000000000001 and ...002 collapse into one id) and overflows a long on uint256. VertexLabelBuilder.checkPrimaryKeys() now rejects a decimal property as a primary key, single or composite, next to the existing sort-key and index guards; core test added.
|
Round 3 in 834ef89: DECIMAL is rejected as a vertex primary key (single and composite) in |
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: DECIMAL is stored exactly on every backend path, bounded, excluded from primary keys, sort keys and indexes, and the earlier review points (struct conversion, typed GraphSON, OLAP index bypass, exponent bound, primary-key ids) are all fixed. Please add the string output for every BigDecimal in Gremlin/REST responses to the release notes. Evidence: read core/struct BytesBuffer, PropertyKey, the schema builders, ConditionQuery/Condition and BatchAPI at 834ef89; checked the negative-scale vint round trip; ran a local memory-backend core probe (eq/neq/within/without against a stored 1.50 with 1.5, all scale-insensitive) plus testAddVertexWithPropertyValueOfDecimal; all latest-head CI checks pass.
|
Thanks for the review and for checking the scale-insensitive comparisons, that was not covered by my tests. Added a "Release notes" section to the description with the three lines that matter for a release: the new type and its limits, every |
|
Companion PR on the toolchain side: apache/hugegraph-toolchain#771 ( |
…s stay exact Jersey parsed request bodies with a default ObjectMapper, so a JSON fraction such as 12345678901234567890.10 reached a DECIMAL property key as a double (17 digits) and decimal fractions had to be sent as strings. An ObjectMapperResolver now enables USE_BIG_DECIMAL_FOR_FLOATS for REST bodies: DECIMAL keys receive every digit, numeric keys are narrowed by DataType.valueToNumber as before (any Number is accepted), and integer keys still reject a fraction. JsonUtil.castNumber converts a BigDecimal to double instead of asserting the JSON type. Tests: VertexApiTest covers a 39-digit literal on a DECIMAL and a DOUBLE key, an exponent literal, an INT key rejecting a fraction, and the batch SUM increment sent as a number literal.
…unds, keep loader JSON decimals exact
Review round 1 of the DECIMAL companion:
- A BigDecimal is written as a plain JSON number (every digit, never
E-notation) instead of a string. Numeric keys keep accepting it (the
server narrows any Number), so `vertex.property("weight",
new BigDecimal("1.5"))` on a DOUBLE key works against every server, and
a server that reads fractions as BigDecimal (apache/hugegraph#3209)
stores a DECIMAL value exactly. Covered by
`VertexApiTest.testCreateWithBigDecimalOnDoubleKey` (runs on the CI
server) and `DecimalDataTypeTest`.
- Both client `valueToDecimal` copies apply the server's bounds (128
significant digits, scale 128 either way), so the HBase direct path
cannot store a value such as 1E+999999999; unit-tested on both copies
and on `BytesBuffer.writeProperty`.
- The loader's JSON line parser reads fractions as BigDecimal
(`USE_BIG_DECIMAL_FOR_FLOATS`), so a decimal column keeps digits a
double would drop; collection elements of another type (a JSON integer
in a decimal list) are converted one by one. `DataTypeUtilTest` covers
a 39-digit JSON value, a mixed list and DOUBLE narrowing.
|
New commit 0146849, prompted by the review of the toolchain companion apache/hugegraph-toolchain#771: REST now reads JSON fractions as |
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The DECIMAL type itself (encoding, bounds, no-index and no-primary-key rules, struct conversion, typed GraphSON) looks correct and the earlier review points are resolved, but the new global ObjectMapperResolver in 0146849 turns every JSON fraction in any REST body into a BigDecimal, which JsonUtil then re-encodes as a string: fractional algorithm/computer job parameters fail at execution and a fractional ~default_value on a DOUBLE key breaks after a schema reload. Evidence: static trace at 0146849 of ObjectMapperResolver, HugeGraphSONModule.registerCommonSerializers(), JsonUtil, AlgorithmAPI/ComputerAPI, AlgorithmJob.execute(), ParameterUtil.parameterDouble(), BinarySerializer/TextSerializer userdata, PropertyKey.defaultValue() and DataType.valueToNumber(); a standalone Jackson 2.9 probe showing {"alpha":0.85} round-trips to the String "0.85"; all 22 latest-head checks pass.
|
|
||
| public ObjectMapperResolver() { | ||
| this.mapper = new ObjectMapper(); | ||
| this.mapper.enable(DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS); |
There was a problem hiding this comment.
USE_BIG_DECIMAL_FOR_FLOATS on the shared Jersey mapper changes every untyped Object in every REST body, not only vertex and edge properties. Combined with the BigDecimalSerializer this PR adds to registerCommonSerializers() (which JsonUtil uses), any fraction that the server re-encodes with JsonUtil.toJson() now comes back as a JSON string, and existing code that expects a Number rejects it.
Two concrete paths:
- Algorithm and computer jobs.
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. - Schema userdata.
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_FLOATS reads {"alpha":0.85} as BigDecimal; a mapper with the same BigDecimal to writeString(toPlainString()) serializer writes the task input as {"parameters":{"alpha":"0.85",...}}; reading it back gives java.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 converting BigDecimal back to Double for DOUBLE/FLOAT keys only where the key is known), and drop the global ObjectMapperResolver. 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_value read after the schema is reloaded.
There was a problem hiding this comment.
Right on both counts, thanks: the job's alpha and a DOUBLE ~default_value would have gone through JsonUtil.toJson as strings. Done the way you propose as the first option: ObjectMapperResolver is gone, Jersey uses its default mapper. Exact fractions are read only by the properties field of BatchAPI.JsonElement (and through it by JsonVertex/JsonEdge in the single and batch APIs) via PropertiesDeserializer: a manual token walk, VALUE_NUMBER_FLOAT -> getDecimalValue(), every other type as before, key order kept. Everything outside properties gets Jackson's default types. Tests: PropertiesDeserializerTest (exact fractions only inside properties, options.alpha stays a Double, null, an array is rejected), JsonUtilTest ({"alpha":0.85} round-trips as a number), VertexApiTest (a DOUBLE key with "~default_value": 1.5 comes back from GET as a number). A schema reload through a restart does not fit the API suite; the mechanism that broke it is gone.
| * @return the BigDecimal, or null if the value is not a Number or String | ||
| * @throws IllegalArgumentException if the string is not a decimal number | ||
| */ | ||
| /* |
There was a problem hiding this comment.
🧹 The valueToDecimal() Javadoc (lines 159-168) now sits above the bounds comment and the DECIMAL_MAX_PRECISION / DECIMAL_MAX_SCALE constants, so the Javadoc tool attaches it to DECIMAL_MAX_PRECISION, and valueToDecimal() at line 179 has no documentation. The struct copy of DataType has the constants before the Javadoc and does not have this problem.
Requested change: move the two constants and their /* Bounds ... */ comment above the /** Convert a value to BigDecimal ... */ block, so the Javadoc directly precedes valueToDecimal().
There was a problem hiding this comment.
Done: the constants with their bounds comment sit above the Javadoc, and the Javadoc directly above valueToDecimal(), as in the struct copy.
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: HStore DECIMAL filters lose precision across the Store query wire format, and unbounded GraphSON BigDecimals can expand into enormous strings; the DECIMAL scale bound also has an Integer.MIN_VALUE bypass. Evidence: static trace through ConditionQuery/QueryAdapter/FilterIterator, HugeGraphSONModule, and both DataType copies; 22 exact-head checks pass.
| // 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) { |
There was a problem hiding this comment.
There was a problem hiding this comment.
Correct, and this was the most serious gap in the PR. QueryAdapter (the core copy and the struct copy) now treats BigDecimal and BigInteger like the primitive wrappers and Date: the value gets a valuecls, and on the store side context.deserialize(value, BigDecimal.class) reads it from the literal's text, never through a double; lists (IN) take the same path from their first element. ConditionQuery.numberEquals in the struct copy got the same exact branch as core, and ranges were already exact (NumericUtil.compareNumber compares through new BigDecimal(toString())). Tests: StoreSerializerTest.testConditionQueryBytesKeepBigDecimal (eq, gte, in through bytes()/fromBytes(), a double unchanged) and VertexApiTest: a vertex filter without an index is refused (NoIndexException), so the test takes the path that really reaches the store, an edge query by vertex + label + properties={"amount": ...}. The edge with a 39-digit amount is returned for the exact value and not for the same value with the last digit changed; on hstore that goes through the pushdown and the Store's FilterIterator, 6/6 with the server and store jars from this commit (lab: 1 PD + 3 Stores).
| @Override | ||
| public void serialize(BigDecimal decimal, JsonGenerator jsonGenerator, | ||
| SerializerProvider provider) throws IOException { | ||
| jsonGenerator.writeString(decimal.toPlainString()); |
There was a problem hiding this comment.
There was a problem hiding this comment.
Done with the second of your options: the serializer writes toPlainString() only while the scale is within the DECIMAL bound (±128), and toString() beyond it, i.e. the scientific form: just as exact and parseable, but without expanding the exponent into characters. A property value is always within the bound (PropertyKey checks it), so nothing changes for graph data; a Gremlin result such as 1E+999999999 goes out as "1E+999999999". The deserializer stays unbounded, since the output side now bounds it. JsonUtilTest: 1E+999999999, 1E-129, 1E+128 (the boundary, plain).
| } | ||
|
|
||
| public static BigDecimal checkDecimalBounds(BigDecimal decimal) { | ||
| int scale = Math.abs(decimal.scale()); |
There was a problem hiding this comment.
There was a problem hiding this comment.
Done in both copies: scale < -DECIMAL_MAX_SCALE || scale > DECIMAL_MAX_SCALE instead of Math.abs. Tests with new BigDecimal(BigInteger.ONE, Integer.MIN_VALUE), MIN_VALUE + 1 and MAX_VALUE in DataTypeTest and in the struct PropertyKeyTest.
…es, store filters, GraphSON output Review round 3 of apache#3209: - The global Jersey ObjectMapper switch (0146849) is gone. Only the "properties" object of a vertex/edge body is read with exact fractions, through PropertiesDeserializer on BatchAPI.JsonElement; job parameters, schema userdata and every other body keep Jackson's default number types, so a fractional algorithm parameter or a DOUBLE ~default_value round-trips as before. - A DECIMAL condition now travels to the store typed: QueryAdapter (core and struct copies) tags BigDecimal and BigInteger values with their class, as it does for primitive wrappers and Date, so the Store's ConditionQuery.fromBytes reads the exact value instead of a double; the struct ConditionQuery compares decimals exactly like the core copy. - The GraphSON BigDecimal serializer uses the plain form only while the scale is within the DECIMAL bound and the scientific form beyond it, so an unbounded Gremlin result cannot expand an exponent into a billion characters. - checkDecimalBounds compares the scale directly with the bound in both copies (Math.abs(Integer.MIN_VALUE) overflowed); the Javadoc of valueToDecimal sits above the method again. Tests: PropertiesDeserializerTest (exact fractions in properties only, null and non-object), StoreSerializerTest (BigDecimal eq/gte/in conditions through bytes()/fromBytes()), DataTypeTest and the struct PropertyKeyTest (extreme scales), JsonUtilTest (exponent form beyond the bound, doubles stay numbers), VertexApiTest (exact filter by a 39-digit value and a near miss, DOUBLE key with a fractional default).
|
Round 3 in 47aa818: the global Jersey mapper is gone, exact fractions only in the |
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: DECIMAL default and REST filter inputs still lose fractional precision, and HStore IN comparisons mishandle values with different scales. Evidence: static trace through request parsing, property query parsing, and Condition.RelationType.IN.
| * 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 |
There was a problem hiding this comment.
~default_value such as 0.1234567890123456789 therefore reaches PropertyKeyAPI as a Double, and valueToDecimal() can only reconstruct its already-rounded text. Please preserve fractional defaults as BigDecimal or reject numeric fractions and require strings, with an exact default-value case.
There was a problem hiding this 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 without 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.
| return parser.getNumberValue(); | ||
| case VALUE_NUMBER_FLOAT: | ||
| // Exact: the literal's digits, not the nearest double | ||
| return parser.getDecimalValue(); |
There was a problem hiding this comment.
properties through API.parseProperties(..., Map.class), where untyped fractional numbers become Double; 0.100000000000000001 is rounded to 0.1 before DECIMAL comparison. This parser only handles request-body properties. Please preserve filter precision or normalize each value against its schema type before building the traversal.
There was a problem hiding this 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.
| .build(); | ||
|
|
||
| static boolean isPrimitive(Class clz) { | ||
| // Values whose class must travel with them: Gson reads an untyped |
There was a problem hiding this comment.
IN/NOT_IN lists, but Condition.RelationType.IN/NOT_IN uses Collection.contains, which calls scale-sensitive BigDecimal.equals. A stored 1.0 therefore misses P.within(new BigDecimal("1.00")), although scalar DECIMAL equality treats them as equal. Compare decimal collection members numerically.
There was a problem hiding this 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.
| * encoding, so it can't be a sort key, a range/secondary index field or an | ||
| * OLAP range property. | ||
| */ | ||
| DECIMAL(12, "decimal", BigDecimal.class); |
There was a problem hiding this comment.
🧹 The PR body marks the hugegraph-doc data-type page as TODO and says it will be a separate PR; this diff has no docs files or paired link. Root AGENTS.md requires matching user-facing docs and a linked paired docs PR when website docs live separately. Please add the docs update or link the paired hugegraph-doc change and coordinate it with this feature.
There was a problem hiding this 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.
…s, IN/NOT_IN by value - PropertyKeyAPI reads user_data through UserdataDeserializer (the same token walk as PropertiesDeserializer), so a DECIMAL ~default_value given as a JSON number keeps every digit; BinarySerializer and TextSerializer reload userdata through JsonUtil.fromJsonExact, so it survives a restart. - API.parseProperties reads the properties filter exactly and the vertex and edge list APIs normalise each plain value to its property key's type (API.normalizeProperties) before building the traversal. - Condition.RelationType.IN/NOT_IN (core and struct copies) match numbers by value once a BigDecimal is involved, like EQ; other numbers keep the contains() semantics. - Tests: PropertiesDeserializerTest (userdata), JsonUtilTest (fromJsonExact), ConditionTest in core and struct, VertexApiTest (numeric filter literal, exact default value on create and after reload). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Round 4 in 0bb7724: a property key's userdata is read exactly in the API and when the schema is read back from the backend (a DECIMAL |
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: DECIMAL query handling loses precision or rejects multi-valued filters, and two newly supported paths change fractional metadata to strings or fail scans. Evidence: exact-head static call paths in API.java and TraversalUtil.java, UserdataDeserializer/PropertiesDeserializer/JsonUtil, and GraphStoreIterator.java.
| for (Map.Entry<String, Object> entry : props.entrySet()) { | ||
| Object value = entry.getValue(); | ||
| if (value == null || value instanceof Collection || value instanceof Map || | ||
| value instanceof P) { |
There was a problem hiding this comment.
There was a problem hiding this comment.
Right. TraversalUtil.predicateNumber, predicateArg and predicateArgs read through JsonUtil.fromJsonExact: a fraction in P.eq(...), P.between(...), P.within(...) is a BigDecimal with every digit, and validPropertyValue converts it to the property key's type when the condition is built (a DOUBLE key still gets a double). TraversalUtilTest: a new case with a 39-digit operand; the existing test now expects BigDecimal for fractional literals, since that is the operand type now. VertexApiTest: P.eq(39 digits) hits, P.eq with the last digit changed misses, P.within(near, exact) hits, on rocksdb and hstore.
| protected static void normalizeProperties(HugeGraph g, Map<String, Object> props) { | ||
| for (Map.Entry<String, Object> entry : props.entrySet()) { | ||
| Object value = entry.getValue(); | ||
| if (value == null || value instanceof Collection || value instanceof Map || |
There was a problem hiding this comment.
There was a problem hiding this comment.
Right. normalizeProperties delegates to TraversalUtil.validPropertyValue (now public), i.e. the rule the traversal itself uses: a list on a SINGLE key converts every member, a scalar on a LIST/SET key stays a scalar for the membership test, a list on a LIST key converts its members. VertexApiTest: a DECIMAL LIST key amounts, a list filter [39 digits, 1.0] hits, a scalar filter with the last digit changed misses.
| @Override | ||
| public Userdata deserialize(JsonParser parser, DeserializationContext context) | ||
| throws IOException { | ||
| Map<String, Object> map = PROPERTIES.deserialize(parser, context); |
There was a problem hiding this comment.
There was a problem hiding this comment.
Right. The deserializer reads only ~default_value exactly (through the now public PropertiesDeserializer.readValue); every other entry goes through the default context.readValue(parser, Object.class), so {"rate":0.85} stays a double and comes back as a JSON number. Tests: PropertiesDeserializerTest (rate and a list in the metadata as doubles, the default as BigDecimal, also when it is a list), VertexApiTest ("rate":0.85 returned by GET as a number).
| variant.setType(VariantType.VT_DOUBLE) | ||
| .setValueDouble((Double) v); | ||
| break; | ||
| case DECIMAL: |
There was a problem hiding this comment.
There was a problem hiding this comment.
Right for DECIMAL: the branch now encodes a collection as a JSON array of plain strings (["1.5","2"]) in VT_STRING, a scalar as before. One remark next to it: every other branch of this method casts the value to a scalar too ((Double) v, (Long) v, ...), so a LIST/SET of any type ends in a ClassCastException there independently of this PR; I am leaving that out of scope here and can open a separate PR that encodes collections for every type, if you want it.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The new PropertyKey.userdata() override converts every ~default_value to the key's runtime type on each userdata write, including schema reads from the backend. For existing non-decimal keys this changes the stored and returned default (a DATE default "2020-01-01" now comes back as "2020-01-01 00:00:00.000"), and a stored default that does not convert now makes the property key fail to load. The four Important points imbajin raised on this head (list-API predicates and multi-valued filters, whole-userdata exact parsing, GraphStoreIterator on LIST/SET) are still open and are not repeated here. Evidence: built hugegraph-core at 0bb7724 with JDK 11 and ran probes against it. PropertyKey with DATE type and userdata ~default_value "2020-01-01" serializes as "2020-01-01 00:00:00.000"; a memory-backend graph created with userdata("~default_value", "2020-01-01") returns the same from JsonUtil.toJson(propertyKey); a DATE default "not-a-date" and an INT LIST default 1 throw IllegalArgumentException from userdata(). Master (60c8803) has no override and SchemaElement.userdata() stores the raw value; BinarySerializer.readPropertyKey sets data_type and cardinality before readUserdata(), so the conversion runs on every schema read.
| @Override | ||
| public void userdata(String key, Object value) { | ||
| if (Userdata.DEFAULT_VALUE.equals(key)) { | ||
| value = this.normalizeDefaultValue(value); |
There was a problem hiding this comment.
Important: This override converts ~default_value for every data type, not only DECIMAL, and it also runs when a schema is read back: BinarySerializer.readPropertyKey() sets data_type and cardinality and then calls readUserdata(), which calls userdata(key, value) for each entry. On master the default stays as the user sent it, and defaultValue() converts it lazily.
Two things change for existing graphs without anyone opting in:
- The stored and returned default changes form. I built hugegraph-core at this head and created a memory-backend property key with
schema.propertyKey("day").asDate().userdata("~default_value", "2020-01-01").create().JsonUtil.toJson(propertyKey)now returns"~default_value":"2020-01-01 00:00:00.000", where master returns"2020-01-01". Clients that readGET /schema/propertykeys/{name}see a different value, and the next write of the key persists the new form. - A stored default that does not convert now throws during the schema read. On master nothing validates
~default_valueat creation (Userdata.check()only rejects null), so a graph can hold a DATE key whose default is not in one of the accepted date formats or a LIST key with a scalar default. With this changevalidValue()throwsIllegalArgumentExceptionfromuserdata()(probe:"not-a-date"on a DATE key,1on an INT LIST key), so after the upgrade every schema lookup of that key fails. On master the same key loads, and the error appears only when the default is actually applied.
Requested change: restrict the eager conversion to DataType.DECIMAL, which is the case this PR needs, and keep the raw value for the other types as before. Alternatively, convert only on the create/append path and never on the backend read path, and fall back to the raw value instead of throwing when an already stored default does not convert. Please add a test that a DATE key with ~default_value "2020-01-01" returns the same string from GET as on master.
There was a problem hiding this comment.
Right, and thanks for probing it on a real build: the round-4 override converted ~default_value for every type and on every schema read, so a DATE "2020-01-01" came back as "2020-01-01 00:00:00.000" and an unconvertible stored default broke loading the key. Done in the first of your forms: the eager conversion applies to DataType.DECIMAL only; every other type keeps the raw value as on master and defaultValue() converts it lazily when it is applied; a DECIMAL default that does not convert stays raw instead of throwing on the read (the error surfaces when the default is applied, as before). One nuance the E2E caught: the API reads ~default_value exactly, so for a non-decimal key the BigDecimal is mapped back to the Double the parser produced on master; a DOUBLE default 1.5 still comes back from GET as a number. Tests: the new PropertyKeyUserdataTest in the unit suite (DECIMAL: BigDecimal, string, list, "not-a-number" kept raw; DATE "2020-01-01" and "not-a-date" kept raw; an INT LIST key with default 1 loads; a DOUBLE default and a custom rate stay doubles) and VertexApiTest: a DATE key with "~default_value": "2020-01-01" returns exactly that string from GET.
… normalisation, exact userdata only for the default value, decimal lists in store scans
- TraversalUtil.predicateNumber/predicateArg(s) read operands exactly: a
fractional literal in P.eq(...)/P.within(...) becomes a BigDecimal and is
converted to the property key's type by validPropertyValue when the
condition is built (a DOUBLE key still gets a double).
- API.normalizeProperties delegates to TraversalUtil.validPropertyValue, so
a list on a single key converts every member and a scalar on a LIST/SET
key keeps its membership semantics.
- UserdataDeserializer reads only ~default_value exactly; other metadata
keeps Jackson's types (a {"rate":0.85} stays a JSON number).
- GraphStoreIterator encodes a DECIMAL LIST/SET as a JSON array of plain
strings instead of casting the collection.
- Tests: TraversalUtilTest (fractional operands), PropertiesDeserializerTest
(metadata types), VertexApiTest (exact P.eq/P.within filter, DECIMAL LIST
key filters, custom userdata fraction round-trips as a number).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Round 5 in d7eb80c: exact predicate operands, cardinality-aware filter normalisation through |
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The DECIMAL type itself (encoding, bounds, no-index and no-primary-key rules, exact store filters, IN/NOT_IN by value) looks right at d7eb80c, and the API-side userdata fix keeps custom metadata as doubles. The schema read path does not: both serializers now read every schema element's userdata with fromJsonExact, so a fractional custom entry on any vertex label, edge label, index label or property key becomes a BigDecimal after a restart and is returned as a JSON string. That contradicts "graphs without decimal properties are unaffected" in the PR description. Evidence: full diff 60c8803..d7eb80c. BinarySerializer.readUserdata() at line 1240 and TextSerializer.readUserdata() at line 914 call JsonUtil.fromJsonExact(userdataStr, Map.class); PropertyKey.userdata(String, Object) converts only ~default_value back; JsonUtil's mapper writes BigDecimal as a string through registerCommonSerializers(). A probe with gremlin-shaded 3.5.1 Jackson reading the stored {"rate":0.85} gives Double and {"rate":0.85} with the plain reader, BigDecimal and {"rate":"0.85"} with USE_BIG_DECIMAL_FOR_FLOATS. All latest-head CI checks pass.
| @SuppressWarnings("unchecked") | ||
| Map<String, Object> userdata = JsonUtil.fromJson(userdataStr, | ||
| Map.class); | ||
| Map<String, Object> userdata = JsonUtil.fromJsonExact(userdataStr, |
There was a problem hiding this comment.
Important: Reading schema userdata with fromJsonExact changes custom metadata on every schema element after a restart, not only the DECIMAL default.
readUserdata() here, and the same method in TextSerializer at line 914, is used for vertex labels, edge labels, index labels and property keys. With USE_BIG_DECIMAL_FOR_FLOATS every stored fraction comes back as a BigDecimal. PropertyKey.userdata(String, Object) turns only ~default_value back into a Double; every other key, and every key on the other schema types, goes to SchemaElement.userdata() unchanged. JsonUtil then writes that BigDecimal as a string through the serializer added to registerCommonSerializers().
So schema.vertexLabel("person").userdata("rate", 0.85) returns "rate": 0.85 from GET /schema/vertexlabels/person until the server restarts (or another node loads the schema from the backend), and "rate": "0.85" after. The next schema write (for example append of a property) stores the string, so the type change becomes permanent. UserdataDeserializer was written to keep {"rate":0.85} a number on the API path, and PropertyKeyUserdataTest checks that only on an in-memory key, never through a backend round trip.
I checked the parse and write steps with the gremlin-shaded 3.5.1 Jackson and a BigDecimal serializer that writes toPlainString(), as the PR does: the stored {"rate":0.85} reads as Double and writes {"rate":0.85} with fromJson, and reads as BigDecimal and writes {"rate":"0.85"} with the exact reader.
The exact reader is not needed here. A DECIMAL ~default_value is held as a BigDecimal and writeUserdata() stores it as a JSON string, so plain fromJson reads a String and normalizeDefaultValue() converts it back exactly.
Requested change: go back to JsonUtil.fromJson(userdataStr, Map.class) in both readUserdata() methods. Add a core test that sets a fractional custom userdata entry on a vertex label and a DECIMAL default on a property key, reloads the schema from the backend, and checks that the first is still a Double and the second is still the exact BigDecimal.
| @@ -222,14 +225,41 @@ protected static void checkUpdatingBody(Collection<? extends Checkable> bodies) | |||
| } | |||
|
|
|||
| @SuppressWarnings("unchecked") | |||
There was a problem hiding this comment.
Minor: The new method was inserted between @SuppressWarnings("unchecked") and parseProperties(), so the annotation now applies to normalizeProperties() and parseProperties() loses it. The annotation also sits above the Javadoc, so the Javadoc tool no longer attaches that comment to normalizeProperties(). PropertyKey.java has the same problem at lines 123-133: the block that describes normalizeDefaultValue() sits directly above the separate Javadoc of undoExact(), so neither method gets the right documentation.
Requested change: put @SuppressWarnings("unchecked") back directly above parseProperties(), and move the normalizeDefaultValue() Javadoc in PropertyKey to that method.
3 participants
Purpose of the PR
The numeric property types today are
BYTE/INT/LONG/FLOAT/DOUBLE. Values that do not fit alongand must not be rounded (token balances in wei, up to 2^256 - 1; money amounts in general) can only be stored asTEXT, which loses the one place where the server itself does arithmetic:update_strategiesinPUT /graph/{vertices,edges}/batch(SUM/BIGGER/SMALLER).UpdateStrategyalready computes inBigDecimal, but the result goes back to the property's type: withDOUBLEaSUMof10^18 + 1is10^18, andTEXTfails the strategy'sNumbertype check. This PR adds an exact decimal type so that accumulating balances during an import works. The design points were posted in #3206 on 2026-09-14; no objections so far.Main Changes
DataType.DECIMAL(12, "decimal", BigDecimal.class)in the server enum and in thehugegraph-structcopy, withisDecimal()andvalueToDecimal()(exact forBigDecimal,BigIntegerand integral Java numbers;Float/Doublethrough their shortest decimal representation; decimal strings).PropertyKey.Builder.asDecimal(), RESTdata_type: DECIMAL.No primary key, no sort key, no index, no OLAP write type with an index:
isNumber()staysfalseon purpose;VertexLabelBuilder,EdgeLabelBuilder,IndexLabelBuilderandPropertyKeyBuilderreject these with an explicit message. A primary key would go throughLongEncoding/NumericUtil, which collapses fractions into adoubleand overflows alongon uint256 (review round 3). There is no fixed-width byte-order-preserving encoding for a decimal, and faking one throughLongEncodingwould be lossy.SUM/MAX/MINaggregate types on the property key are allowed, as for numbers.Encoding in
BytesBuffer(server core and struct):vint(len)+ unscaled two's-complement bytes +vint(scale). Exact for any precision, scale preserved, 33 bytes for a uint256. Existing encodings untouched;OffheapCachegets the new value type appended at the end of its enum.JSON: always a plain string on output (
toPlainString(),HugeGraphSONModule); a string or a number literal accepted on input. A JSON number is adoubleto most clients, so a string is the only lossless representation.ConditionQuerycompares exactly when one side is aBigDecimal(instead of throughdoubleValue()); the store-side row decoder (GraphStoreIterator) maps a decimal to a string variant.BatchAPI.updateExistElement: the JSON value is normalised through the property key before theupdate_strategiesstrategy runs, on both paths (two entries of one id within a request; request vs stored element). Found by the new API test: the strategy used to receive the raw JSON value, which only worked for the types Jackson happens to produce, so a decimal (or a date) sent as a string failed the type check.Review round 1 (9d5eaab): the
hugegraph-structcopy converts DECIMAL values too (DataType.valueToDecimal(), a decimal branch inPropertyKey.convSingleValue(),Builder.asDecimal()), so a decimal default value reloaded from JSON as a string normalises on the store side as well;BigDecimalSerializer.serializeWithType()for the typed GraphSON v2/v3 mappers of gremlin-server, keeping TinkerPop'sgx:BigDecimaltype id and carrying the plain string in@value;OLAP_SECONDARY/OLAP_RANGErejected for a DECIMAL key inPropertyKeyBuilder.checkOlap()and the no-index guard repeated inIndexLabelBuilder.build(), which the OLAP path reaches withoutcheckFields().A fraction can be sent as a JSON number or as a string and is exact either way: the
propertiesobject of a vertex/edge body, a property key'suser_data(its~default_value) and thepropertiesfilter of the list APIs are read with every digit (PropertiesDeserializer,UserdataDeserializer,JsonUtil.fromJsonExact), then converted to the property key's type. The toolchain side is apache/hugegraph-toolchain#771.Verifying these changes
unit/core/DataTypeTest: predicates,valueToDecimalfor uint256 max, wei scale, integral and binary numbers, invalid stringsunit/serializer/BytesBufferTest: exact byte layout for-1.5,0, uint256 max; scale round trip; decimal listsunit/util/JsonUtilTest: string on output, string or number on inputcore/PropertyKeyCoreTest: create, value normalisation,calcSum(), list cardinalitycore/IndexLabelCoreTest: secondary / range / shard / unique on a decimal all rejectedcore/EdgeLabelCoreTest: decimal sort key rejected, decimal edge property finecore/VertexLabelCoreTest.testAddVertexLabelWithDecimalPrimaryKey: a single and a composite decimal primary key rejected, a decimal next to a text primary key finecore/VertexCoreTest: uint256 and 18-fraction-digit values through commit and reload, exacthas()vs the neighbouring value,gt/lt/gte, update, invalid valuesapi/VertexApiTest:PUT /graph/vertices/batchwithSUM:2^256-2+1(number literal), then two entries of one vertex in one request ("0.000000000000000000","0.000000000000000001"), thenBIGGER; response andGETcarry the exact stringPropertyKeyTest: groovy schema string, structBytesBufferround trip; decimal conversion from string, integral,BigIntegerand a default value (single and list)unit/core/DataTypeTest.testValueToDecimalBoundsand structPropertyKeyTest:1E+999999999,1E-999999999,1E+129, 129 digits rejected; 128 digits,1E+128,1E-128, uint256 with 18 fraction digits acceptedunit/serializer/HugeGraphSONModuleTest: aBigDecimalthroughGraphSONMessageSerializerV1d0/V2d0/V3d0withHugeGraphIoRegistry,gx:BigDecimalwith the plain string in@value, round trip for1.5,1E-18, uint256 maxcore/PropertyKeyCoreTest.testAddOlapPropertyKeyWithDecimalType:OLAP_SECONDARY/OLAP_RANGEon a decimal rejected, no*olap_by_rankindex,OLAP_COMMONallowedunit-test687/688 (SecurityManagerTest.testFilefails identically on plain master on a non-English locale, unrelated);core-teston rocksdb and memory for the four touched classes green (383 tests on rocksdb);api-test,rocksdb162 tests, 0 failures.cluster/decimal_e2e.py, 37 checks: schema with a decimal default value, no-index rule, OLAP write types, REST with batchSUMand the default value, Gremlin through the REST proxy and straight on gremlin-server with GraphSON v1/v2/v3), run on hstore and rocksdb against dists froma28554eand9d5eaab: before 24 pass / 9 fail (theOLAP_SECONDARYgap and all 8 typed GraphSON answers), after 33 pass / 0 fail on both backends; logs inresults/decimal/e2e/.balance/hi/lo DECIMAL, 5 rounds ofPUT /graph/vertices/batchwithupdate_strategies: {balance: SUM, hi: BIGGER, lo: SMALLER}, random increments up to 2^255 with 18 fraction digits, 30 % negative, batches of 500, 4 writer threads, 50 accounts per round appearing twice in one request; 2 000 edges with a decimalamountbehind an INT sort key. Every value read back and compared exactly with a PythonDecimaloracle: 50 250 upserts, 0 errors, 0 / 10 000 mismatches on both backends; decimal sort key / range index rejected with the intended message. Script and logs:cluster/decimal_sum_bench.pyandresults/decimal/in https://github.com/SebastianGruza/hugegraph-validation.decimalin the property key data type list andasDecimal()in the client table, en and cn).Note on compatibility
A graph that already contains a DECIMAL property key cannot be opened by a server built without this change (
No enum constant DataType.DECIMALat startup), as with any new data type; worth one line in the release notes.The
BigDecimalserializer is registered throughregisterCommonSerializers(), so it applies to everyjava.math.BigDecimala response carries, not only to DECIMAL property values: a Groovy decimal literal such asg.inject(1.5)or2 * 1.1came back as the JSON number1.5before this PR and comes back as the string"1.5"now (GraphSON v1 and the REST/gremlinproxy), and as{"@type":"gx:BigDecimal","@value":"1.5"}instead of@value: 1.5on GraphSON v2/v3. Deliberate, because a JSON number is adoubleto most clients; aBigDecimaldoes not record where it came from, so the serializer cannot keep numbers for non-property values. For the release notes as well.Values are bounded to 128 significant digits and an absolute scale of 128 (
DataType.DECIMAL_MAX_PRECISION/DECIMAL_MAX_SCALE), checked invalueToDecimal()on every input path including theSUMresult of a batch update;1E+999999999is rejected withDecimal value out of bounds.Release notes
DECIMAL(java.math.BigDecimal,PropertyKey.Builder.asDecimal(), RESTdata_type: DECIMAL) for exact amounts, e.g. token balances up to 2^256 - 1 with 18 fraction digits; usable with theSUM/BIGGER/SMALLERupdate strategies ofPUT /graph/{vertices,edges}/batch. Values are bounded to 128 significant digits and an absolute scale of 128. A decimal cannot be a vertex primary key, a sort key, an index field of any type or an OLAP write type with an index.java.math.BigDecimalin a REST or Gremlin response is now serialised as a JSON string ("1.5") instead of a number: on GraphSON v1 and the REST/gremlinproxy the value itself, on GraphSON v2/v3{"@type":"gx:BigDecimal","@value":"1.5"}. This also applies to Groovy decimal literals such asg.inject(1.5), which came back as the number1.5before. In batch updates a fraction has to be sent as a string.DECIMALproperty key cannot be opened by a server built without this change (No enum constant DataType.DECIMALat startup).Note on public API
New enum constant
DataType.DECIMAL(code 12), new builder methodPropertyKey.Builder.asDecimal(), new REST valuedata_type: DECIMAL. No change to existing types, encodings or endpoints; graphs without decimal properties are unaffected.