SebastianGruza commented on code in PR #3209:
URL: https://github.com/apache/hugegraph/pull/3209#discussion_r4030601405
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/type/define/DataType.java:
##########
@@ -143,6 +156,42 @@ public <V> Number valueToNumber(V value) {
return number;
}
+ /**
+ * Convert a value to BigDecimal: BigDecimal as is, any other Number and a
+ * decimal string through their exact decimal representation. Float and
+ * Double go through Number.toString(), i.e. the shortest string that
+ * round-trips the binary value, so a client that already holds a lossy
+ * double gets that double, exactly.
+ *
+ * @return the BigDecimal, or null if the value is not a Number or String
+ * @throws IllegalArgumentException if the string is not a decimal number
+ */
+ public <V> BigDecimal valueToDecimal(V value) {
+ if (!this.isDecimal()) {
+ return null;
+ }
+ if (value instanceof BigDecimal) {
+ return (BigDecimal) value;
+ }
+ if (value instanceof BigInteger) {
+ return new BigDecimal((BigInteger) value);
+ }
+ if (value instanceof Byte || value instanceof Short ||
+ value instanceof Integer || value instanceof Long) {
+ return BigDecimal.valueOf(((Number) value).longValue());
+ }
+ if (!(value instanceof Number) && !(value instanceof String)) {
+ return null;
+ }
+ String text = value.toString().trim();
+ try {
+ return new BigDecimal(text);
Review Comment:
Done in bae56ca, good catch. The bound: at most 128 significant digits and
an absolute scale of at most 128 (`DataType.DECIMAL_MAX_PRECISION` /
`DECIMAL_MAX_SCALE`, the same pair in the struct copy), checked by
`checkDecimalBounds()` at the end of `valueToDecimal()`. uint256 with 18
fraction digits is 96 digits, so more than 30 digits of headroom remain, and
the longest possible `toPlainString()` is about 256 characters. The message
reads `Decimal value out of bounds: precision 1, scale -999999999 (at most 128
significant digits and a scale of at most 128 in either direction)`.
One thing your comment touched on that I had not seen:
`PropertyKey.convValue()` returned the value untouched when its type already
matched, so a ready-made `BigDecimal` (a Gremlin literal in
`addV().property()`, the `SUM` result in `BatchAPI`) never reached
`valueToDecimal()` at all. The struct test caught it: `validValueOrThrow(new
BigDecimal("1E+999999999"))` did not throw. Both copies of `convValue()` now
skip the short-circuit for decimals, so `validValueOrThrow` after the strategy
really does cover the `SUM` result, as you wrote.
Tests: `DataTypeTest.testValueToDecimalBounds` (`1E+999999999`,
`1E-999999999`, `1E+129` and 129 digits rejected; 128 digits, `1E+128`,
`1E-128` and uint256 with 18 fraction digits accepted; the same bound for a
ready-made `BigDecimal`), struct `PropertyKeyTest` (the same inputs through
`validValueOrThrow`), `PropertyKeyCoreTest` (a string and a `BigDecimal`
through `validValue`). End to end on the lab (rocksdb,
`cluster/decimal_e2e.py`, `results/decimal/e2e/after2-rocksdb.log` in
hugegraph-validation): create with `1E+999999999` → 400 "out of bounds", create
with `1E+128` and with 128 nines → 201, batch `SUM` of 128 nines + 1 (a
129-digit result) → 400 "out of bounds"; 37 PASS, 0 FAIL, 4 N-A.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/io/HugeGraphSONModule.java:
##########
@@ -183,6 +184,10 @@ public static void registerCommonSerializers(SimpleModule
module) {
module.addSerializer(Blob.class, new BlobSerializer());
module.addDeserializer(Blob.class, new BlobDeserializer());
+
+ // Decimals travel as strings: JSON numbers are doubles to most clients
+ module.addSerializer(BigDecimal.class, new BigDecimalSerializer());
Review Comment:
Right, the PR description was inaccurate there. Added to "Note on
compatibility" and for the release notes: through `registerCommonSerializers()`
the serializer applies to every `java.math.BigDecimal` in a response, so a
Groovy literal (`g.inject(1.5)`, `2 * 1.1`) used to come back as the number
`1.5` and now comes back as `"1.5"` in V1 and through the REST `/gremlin`
proxy, and as `{"@type":"gx:BigDecimal","@value":"1.5"}` instead of a number in
`@value` on V2/V3. The "numbers for BigDecimals that are not DECIMAL property
values" variant cannot be done in the serializer, since a `BigDecimal` carries
no record of where it came from; it would need a wrapper type on property
values, which I think is worse than one plain rule, "a BigDecimal is always a
string". If the maintainers prefer backward compatibility, the change is one
line (`writeNumber` instead of `writeString` in `@value`), but then the
JS/Python clients get a double.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
