feat(core): add DECIMAL (BigDecimal) property data type - #3209
SebastianGruza wants to merge 1 commit 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% 38.05% +0.19%
- Complexity 6586 6624 +38
============================================
Files 800 800
Lines 68985 69070 +85
Branches 9172 9192 +20
============================================
+ Hits 26120 26286 +166
+ Misses 39796 39693 -103
- Partials 3069 3091 +22 ☔ 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.
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.
| 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.
| 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.
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.isNumber()staysfalseon purpose;PropertyKeyBuilder,IndexLabelBuilderandEdgeLabelBuilderreject these with an explicit message. 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.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.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.Known limit, documented in the issue: in the batch update a fraction has to be sent as a string, because the request's
propertiesmap is parsed by Jackson before any schema is known (0.000000000000000001becomes adoubleliteral); integral literals are exact.hugegraph-client/ loader / Hubble will get the type in a separate toolchain change.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/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 tripunit-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.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.hugegraph-docrepository needs aDECIMALrow; I will open that PR once the type is in.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.