bitflicker64 commented on code in PR #771:
URL:
https://github.com/apache/hugegraph-toolchain/pull/771#discussion_r4060073090
##########
hugegraph-loader/src/main/java/org/apache/hugegraph/loader/util/DataTypeUtil.java:
##########
@@ -328,6 +331,21 @@ private static Number parseNumber(String key, Object value,
}
}
+ private static BigDecimal parseDecimal(String key, Object rawValue) {
+ BigDecimal decimal;
+ try {
+ decimal = DataType.DECIMAL.valueToDecimal(rawValue);
Review Comment:
⚠️ For JSON sources this conversion gets a value that has already lost
precision. `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.
##########
hugegraph-client/src/main/java/org/apache/hugegraph/serializer/direct/util/BytesBuffer.java:
##########
@@ -750,6 +751,13 @@ public void writeProperty(DataType dataType, Object value)
{
this.writeLong(uuid.getMostSignificantBits());
this.writeLong(uuid.getLeastSignificantBits());
break;
+ 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);
Review Comment:
⚠️ The Javadoc on `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.
##########
hugegraph-client/src/main/java/org/apache/hugegraph/client/RestClient.java:
##########
@@ -49,6 +52,11 @@ public class RestClient extends AbstractRestClient {
SimpleModule module = new SimpleModule();
module.addDeserializer(Path.class, new PathDeserializer());
RestResult.registerModule(module);
+
+ // Request bodies: a decimal goes as a plain string, see JsonUtil
+ SimpleModule decimals = new SimpleModule();
+ decimals.addSerializer(BigDecimal.class, new BigDecimalSerializer());
+ JsonUtilCommon.registerModule(decimals);
Review Comment:
⚠️ This registers the string serializer on the shared `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".
--
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]
