feat(client): add the DECIMAL (BigDecimal) property data type - #771
SebastianGruza wants to merge 3 commits into
Conversation
Companion of apache/hugegraph#3209 (issue apache/hugegraph#3206): the server gains `DataType.DECIMAL`, an exact arbitrary-precision property type meant for amounts that go through batch `update_strategies: SUM`. This makes the toolchain able to declare and carry such values. hugegraph-client - `DataType.DECIMAL(12, "decimal", BigDecimal.class)` with `isDecimal()` and `valueToDecimal()` (same conversion rules as the server) in the public enum and in the direct-serializer copy; `PropertyKey.Builder .asDecimal()`. - A `BigDecimal` is serialized as a plain string ("1.10", never "1.1E+2") in request bodies and query parameters: a JSON number is parsed as a double on the server side and would lose precision and trailing zeros. Values read back are the plain string the server returns; `new BigDecimal(String)` restores them exactly. - `BytesBuffer.writeProperty` writes the server's DECIMAL layout (unscaled two's-complement bytes + scale) for the direct loaders. - `serializer/direct/struct/DataType` could never be initialised (its code table was filled before it was created); ordered the statics. - Tests: `DecimalDataTypeTest` (unit) and `DecimalPropertyApiTest` (API, skipped with `Assume` while the CI server has no DECIMAL yet). Two `BatchUpdateElementApiTest` assertions now accept the message the server produces once it normalises batch values to the property type. hugegraph-loader / hugegraph-spark-connector - `decimal` columns convert to `BigDecimal` (`DataTypeUtilTest`). hugegraph-hubble - Groovy schema export emits `.asDecimal()`.
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The DECIMAL enum entry, asDecimal(), the Groovy export and the direct-serializer layout line up with apache/hugegraph#3209 (code 12, unscaled bytes plus VInt scale). Three gaps remain: the global string serializer changes what every BigDecimal sent to numeric keys looks like, the loader's JSON source rounds decimals through double before parseDecimal sees them, and the client conversion skips the server's precision and scale bounds, which the HBase direct path relies on. Evidence: gh pr diff 771 at e828355; AbstractRestClient serializes bodies through JsonUtilCommon.toJson; server DataType.valueToNumber returns null for a non-numeric-typed String other than Infinity/NaN; a default ObjectMapper reads {"amount": 12345678901234567890.10} as Double 1.2345678901234567E19, which new BigDecimal(v.toString()) turns into 12345678901234567000; #3209 adds DataType.checkDecimalBounds (128 digits, scale 128) on the server; CI on this head is action_required, so there is no CI signal.
| // Request bodies: a decimal goes as a plain string, see JsonUtil | ||
| SimpleModule decimals = new SimpleModule(); | ||
| decimals.addSerializer(BigDecimal.class, new BigDecimalSerializer()); | ||
| JsonUtilCommon.registerModule(decimals); |
There was a problem hiding this comment.
JsonUtilCommon mapper, so every BigDecimal in any request body now goes out as "1.5", not only values for DECIMAL keys.
On the server, a DOUBLE/FLOAT/LONG/INT key goes through DataType.valueToNumber, which returns null unless the value is a Number or one of the Infinity/NaN strings. So a client that today writes vertex.property("price", new BigDecimal("1.5")) to a asDouble() key (common when values come from JDBC NUMERIC columns) gets a 400 Invalid property value after this change, and a BigDecimal Gremlin binding turns into a string inside the script. The loader and spark connector are safe only because they convert to Double first.
Please either make the server accept numeric strings for numeric keys as part of #3209, or limit the string form to values headed for DECIMAL keys, and add an API test that writes a BigDecimal to a DOUBLE key. If the behaviour change is intended, it needs a line in the PR description and release notes under "public API".
There was a problem hiding this comment.
Good catch, thanks: a client feeding JDBC NUMERIC into an asDouble() key would have got a 400. I changed the approach in a57545f: a BigDecimal is no longer a string but a plain JSON number in toPlainString() form (1000, not 1E+3; every digit). For numeric keys nothing changes against any server, since valueToNumber accepts any Number; for DECIMAL the exactness comes from the server side: in apache/hugegraph#3209 (0146849b) Jersey now reads JSON fractions as BigDecimal (ObjectMapperResolver, USE_BIG_DECIMAL_FOR_FLOATS), so a 39-digit literal reaches a DECIMAL key intact while a DOUBLE key narrows it to a double as before. VertexApiTest.testCreateWithBigDecimalOnDoubleKey writes new BigDecimal("1.5") to a DOUBLE key and reads 1.5 back; it runs in the regular ApiTestSuite, i.e. against the 1.7.0 server in CI as well. A Gremlin binding holding a BigDecimal stays a number. There is no public-behaviour change left to note in the release notes; the PR description's "public API" section is updated accordingly.
| private static BigDecimal parseDecimal(String key, Object rawValue) { | ||
| BigDecimal decimal; | ||
| try { | ||
| decimal = DataType.DECIMAL.valueToDecimal(rawValue); |
There was a problem hiding this comment.
JsonLineParser reads lines with the loader's default ObjectMapper (JsonUtil.convertMap), which parses float literals as Double. A line like {"amount": 12345678901234567890.10} reaches here as 1.2345678901234567E19 and is stored as 12345678901234567000 with no error. I reproduced this with a plain ObjectMapper and new BigDecimal(v.toString()).
The same parser also makes decimal list columns from JSON unusable: the list holds Double/String elements, checkDataType only accepts BigDecimal, and parseMultiValues then fails with "must be String type" (the new test asserts that failure).
Please read JSON numbers as BigDecimal for these columns (for example DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS on the line parser, then let parseNumber narrow for DOUBLE keys), or reject Float/Double input for DECIMAL keys so the loss is loud. A JSON-source case with more than 17 significant digits in DataTypeUtilTest would pin it.
There was a problem hiding this comment.
Done the way you suggest: the loader's JsonUtil.MAPPER has USE_BIG_DECIMAL_FOR_FLOATS, so JsonLineParser hands fractions over as BigDecimal and parseNumber narrows them for DOUBLE/FLOAT/INT/LONG keys as before (integer literals still arrive as Integer/Long). Lists: parseMultiValues now converts collection elements one by one through parseSingleValue when not all of them already have the right type, so [1.10, 2, 3E-18] yields three BigDecimals instead of "must be String type"; my earlier test asserted that failure and is corrected. DataTypeUtilTest.testConvertDecimalFromJsonLine parses a line with a 39-digit value, a mixed list and a DOUBLE key. I kept accepting Double input for DECIMAL keys (shortest round-trip representation, the same rule as the server) rather than rejecting it: after this change a Double no longer comes out of the JSON parser at all.
| case DECIMAL: | ||
| // Same layout as the server: unscaled two's-complement | ||
| // bytes followed by the scale, exact for any precision | ||
| BigDecimal decimal = dataType.valueToDecimal(value); |
There was a problem hiding this comment.
valueToDecimal says it converts "the same way the server does", but #3209 also runs DataType.checkDecimalBounds (at most 128 significant digits, scale at most 128 either way), and this copy does not. Through the REST path the server still rejects bad values, but the HBase direct loader writes these bytes straight into storage via HBaseSerializer, so a source value such as 1E+999999999 is stored in a few bytes, and every later server read calls toPlainString() on it and builds a string of about a billion characters. That is the case the server-side bound exists to stop.
Please apply the same bounds in both client valueToDecimal copies (or in BytesBuffer before writing), with constants matching #3209, and add a unit test that an out-of-bounds value is rejected.
There was a problem hiding this comment.
Done: both valueToDecimal copies end in checkDecimalBounds with the #3209 constants (DECIMAL_MAX_PRECISION = 128, DECIMAL_MAX_SCALE = 128), so BytesBuffer.writeProperty rejects 1E+999999999 before anything reaches HBase. DecimalDataTypeTest.testDecimalBounds covers 128 digits and 1E±128 as the boundary, 129 digits, 1E-129 and 1E+999999999 on both copies and on writeProperty.
…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.
|
Round 1 in a57545f: |
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The round-1 fixes hold at a57545f. BigDecimal now goes out as a plain JSON number, both valueToDecimal copies apply the 128/128 bounds, and the direct-serializer layout and code 12 match apache/hugegraph#3209 at 0146849b. Two minor points remain. The loader-wide USE_BIG_DECIMAL_FOR_FLOATS changes the text stored for JSON fractions loaded into TEXT keys, and the DECIMAL enum comment still says values are sent as strings. Evidence: git diff 3b385c3d a57545f3 (full head diff, 19 files); #3209 BytesBuffer.writeProperty/readProperty DECIMAL case and HugeGraphSONModule.BigDecimalSerializer (writes a string) read against the client; a Jackson 2.12.3 program parsing {"a":1.50,"b":1e-7,"c":12345678901.0} with and without USE_BIG_DECIMAL_FOR_FLOATS; the CI workflows on this head are all action_required, so there is no CI signal.
| * every digit (a double would keep 17), the other numeric types are | ||
| * narrowed by DataTypeUtil as before. | ||
| */ | ||
| MAPPER.enable(DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS); |
There was a problem hiding this comment.
🧹 This flag is set on the loader's only ObjectMapper, so it also changes the value that JSON sources store in TEXT keys, not only DECIMAL ones. DataTypeUtil.parseSingleValue (line 206) returns value.toString() for a Number going into a TEXT key, and a BigDecimal prints differently from the Double it replaces. I ran Jackson 2.12.3 on the same line with and without the flag:
| JSON literal | before (Double) | after (BigDecimal) |
|---|---|---|
1.50 |
1.5 |
1.50 |
1e-7 |
1.0E-7 |
1E-7 |
12345678901.0 |
1.2345678901E10 |
12345678901.0 |
An existing JSON-to-TEXT mapping therefore stores different strings after this upgrade. When that column is a primary key, a reload produces new vertex ids next to the old ones. The comment above and the PR description say only that other numeric types keep narrowing, and they do not mention this.
Requested change: keep the old TEXT output, for example by returning Double.toString(((BigDecimal) value).doubleValue()) for a BigDecimal in the TEXT branch (this gives the same string the Double path produced), and add a TEXT case to testConvertDecimalFromJsonLine. If the new form is intended, say so in the PR description and the release notes.
There was a problem hiding this comment.
Good catch, thanks: that would have been a silent id change on reload. Done in the form you propose: the TEXT branch of parseSingleValue formats a BigDecimal through Double.toString(((BigDecimal) value).doubleValue()), i.e. exactly the string the Double path produced. testConvertDecimalFromJsonLine now has a TEXT case with your three literals plus an integer: 1.50 -> "1.5", 1e-7 -> "1.0E-7", 12345678901.0 -> "1.2345678901E10", 7 -> "7". The comment above the flag now speaks about all target types.
| UUID(11, "uuid", UUID.class), | ||
| /* | ||
| * Arbitrary-precision decimal (java.math.BigDecimal), stored exactly by | ||
| * the server; sent and received as a plain decimal string in JSON |
There was a problem hiding this comment.
🧹 This comment says a DECIMAL value is "sent and received as a plain decimal string in JSON". a57545f changed the send side: BigDecimalSerializer calls generator.writeNumber(value.toPlainString()), and DecimalDataTypeTest.testDecimalIsSerializedAsPlainNumber asserts "balance":12345678901234567890.10 with no quotes. Only the read side is a string, because #3209's HugeGraphSONModule.BigDecimalSerializer uses writeString. This is the public enum, so the comment is where client users will look to learn the wire format.
Requested change: reword it along the lines of "sent as a plain JSON number (toPlainString), returned by the server as a plain decimal string".
There was a problem hiding this comment.
Done: "sent as a plain JSON number (BigDecimal.toPlainString(), every digit), returned by the server as a plain decimal string; new BigDecimal(String) restores it exactly".
… DECIMAL wire format Review round 2 of the DECIMAL companion: - The loader parses JSON fractions as BigDecimal since a57545f; a fraction loaded into a TEXT key printed differently ("1.50" instead of "1.5"), which would change primary-key ids on a reload. The TEXT branch now formats a BigDecimal through Double.toString, the string the Double path produced; DataTypeUtilTest pins 1.50, 1e-7, 12345678901.0 and an integer. - The public DataType.DECIMAL comment describes the actual wire format: sent as a plain JSON number, returned by the server as a plain decimal string.
|
Round 2 in 56fcad0: a JSON fraction loaded into a TEXT key yields the same string as before the BigDecimal parsing (test with four literals), and the |
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The DECIMAL enum entry, asDecimal(), the plain-number serializer, the 128/128 bounds and the direct-serializer layout match apache/hugegraph#3209 at 0146849b, and the round-2 changes answer the earlier comments. One regression came in with round 2: the TEXT branch of DataTypeUtil.parseSingleValue now formats every BigDecimal through double, and JDBC sources hand BigDecimal to it for every DECIMAL/NUMERIC column, so existing JDBC-to-TEXT loads store different strings and PK-derived vertex ids change. Evidence: git diff 3b385c3d..56fcad0e over the exact head; JDBCFetcher and RowFetcher pass ResultSet.getObject(i) through unchanged; a JDK run of Double.toString(new BigDecimal(s).doubleValue()) gives 12345 -> 12345.0, 12.50 -> 12.5, 12345678901234567890.12 -> 1.2345678901234567E19, where the old value.toString() kept the input; every workflow run on this head is action_required, so there is no CI signal.
| // JSON fractions are parsed as BigDecimal (see JsonUtil); | ||
| // a TEXT key keeps the string the double path produced | ||
| // ("1.5", not "1.50"), so existing ids do not change | ||
| return Double.toString(((BigDecimal) value).doubleValue()); |
There was a problem hiding this comment.
BigDecimal, not only for the ones the JSON parser now produces. JDBC sources are the common other producer: JDBCFetcher and RowFetcher put ResultSet.getObject(i) straight into the line, and MySQL/PostgreSQL DECIMAL/NUMERIC and every Oracle NUMBER column come back as BigDecimal. Before this PR those values reached the value instanceof Number check below and kept value.toString().
Output for a TEXT key, old vs new (checked on a JDK):
| JDBC value | before | after |
|---|---|---|
12345 (Oracle NUMBER(10)) |
12345 |
12345.0 |
12.50 (DECIMAL(10,2)) |
12.50 |
12.5 |
12345678901234567890.12 |
12345678901234567890.12 |
1.2345678901234567E19 |
So a JDBC column mapped to a TEXT key now stores different strings, the last one lossy, and when that key is a primary key a reload creates new vertex ids next to the old ones. This is the same silent id change the round-2 fix was meant to prevent for JSON, moved to JDBC.
Please apply the double formatting only when the value came from the JSON parser, for example when source is a FileSource/HDFSSource or KafkaSource whose format() is JSON, and keep value.toString() for everything else. A DataTypeUtilTest case that converts new BigDecimal("12.50") for a TEXT key with a JDBCSource and expects "12.50" would pin it.
Purpose of the PR
Toolchain side of apache/hugegraph#3209 (issue apache/hugegraph#3206): the server gains
DataType.DECIMAL, an exact arbitrary-precision property type meant for amounts that go through batchupdate_strategies: SUM. Without this change the Java client cannot declare such a key (asDecimal()), aBigDecimalsent through Jackson lands as a JSON number that the server reads as a double, the loader cannot convert adecimalcolumn, and Hubble cannot export such a schema to Groovy.The PR is self-contained: it compiles and tests against today's server, and the API test only switches on when the server knows DECIMAL.
Main Changes
hugegraph-client
DataType.DECIMAL(12, "decimal", BigDecimal.class)withisDecimal()andvalueToDecimal()(the same conversion rules as the server: BigDecimal and integral numbers exactly, float/double through their shortest representation, decimal strings) in the public enum and in the direct-serializer copy;PropertyKey.Builder.asDecimal(). Code 12, as in #3209.BigDecimalis serialized as a plain JSON number (1.10, never1.1E+2) in request bodies (JsonUtilCommon, registered inRestClient) and in query parameters (the client'sJsonUtil); the server side of #3209 reads JSON fractions asBigDecimal, so every digit reaches a DECIMAL key, while numeric keys keep narrowing the number as they always did. Values read back are the string the server returns;new BigDecimal(String)restores them exactly. This matches how the client treats DATE (long) and UUID (string) today: no schema-driven conversion on read.BytesBuffer.writePropertywrites the server's DECIMAL layout (unscaled two's-complement bytes + scale) for the direct loaders (HBase).serializer/direct/struct/DataTypecould never be initialised, because its static block filled the code table before the table was created (NPE on first use). Nothing referenced it until the new unit test did; the initialisers are now ordered.DecimalDataTypeTest(unit, 5) andDecimalPropertyApiTest(API, 6, guarded byAssumeso a server without DECIMAL skips the class instead of failing). Two assertions inBatchUpdateElementApiTestnow accept the message the server produces after #3209 (batch values are normalised to the key's data type before the strategy runs, soDate, Dateinstead ofDate, String); the assertion checks the prefix and passes on both servers.hugegraph-loader, hugegraph-spark-connector
decimalcolumns are converted toBigDecimalthrough the client'sDataType.valueToDecimal(with the server's 128/128 bounds), with error messages in the style of the rest ofDataTypeUtil; the loader's JSON parser reads fractions asBigDecimaland list elements are converted one by one;DataTypeUtilTest(unit, 3).hugegraph-hubble
.asDecimal()(one line inGroovySchemaCompatibility).Verifying these changes
UnitTestSuite(JDK 11)DecimalPropertyApiTestagainst a server built from #3209 (rocksdb, auth off, defaultBaseClientTestsettings)UnitTestSuiteapache-rat:check,checkstyle:check(client, loader, spark)What the API test covers on the wire: a
data_type: DECIMALkey through the API andSchemaManager; a vertex withnew BigDecimal("12345678901234567890.10")read back as the same plain string through the vertex API, the driver and Gremlin;1E-18,42, uint256 max and an integer literal stay exact;"1,10"is rejected with 400; batchSUMgives0.3for0.1 + 0.2and keeps the 18th fraction digit on a 21-digit value; a range index on a decimal key is rejected with 400.Does this PR potentially affect the following parts?
DataTypevalue and a new builder method; aBigDecimalis now serialized as a plain JSON number intoPlainString()form instead of Jackson's scientific notation, still a number, so existing numeric keys behave as before)Documentation Status
Doc - TODO(the data types page in hugegraph-doc, together with the docs for #3209; separate PR)Doc - DoneDoc - No Need