feat(client): add the DECIMAL (BigDecimal) property data type - #771
SebastianGruza wants to merge 7 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.
There was a problem hiding this comment.
Good catch, thanks: I moved the same problem from JSON to JDBC. Done the way you suggest: the TEXT branch formats a BigDecimal through double only when the value came from the JSON parser (FileSource/HDFSSource with format() == JSON, or KafkaSource with getFormat() == JSON, helper fromJsonParser); every other BigDecimal, including JDBC, keeps value.toString() as before this PR. DataTypeUtilTest.testConvertJdbcDecimalToTextKeepsItsText converts your three values from a JDBCSource: 12.50 -> "12.50", 12345 -> "12345", 12345678901234567890.12 unchanged; the JSON case from round 2 now uses a source with format: JSON explicitly.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #771 +/- ##
=============================================
- Coverage 62.49% 46.42% -16.08%
- Complexity 1903 4485 +2582
=============================================
Files 262 604 +342
Lines 9541 29509 +19968
Branches 886 3464 +2578
=============================================
+ Hits 5963 13699 +7736
- Misses 3190 14464 +11274
- Partials 388 1346 +958 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…hen it came from the JSON parser Review round 3: JDBC sources hand BigDecimal for every DECIMAL/NUMERIC column, and the round-2 TEXT branch formatted those through double too (12.50 -> 12.5, long values lossy), changing primary-key ids on reload. The double formatting now applies only to sources parsed as JSON (FileSource/HDFSSource and KafkaSource with format JSON); any other BigDecimal keeps its own text. DataTypeUtilTest pins the JDBC case.
|
Round 3 in 268af65: the |
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: DECIMAL bounds can be bypassed and plain JSON serialization can exhaust client memory; loader numeric null matching also changes. Evidence: static trace through DataType bounds, REST serialization, loader mapping/conversion, and API test setup.
| @Override | ||
| public void serialize(BigDecimal value, JsonGenerator generator, | ||
| SerializerProvider provider) throws IOException { | ||
| generator.writeNumber(value.toPlainString()); |
There was a problem hiding this comment.
value.toPlainString() for every BigDecimal without checking the DECIMAL bounds or output length. new BigDecimal("1E+999999999") needs roughly a billion plain characters and can exhaust the client heap before the server rejects it. Use a bounded or scientific JSON representation, or reject out-of-range values before expansion.
There was a problem hiding this comment.
Right. exactString(): the plain form only while the value is within the DECIMAL bounds (128 digits, scale within ±128), toString() beyond them, i.e. the scientific form: still an exact JSON number, and 1E+999999999 stays eleven characters for the server to reject on its side. The same rule as HugeGraphSONModule in apache/hugegraph#3209. testSerializerDoesNotExpandOutOfBoundsValues: 1E+3 -> 1000, 1E-128 plain, 1E+999999999 and 1E-129 scientific, through JsonUtil.toJson.
| } | ||
|
|
||
| public static BigDecimal checkDecimalBounds(BigDecimal decimal) { | ||
| int scale = Math.abs(decimal.scale()); |
There was a problem hiding this comment.
Math.abs(Integer.MIN_VALUE) remains negative, so this check accepts new BigDecimal(BigInteger.ONE, Integer.MIN_VALUE); the direct BytesBuffer.writeProperty path can then persist a DECIMAL outside the stated ±128 scale bound. Compare scale directly to both limits here and in the duplicate check in serializer/direct/struct/DataType.
There was a problem hiding this comment.
Right, the same slip I fixed on the server side in round 3. Both copies compare the scale to both limits directly. testDecimalBounds checks new BigDecimal(BigInteger.ONE, Integer.MIN_VALUE) and MAX_VALUE on both copies and on BytesBuffer.writeProperty.
| return values; | ||
| if (values instanceof Collection) { | ||
| // JSON file should not parse again | ||
| if (checkCollectionDataType(key, (Collection<?>) values, dataType)) { |
There was a problem hiding this comment.
checkDataType branch only checks the class and never applies the 128/128 bound used by scalar parseDecimal. Thus [1E+129] passes loader conversion while the same scalar is rejected. Validate every DECIMAL element before returning the collection.
There was a problem hiding this comment.
Right. The DECIMAL branch of checkDataType counts an element as already typed only when it is a BigDecimal within the bounds; otherwise the list goes through the element-by-element conversion and parseDecimal rejects it like the scalar. testConvertDecimalList: [1E+129] and [1E+129, 2] rejected.
| * 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.
null_values configuration and JSON rows as BigDecimal. ElementBuilder.retainField() uses Set.contains, so null_values: [1.0] no longer matches a JSON field 1.00 (BigDecimal.equals is scale-sensitive), although the previous Double values matched. Normalize numeric null sentinels or compare them numerically.
There was a problem hiding this comment.
Right. ElementBuilder.isNullValue: contains first, then for numbers a comparison by value through BigDecimal.compareTo (both sides are BigDecimal now from the same mapper; Long/Double values from JDBC or CSV match too). New NullValuesTest in the loader UnitTestSuite: a mapping with null_values: [1.0, ""] parsed through JsonUtil, a JSON row 1.00 matches, 1.5 does not, "" matches, 1L and 1.0d match, "1.0" does not.
| try { | ||
| propertyKeyAPI.create(amount); | ||
| } catch (ServerException e) { | ||
| Assume.assumeTrue("The server has no DECIMAL data type: " + |
There was a problem hiding this comment.
🧹 This assumption catches every ServerException while creating the DECIMAL key and skips the whole class, including failures from a server that supports DECIMAL. A regression or unrelated server error can therefore make the API tests appear skipped. Skip only the known unsupported-type response and rethrow other errors.
There was a problem hiding this comment.
Done: the skip applies only when the message is the enum rejection during deserialization (DataType, "DECIMAL", "not one of the values accepted", which is exactly what 1.7.0 returns); any other ServerException propagates. Checked against a 1.7.0 server on the lab: 1 skipped with that message.
…ctly, check list elements, match numeric null values by value - BigDecimalSerializer writes the plain form only within the DECIMAL bounds (128 digits, scale within +-128) and the scientific form beyond them, so a value such as 1E+999999999 is not expanded on the client before the server rejects it. - checkDecimalBounds compares the scale to both limits directly in both DataType copies: Math.abs(Integer.MIN_VALUE) stays negative. - Loader: a BigDecimal list element counts as already typed only within the bounds; otherwise it goes through parseDecimal and is rejected like the scalar. - Loader: numeric null_values match a field by value (BigDecimal.equals is scale sensitive; both sides are BigDecimal since the mapper change). - DecimalPropertyApiTest skips only on the unsupported-enum response of a server without the type and rethrows anything else. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Round 4 in 4ca9ce2: the plain form only within the DECIMAL bounds (scientific beyond them, no expansion on the client), the scale compared directly in both copies, DECIMAL list elements checked against the bounds, numeric |
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: Existing loader mappings for JSON numeric values can stop matching after the parser preserves decimal scale, changing mapped properties or IDs; the inline comments cover additional conversion edge cases. Evidence: Mapping keys are built with String.valueOf(value) and looked up by exact string key.
| * 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.
There was a problem hiding this comment.
Right, the same effect as with TEXT keys in round 2, and it gets the same recipe. DataTypeUtil.mappingKey(value, source): for a BigDecimal from a source parsed as JSON the lookup key is the string the double path produced (1.00 -> "1.0", 1e-7 -> "1.0E-7"), any other value goes through String.valueOf as before; ElementBuilder.mappingValue uses the helper. Test NullValuesTest.testJsonFractionMappingKey: a JSON row with 1.00, 1e-7, -0.0, 7, "1.00", and the same BigDecimal from a CSV/JDBC source keeping its own text.
| // ("1.5", not "1.50"), so existing ids do not change. | ||
| // A BigDecimal from any other source (JDBC DECIMAL/NUMERIC) | ||
| // keeps its own text, as before | ||
| return Double.toString(((BigDecimal) value).doubleValue()); |
There was a problem hiding this comment.
There was a problem hiding this comment.
Right. JsonValueDeser (registered for Object.class on the loader's mapper, extends UntypedObjectDeserializer.Vanilla) returns fractions as BigDecimal but -0.0 as Double -0.0, since BigDecimal cannot carry the sign of zero; a TEXT key and an id keep "-0.0", a DECIMAL key gets zero. Covered in testConvertDecimalFromJsonLine.
| return new BigDecimal((BigInteger) number); | ||
| } | ||
| if (number instanceof Double || number instanceof Float) { | ||
| return new BigDecimal(number.toString()); |
There was a problem hiding this comment.
There was a problem hiding this comment.
Right. toBigDecimal returns null for NaN and the infinities and isNullValue leaves them to the plain contains (equal only to themselves). Tests in NullValuesTest.
| } | ||
| E.checkArgument(decimal != null, | ||
| "Failed to convert value(key='%s') '%s'(%s) to Decimal", | ||
| key, rawValue, rawValue.getClass()); |
There was a problem hiding this comment.
🧹 Blocking: no. Summary: A null element in a DECIMAL collection raises NullPointerException instead of a conversion error; reject null explicitly or build the error without dereferencing it. Evidence: valueToDecimal(null) returns null, then this error argument evaluates rawValue.getClass().
There was a problem hiding this comment.
Done: parseDecimal rejects a null element with the conversion error instead of an NPE; test in testConvertDecimalList.
…e non-finite numbers and null decimal elements
- JsonValueDeser: untyped JSON fractions stay BigDecimal except a negative
zero, which BigDecimal cannot carry; it stays a Double so TEXT values and
ids keep "-0.0".
- DataTypeUtil.mappingKey: a JSON fraction looks up the mapping by the
string the double path produced ("1.0" for 1.00), so existing mappings
keep matching; any other BigDecimal keeps its own text.
- isNullValue: a NaN or infinity from JDBC is not converted to BigDecimal
(equal only to itself through contains()).
- parseDecimal rejects a null element with a conversion error.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Round 5 in 991a97f: the mapping lookup key of a JSON fraction is the string it had before BigDecimal (as with TEXT keys), a negative zero keeps its sign through a custom untyped deserializer, NaN/Infinity from JDBC no longer break the null-value filter, a null element in a DECIMAL list is a conversion error. Loader |
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The round 5 mapping-key change makes the loader round a JSON decimal through double whenever the struct has any value_mapping, so a DECIMAL column loses digits without an error. Vertex ids built from a JSON fraction with CUSTOMIZE_STRING also change from the old Double text to the BigDecimal text. The client side (code 12, asDecimal(), the plain-number serializer, the 128/128 bounds, and the direct-serializer layout) matches apache/hugegraph#3209 at d7eb80c8. Evidence: full diff 3b385c3..991a97f. ElementMapping.mappingValue() returns the lookup string when no entry matches, and ElementBuilder.customizeId() and olapVertexId() call idValue.toString(). A jshell run with Jackson 2.12.3 and USE_BIG_DECIMAL_FOR_FLOATS gives 12345678901234567890.10, then mapping key 1.2345678901234567E19, then 12345678901234567000 at the DECIMAL key. BigDecimal 1.50 and 1e2 print as 1.50 and 1E+2, where Double printed 1.5 and 100.0.
| return fieldValue; | ||
| } | ||
| String fieldStrValue = String.valueOf(fieldValue); | ||
| String fieldStrValue = DataTypeUtil.mappingKey(fieldValue, this.struct.input()); |
There was a problem hiding this comment.
Important: With any value_mapping in the struct, this line now sends a DECIMAL column through double, and the column loses digits without an error.
ElementMapping.mappingValue() starts from mappingValue = rawValue and replaces it only on a match. When nothing matches, the result is the lookup string itself. mappingKey() now builds that string with Double.toString(value.doubleValue()). Every property field goes through mappingValue() (lines 539, 611, 702 and others), including fields that have no mapping entry. So a struct that maps only city still turns amount into a rounded string. parseDecimal then accepts that string as a valid decimal.
I checked this in jshell with Jackson 2.12.3 and USE_BIG_DECIMAL_FOR_FLOATS:
| JSON literal | key from mappingKey |
value at the DECIMAL key |
|---|---|---|
12345678901234567890.10 |
1.2345678901234567E19 |
12345678901234567000 |
0.123456789012345678 |
0.12345678901234568 |
0.12345678901234568 |
Before round 5 this path used String.valueOf, which kept every digit of a BigDecimal. NullValuesTest.testJsonFractionMappingKey checks the lookup key only. No test sends a mapped struct into a DECIMAL key.
Requested change: use the double text only to look up the map. When no entry matches, return the original fieldValue, not the lookup string. Please add a test with a value_mapping on another field and a 20-digit JSON decimal loaded into a DECIMAL key.
There was a problem hiding this comment.
Right, thanks for the jshell table. The double text is now used only for the lookup: ElementMapping.mappedValue() returns null when no entry matches, and DataTypeUtil.mapValue() then returns the original field value itself (an identity check, not the lookup string). Test testMappedStructKeepsUnmappedDecimal: a struct with a value_mapping on city and a 20-digit JSON decimal loaded into a DECIMAL key, expected exactly 12345678901234567890.10.
| /** | ||
| * The string a JSON fraction had before it was read as BigDecimal: the | ||
| * shortest double representation ("1.5" for 1.50, "1.0E-7" for 1e-7). | ||
| * TEXT values and mapping lookups keep the keys they always had. |
There was a problem hiding this comment.
Important: TEXT values and mapping keys now keep their old strings, but vertex ids built from a JSON fraction still change.
ElementBuilder.customizeId() builds a CUSTOMIZE_STRING id with idValue.toString() (line 354). olapVertexId() does the same for CUSTOMIZE_STRING and PRIMARY_KEY (line 840). Edge endpoints go through customizeId() too, because EdgeBuilder builds source and target through VertexKVPairs. With no value_mapping, idValue is the raw BigDecimal from JsonValueDeser, so it never passes through jsonNumberText.
| JSON id literal | id before this PR (Double) | id at this head (BigDecimal) |
|---|---|---|
1.50 |
1.5 |
1.50 |
1e2 |
100.0 |
1E+2 |
1e-7 |
1.0E-7 |
1E-7 |
A reload of an existing JSON file creates new vertices next to the old ones, and edges point at ids that no longer match. Integer literals are not affected because they still parse as Integer or Long.
Requested change: format a JSON BigDecimal with jsonNumberText wherever the loader turns a field value into a string id. That covers customizeId() and olapVertexId(), and splitField() for flat ids. Please add a test that loads a CUSTOMIZE_STRING id of 1.50 from a JSON row and expects 1.5.
There was a problem hiding this comment.
Right. New DataTypeUtil.idText(value, source) formats a JSON BigDecimal through jsonNumberText; customizeId(), olapVertexId() and splitField() use it. Test testJsonFractionIdText: a CUSTOMIZE_STRING id of 1.50 from a JSON row gives 1.5, 1e2 gives 100.0, as before the PR.
… string ids from JSON fractions keep the double text Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Round 6 in 5255714: a mapping lookup never replaces an unmapped value (the double text is used only to find the entry), and string ids built from JSON fractions keep the former double text. Loader 19/19, checkstyle clean. Against server 371275c02 on the lab, |
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: Numeric mapping outputs can change existing text values and IDs, and the serializer rejects the server-accepted 1E+128 boundary after plain expansion. Evidence: exact-head review of DataTypeUtil, JsonValueDeser, ElementMapping, BigDecimalSerializer, and apache/hugegraph#3209 PropertiesDeserializer/DataType bounds.
| } else if (dataType.isDecimal()) { | ||
| return parseDecimal(key, value); | ||
| } else if (dataType.isText()) { | ||
| if (value instanceof BigDecimal && fromJsonParser(source)) { |
There was a problem hiding this comment.
| int scale = value.scale(); | ||
| if (value.precision() <= DataType.DECIMAL_MAX_PRECISION && | ||
| scale >= -DataType.DECIMAL_MAX_SCALE && scale <= DataType.DECIMAL_MAX_SCALE) { | ||
| return value.toPlainString(); |
There was a problem hiding this comment.
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The round-6 mapValue change returns unmapped field values in their raw type instead of the string the loader used before, so a JSON boolean or JDBC timestamp loaded into a TEXT key fails whenever the struct has any value_mapping. The two open findings from imbajin at this head (mapped replacement values as BigDecimal, and the 1E+128 plain expansion) also reproduce statically and are not repeated here. Evidence: diff of ElementBuilder.mappingValue against 3b385c3, a temporary loader unit test at 5255714 (JDK 11, mvn -o -pl hugegraph-loader -am test) showing the TEXT conversion error, and apache/hugegraph#3209 at 2034d240 for the server side.
| return fieldValue; | ||
| } | ||
| Object mapped = mapping.mappedValue(fieldName, mappingKey(fieldValue, source), caseSensitive); | ||
| return mapped != null ? mapped : fieldValue; |
There was a problem hiding this comment.
Important: An unmapped field in a struct that has any value_mapping now keeps its raw type, so loads that worked before fail on TEXT keys.
Before this PR, ElementBuilder.mappingValue() called String.valueOf(fieldValue) and ElementMapping.mappingValue() returned that string when no entry matched. So once a struct had a value_mapping on any field, every other field reached DataTypeUtil.convert as a String. Here the unmapped case returns the original object. For a value that is neither a String nor a Number, such as a JSON true or a JDBC java.sql.Timestamp, the TEXT branch of parseSingleValue does not match and checkDataType rejects it.
I ran this at 5255714 with a struct that maps only city and a JSON row {"city":1,"active":true}, converting active for a TEXT key:
mapValuereturnsBoolean trueDataTypeUtil.convertthrowsThe value(key='active') 'true'(class java.lang.Boolean) is not match with data type TEXT and can't convert to it- the pre-PR input
String.valueOf(true)converts to"true"
An existing mapping that loads booleans or JDBC timestamps into TEXT keys next to a value_mapping stops loading after the upgrade.
Requested change: keep the raw object only where the round-6 fix needs it (a BigDecimal, so a DECIMAL column keeps every digit), and return String.valueOf(fieldValue) for every other unmapped value as before. For example: if (mapped != null) return mapped; return fieldValue instanceof BigDecimal ? fieldValue : String.valueOf(fieldValue);. Please add a NullValuesTest case that converts an unmapped JSON boolean to a TEXT key in a struct that has a value_mapping.
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