bitflicker64 commented on code in PR #3209:
URL: https://github.com/apache/hugegraph/pull/3209#discussion_r4030330623
##########
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:
🧹 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.
##########
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:
⚠️ 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.
--
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]
