Skip to content

feat(core): add DECIMAL (BigDecimal) property data type - #3209

Open
SebastianGruza wants to merge 3 commits into
apache:masterfrom
SebastianGruza:feat/decimal-datatype
Open

SebastianGruza wants to merge 3 commits into
apache:masterfrom
SebastianGruza:feat/decimal-datatype

Conversation

@SebastianGruza

@SebastianGruza SebastianGruza commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Purpose of the PR

The numeric property types today are BYTE/INT/LONG/FLOAT/DOUBLE. Values that do not fit a long and must not be rounded (token balances in wei, up to 2^256 - 1; money amounts in general) can only be stored as TEXT, which loses the one place where the server itself does arithmetic: update_strategies in PUT /graph/{vertices,edges}/batch (SUM / BIGGER / SMALLER). UpdateStrategy already computes in BigDecimal, but the result goes back to the property's type: with DOUBLE a SUM of 10^18 + 1 is 10^18, and TEXT fails the strategy's Number type 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 the hugegraph-struct copy, with isDecimal() and valueToDecimal() (exact for BigDecimal, BigInteger and integral Java numbers; Float/Double through their shortest decimal representation; decimal strings). PropertyKey.Builder.asDecimal(), REST data_type: DECIMAL.

  • No sort key, no index, no OLAP range: isNumber() stays false on purpose; PropertyKeyBuilder, IndexLabelBuilder and EdgeLabelBuilder reject these with an explicit message. There is no fixed-width byte-order-preserving encoding for a decimal, and faking one through LongEncoding would be lossy. SUM/MAX/MIN aggregate 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; OffheapCache gets 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 a double to most clients, so a string is the only lossless representation.

  • ConditionQuery compares exactly when one side is a BigDecimal (instead of through doubleValue()); 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 the update_strategies strategy 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-struct copy converts DECIMAL values too (DataType.valueToDecimal(), a decimal branch in PropertyKey.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's gx:BigDecimal type id and carrying the plain string in @value; OLAP_SECONDARY/OLAP_RANGE rejected for a DECIMAL key in PropertyKeyBuilder.checkOlap() and the no-index guard repeated in IndexLabelBuilder.build(), which the OLAP path reaches without checkFields().

Known limit, documented in the issue: in the batch update a fraction has to be sent as a string, because the request's properties map is parsed by Jackson before any schema is known (0.000000000000000001 becomes a double literal); integral literals are exact. hugegraph-client / loader / Hubble will get the type in a separate toolchain change.

Verifying these changes

  • Trivial rebase, no need to test
  • Unit tests / core tests / API tests added and passing locally (JDK 11, through the CI scripts):
    • unit/core/DataTypeTest: predicates, valueToDecimal for uint256 max, wei scale, integral and binary numbers, invalid strings
    • unit/serializer/BytesBufferTest: exact byte layout for -1.5, 0, uint256 max; scale round trip; decimal lists
    • unit/util/JsonUtilTest: string on output, string or number on input
    • core/PropertyKeyCoreTest: create, value normalisation, calcSum(), list cardinality
    • core/IndexLabelCoreTest: secondary / range / shard / unique on a decimal all rejected
    • core/EdgeLabelCoreTest: decimal sort key rejected, decimal edge property fine
    • core/VertexCoreTest: uint256 and 18-fraction-digit values through commit and reload, exact has() vs the neighbouring value, gt/lt/gte, update, invalid values
    • api/VertexApiTest: PUT /graph/vertices/batch with SUM: 2^256-2 + 1 (number literal), then two entries of one vertex in one request ("0.000000000000000000", "0.000000000000000001"), then BIGGER; response and GET carry the exact string
    • struct PropertyKeyTest: groovy schema string, struct BytesBuffer round trip; decimal conversion from string, integral, BigInteger and a default value (single and list)
    • unit/core/DataTypeTest.testValueToDecimalBounds and struct PropertyKeyTest: 1E+999999999, 1E-999999999, 1E+129, 129 digits rejected; 128 digits, 1E+128, 1E-128, uint256 with 18 fraction digits accepted
    • unit/serializer/HugeGraphSONModuleTest: a BigDecimal through GraphSONMessageSerializerV1d0/V2d0/V3d0 with HugeGraphIoRegistry, gx:BigDecimal with the plain string in @value, round trip for 1.5, 1E-18, uint256 max
    • core/PropertyKeyCoreTest.testAddOlapPropertyKeyWithDecimalType: OLAP_SECONDARY/OLAP_RANGE on a decimal rejected, no *olap_by_rank index, OLAP_COMMON allowed
    • Results: struct 4/4; unit-test 687/688 (SecurityManagerTest.testFile fails identically on plain master on a non-English locale, unrelated); core-test on rocksdb and memory for the four touched classes green (383 tests on rocksdb); api-test,rocksdb 162 tests, 0 failures.
  • End-to-end suite for the review paths (cluster/decimal_e2e.py, 37 checks: schema with a decimal default value, no-index rule, OLAP write types, REST with batch SUM and 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 from a28554e and 9d5eaab: before 24 pass / 9 fail (the OLAP_SECONDARY gap and all 8 typed GraphSON answers), after 33 pass / 0 fail on both backends; logs in results/decimal/e2e/.
  • Cluster check on HStore (PD + 3 stores) and RocksDB: 10 000 vertices with balance/hi/lo DECIMAL, 5 rounds of PUT /graph/vertices/batch with update_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 decimal amount behind an INT sort key. Every value read back and compared exactly with a Python Decimal oracle: 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.py and results/decimal/ in https://github.com/SebastianGruza/hugegraph-validation.
  • Docs: the property-key data type list in the hugegraph-doc repository needs a DECIMAL row; I will open that PR once the type is in.

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.DECIMAL at startup), as with any new data type; worth one line in the release notes.

The BigDecimal serializer is registered through registerCommonSerializers(), so it applies to every java.math.BigDecimal a response carries, not only to DECIMAL property values: a Groovy decimal literal such as g.inject(1.5) or 2 * 1.1 came back as the JSON number 1.5 before this PR and comes back as the string "1.5" now (GraphSON v1 and the REST /gremlin proxy), and as {"@type":"gx:BigDecimal","@value":"1.5"} instead of @value: 1.5 on GraphSON v2/v3. Deliberate, because a JSON number is a double to most clients; a BigDecimal does 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 in valueToDecimal() on every input path including the SUM result of a batch update; 1E+999999999 is rejected with Decimal value out of bounds.

Note on public API

New enum constant DataType.DECIMAL (code 12), new builder method PropertyKey.Builder.asDecimal(), new REST value data_type: DECIMAL. No change to existing types, encodings or endpoints; graphs without decimal properties are unaffected.

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

codecov Bot commented Sep 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 61.66667% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.04%. Comparing base (60c8803) to head (bae56ca).
⚠️ Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
...va/org/apache/hugegraph/io/HugeGraphSONModule.java 58.82% 6 Missing and 1 partial ⚠️
...e/hugegraph/schema/builder/PropertyKeyBuilder.java 0.00% 6 Missing ⚠️
.../java/org/apache/hugegraph/api/graph/BatchAPI.java 78.57% 2 Missing and 1 partial ⚠️
...apache/hugegraph/backend/query/ConditionQuery.java 0.00% 3 Missing ⚠️
...he/hugegraph/schema/builder/IndexLabelBuilder.java 50.00% 0 Missing and 2 partials ⚠️
.../java/org/apache/hugegraph/schema/PropertyKey.java 75.00% 0 Missing and 1 partial ⚠️
...che/hugegraph/schema/builder/EdgeLabelBuilder.java 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3209      +/-   ##
============================================
+ Coverage     37.86%   38.04%   +0.18%     
- Complexity     6586     6626      +40     
============================================
  Files           800      800              
  Lines         68985    69084      +99     
  Branches       9172     9196      +24     
============================================
+ Hits          26120    26286     +166     
+ Misses        39796    39705      -91     
- Partials       3069     3093      +24     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

⚠️ Blocking: yes. Summary: This makes struct schema generation emit an API that the struct builder does not expose. Evidence: struct PropertyKey.Builder has no asDecimal(), and convSingleValue has no DECIMAL conversion/valueToDecimal; after schema/default JSON values arrive as strings, validValueOrThrow("1.5") returns null/throws, unlike server PropertyKey. Requested change: add the struct conversion and builder method, plus string/numeric/default-value tests.

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: 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 bitflicker64 left a comment

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.

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> {

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.

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.

case UUID:
builder.append(".asUUID()");
break;
case DECIMAL:

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 struct copy now knows 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.

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, 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
@SebastianGruza

Copy link
Copy Markdown
Contributor Author

Thanks for the reviews. All three blocking points are handled in 9d5eaab: the DECIMAL conversion in the struct copy (method, branch in convSingleValue, asDecimal() on the builder), serializeWithType for GraphSON v2/v3 with the gx:BigDecimal type id kept, and OLAP_SECONDARY/OLAP_RANGE rejected for decimals plus a guard in IndexLabelBuilder.build(). Four new tests (struct ×2, unit GraphSON, core OLAP), details in the inline threads. Locally on JDK 11: struct 5/5, unit 47/47 for the touched classes, core on rocksdb 384 tests in the four touched classes, 0 failures.

On top of that, end to end on live servers (hstore on PD + 3 stores, rocksdb), dists built from a28554e and 9d5eaab: 37 checks (schema, no-index rule, OLAP, REST with batch SUM and a default value, Gremlin through the REST proxy and straight on gremlin-server in GraphSON v2/v3), before 24 pass / 9 fail, after 33 pass / 0 fail on both backends; script cluster/decimal_e2e.py, logs results/decimal/e2e/, write-up in docs/decimal-datatype.md in https://github.com/SebastianGruza/hugegraph-validation.

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 (No enum constant DataType.DECIMAL at startup). PR description updated with the three points under "Main Changes" and "Verifying".

@bitflicker64 bitflicker64 left a comment

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.

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);

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.

⚠️ Important. Nothing bounds the exponent here. "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.

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 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());

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.

🧹 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.

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.

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
@SebastianGruza

Copy link
Copy Markdown
Contributor Author

Round 2 in bae56ca: DECIMAL values are bounded to 128 significant digits and an absolute scale of 128, checked in valueToDecimal() in both copies and, after removing the short-circuit in PropertyKey.convValue(), also for ready-made BigDecimals (a Gremlin literal, the SUM result). The compatibility note now says that every BigDecimal in a response comes out as a string (V1, REST proxy) or as gx:BigDecimal with the string in @value (V2/V3). Locally on JDK 11: struct 5/5, unit 48/48 for the touched classes, core on rocksdb 339 tests in three classes with no failures, api on rocksdb green. E2E (cluster/decimal_e2e.py, now 41 checks with three new ones for the bound) on rocksdb with a dist from this head: 37 PASS, 0 FAIL, 4 N-A, log results/decimal/e2e/after2-rocksdb.log in hugegraph-validation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] Add a DECIMAL (BigDecimal) property data type for exact amounts

3 participants