bitflicker64 commented on code in PR #3209:
URL: https://github.com/apache/hugegraph/pull/3209#discussion_r4025137812
##########
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:
‼️ `BigDecimalSerializer` only overrides `serialize()`, but this module is
registered into the typed GraphSON mappers used by gremlin-server
(`GraphSONMessageSerializerV2d0`/`V3d0` in `gremlin-server.yaml`, and V3d0 also
answers `application/json`). Jackson then calls `serializeWithType()`, which
`StdSerializer` does not implement. I checked this at a28554e by building a
`ResponseMessage` with `new BigDecimal("1.5")` and serializing it through each
serializer configured with `ioRegistries: [HugeGraphIoRegistry]`. V1d0 returns
`"1.5"`. V2d0 and V3d0 both fail with `InvalidDefinitionException: Type id
handling not implemented for type java.math.BigDecimal (by serializer of type
...HugeGraphSONModule$BigDecimalSerializer)`. Without the registry the same
serializers emit `{"@type":"gx:BigDecimal","@value":1.5}`. So
`g.V().values('balance')` on a DECIMAL key fails over GraphSON v2/v3, and so
does any existing script that returns a BigDecimal, such as a Groovy decimal
literal (`g.in
ject(1.5)`). That used to work. Requested change: implement
`serializeWithType` (for example via `typeSer.writeTypePrefix/writeTypeSuffix`,
as the other typed serializers in this module do), or limit the string
serializer to `JsonUtil` and leave the TinkerPop `gx:BigDecimal` handling
alone. Add a test that serializes a BigDecimal through GraphSON v2 and v3 with
`HugeGraphIoRegistry`.
##########
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:
⚠️ The decimal guard is in `checkFields()`, which only runs on the
user-facing `create()` path. OLAP property keys build their index through
`SchemaTransaction.createIndexLabelForOlapPk()`, which calls
`IndexLabelBuilder.build()` directly and skips `checkFields()`.
`PropertyKeyBuilder.checkOlap()` also rejects only `OLAP_RANGE` for non-numeric
types. On RocksDB at a28554e,
`schema.propertyKey("rank").asDecimal().writeType(WriteType.OLAP_SECONDARY).create()`
succeeds and creates index label `*olap_by_rank type=SECONDARY`. That
contradicts the rule this PR states (no index of any type on a decimal). The
secondary index key is also built from `value.toString()`
(`SplicingIdGenerator.concatValues`), and for BigDecimal that output depends on
scale and can use exponent notation (`1E+21`), so equal numbers can map to
different index keys. Requested change: reject `OLAP_SECONDARY` (and any OLAP
write type that builds an index) for `DataType.DECIMAL` in
`PropertyKeyBuilder.checkOlap()`
, or move the decimal check into `build()`, and add a core test for it.
##########
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:
⚠️ The struct copy now knows `DataType.DECIMAL`, but struct
`PropertyKey.convSingleValue()` has no decimal branch (only
number/date/uuid/blob), and struct `DataType` has no `valueToDecimal()`. For a
DECIMAL key, a `String` or `Long` value falls through to `checkDataType()` and
returns null. One concrete case is `defaultValue()`: userdata is reloaded from
JSON, so a decimal default arrives as a string, and `validValueOrThrow(raw)`
then throws. The server-side `PropertyKey` in this PR converts these values
correctly. Requested change: port `valueToDecimal()` into the struct `DataType`
and add the decimal branch to struct `convSingleValue()`, with tests for
string, integral and default-value input.
--
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]
