SebastianGruza commented on code in PR #3209:
URL: https://github.com/apache/hugegraph/pull/3209#discussion_r4027520627
##########
hugegraph-struct/src/main/java/org/apache/hugegraph/struct/schema/PropertyKey.java:
##########
@@ -424,6 +424,9 @@ public String convert2Groovy(boolean attachIdFlag) {
case UUID:
builder.append(".asUUID()");
break;
+ case DECIMAL:
Review Comment:
Done in 9d5eaab: `valueToDecimal()` ported into the struct `DataType`
unchanged from the server copy, a decimal branch in struct
`PropertyKey.convSingleValue()`, and `asDecimal()` on the struct
`PropertyKey.Builder`. Tests in struct `PropertyKeyTest`: conversion from a
string, `Long`/`Integer`, `BigInteger`, exponent notation and uint256 max,
`"1,5"` and a `Date` rejected; a default value from userdata as a string, as an
integral literal and as a list under LIST cardinality, all coming back as
`BigDecimal`. struct 5/5 on JDK 11.
##########
hugegraph-struct/src/main/java/org/apache/hugegraph/struct/schema/PropertyKey.java:
##########
@@ -424,6 +424,9 @@ public String convert2Groovy(boolean attachIdFlag) {
case UUID:
builder.append(".asUUID()");
break;
+ case DECIMAL:
Review Comment:
Done in 9d5eaab, same change as in the thread above: `valueToDecimal()` in
the struct `DataType`, the decimal branch in `convSingleValue()`, `asDecimal()`
on the builder, tests for string, integral, `BigInteger` and default-value
input (single and list).
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/io/HugeGraphSONModule.java:
##########
@@ -956,4 +962,36 @@ public Blob deserialize(JsonParser jsonParser,
return Blob.wrap(bytes);
}
}
+
+ private static class BigDecimalSerializer extends
StdSerializer<BigDecimal> {
Review Comment:
Done in 9d5eaab, and thanks for checking this through the real serializers,
I had only tested `JsonUtil`. `BigDecimalSerializer` now has
`serializeWithType()` in the same shape as `IdSerializer` in this module:
`typeSer.typeId(value, VALUE_STRING)`, prefix, `serialize()`, suffix. While at
it I removed the `BigDecimal` entry from the module's `TYPE_DEFINITIONS`: with
it the type id came out as `hugegraph:BigDecimal`, which no client knows;
without it the id stays `gx:BigDecimal` from `GraphSONXModule`, and our
serializer still wins the lookup because the registry is added later.
Result: V1 gives `"1.5"`, V2 and V3 give
`{"@type":"gx:BigDecimal","@value":"1.5"}`. The string in `@value` is
deliberate: a number there is decoded as a double by the JS/Python clients, and
this type exists to avoid exactly that; Jackson's default `BigDecimal`
deserializer and `gx:BigDecimal` in gremlin-python both accept a string. If you
would rather keep a number in `@value` for compatibility with the previous
`gx:BigDecimal` output, it is a one-line change, but then it should be said
explicitly in the type's description.
Test: new `unit/serializer/HugeGraphSONModuleTest` (in `UnitTestSuite`)
builds a `ResponseMessage` with a `BigDecimal`, runs it through
`GraphSONMessageSerializerV1d0/V2d0/V3d0` configured with `ioRegistries:
[HugeGraphIoRegistry]`, checks the type prefix and the string in `@value`, and
deserializes the response back to an equal `BigDecimal` for `1.5`, `1E-18` and
uint256 max. 3/3 on JDK 11.
End to end on the lab, dists from both heads, hstore and rocksdb
(`cluster/decimal_e2e.py`, group R6 in `results/decimal/e2e/` of
https://github.com/SebastianGruza/hugegraph-validation): before, every one of
the 8 queries through gremlin-server with `Accept` v2.0 and v3.0 (`values()` on
uint256 max, `g.inject(1.5)`, `values()` of a default value, `sum()`) → `500
Type id handling not implemented`; after, all 8 return `gx:BigDecimal` with the
exact value, `sum()` exact to the 18th fraction digit. `/gremlin` through the
REST proxy (`application/json`, untyped) worked on both heads.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/schema/builder/IndexLabelBuilder.java:
##########
@@ -472,6 +472,9 @@ private void checkFields(Set<Id> propertyIds) {
E.checkArgument(pkey.aggregateType().isIndexable(),
"The aggregate type %s is not indexable",
pkey.aggregateType());
+ E.checkArgument(!pkey.dataType().isDecimal(),
Review Comment:
Done in 9d5eaab, both things you asked for: `PropertyKeyBuilder.checkOlap()`
rejects for DECIMAL every OLAP write type that builds an index (everything but
`OLAP_COMMON`) with "decimal keys can't be indexed", and
`IndexLabelBuilder.build()` carries the same guard as `checkFields()`, so the
rule also holds on the `createIndexLabelForOlapPk()` path and for any future
caller of `build()`. The `toString()`/scale point about the secondary index key
becomes moot, since such an index can no longer exist.
Test: `PropertyKeyCoreTest.testAddOlapPropertyKeyWithDecimalType` behind
`Assume supportsOlapProperties`: `OLAP_SECONDARY` and `OLAP_RANGE` on a decimal
key → `NotAllowException`, `*olap_by_rank` does not exist, `OLAP_COMMON` (no
index) still passes. On rocksdb: `PropertyKeyCoreTest` 25/25,
`IndexLabelCoreTest` 45/45. End to end through REST on the lab (group R3 in
`results/decimal/e2e/`): before, `write_type: OLAP_SECONDARY` on a decimal key
→ 202 and the key is created; after → 400 "decimal keys can't be indexed",
`OLAP_RANGE` 400, `OLAP_COMMON` 202, on hstore and rocksdb.
--
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]
