bitflicker64 commented on code in PR #771:
URL:
https://github.com/apache/hugegraph-toolchain/pull/771#discussion_r4068326095
##########
hugegraph-client/src/main/java/org/apache/hugegraph/structure/constant/DataType.java:
##########
@@ -33,7 +35,12 @@ public enum DataType {
TEXT(8, "text", String.class),
BLOB(9, "blob", byte[].class),
DATE(10, "date", Date.class),
- UUID(11, "uuid", UUID.class);
+ 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
Review Comment:
🧹 This comment says a DECIMAL value is "sent and received as a plain decimal
string in JSON". a57545f3 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".
##########
hugegraph-loader/src/main/java/org/apache/hugegraph/loader/util/JsonUtil.java:
##########
@@ -47,6 +48,15 @@ public final class JsonUtil {
private static final Logger LOG = Log.logger(JsonUtil.class);
private static final ObjectMapper MAPPER = new ObjectMapper();
+ static {
+ /*
+ * Read JSON fraction literals as BigDecimal: a DECIMAL column keeps
+ * 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);
Review 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.
--
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]
