-
Notifications
You must be signed in to change notification settings - Fork 636
feat(core): add DECIMAL (BigDecimal) property data type #3209
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -114,6 +114,12 @@ public IndexLabel build() { | |
| indexLabel.indexType(this.indexType); | ||
| for (String field : this.indexFields) { | ||
| PropertyKey propertyKey = graph.propertyKey(field); | ||
| // Also guarded in checkFields(), but build() is reached directly | ||
| // by the OLAP property-key path, which skips checkFields() | ||
| E.checkArgument(!propertyKey.dataType().isDecimal(), | ||
| "Not allowed to build index on property key " + | ||
| "'%s' whose data type is decimal", | ||
| propertyKey.name()); | ||
| indexLabel.indexField(propertyKey.id()); | ||
| } | ||
| indexLabel.userdata(this.userdata); | ||
|
|
@@ -472,6 +478,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(), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 9d5eaab, both things you asked for: Test: |
||
| "Not allowed to build index on property key " + | ||
| "'%s' whose data type is decimal", pkey.name()); | ||
|
|
||
| if (pkey.cardinality().multiple()) { | ||
| E.checkArgument(fields.size() == 1, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BigDecimalSerializeronly overridesserialize(), but this module is registered into the typed GraphSON mappers used by gremlin-server (GraphSONMessageSerializerV2d0/V3d0ingremlin-server.yaml, and V3d0 also answersapplication/json). Jackson then callsserializeWithType(), whichStdSerializerdoes not implement. I checked this at a28554e by building aResponseMessagewithnew BigDecimal("1.5")and serializing it through each serializer configured withioRegistries: [HugeGraphIoRegistry]. V1d0 returns"1.5". V2d0 and V3d0 both fail withInvalidDefinitionException: 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}. Sog.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.inject(1.5)). That used to work. Requested change: implementserializeWithType(for example viatypeSer.writeTypePrefix/writeTypeSuffix, as the other typed serializers in this module do), or limit the string serializer toJsonUtiland leave the TinkerPopgx:BigDecimalhandling alone. Add a test that serializes a BigDecimal through GraphSON v2 and v3 withHugeGraphIoRegistry.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 9d5eaab, and thanks for checking this through the real serializers, I had only tested
JsonUtil.BigDecimalSerializernow hasserializeWithType()in the same shape asIdSerializerin this module:typeSer.typeId(value, VALUE_STRING), prefix,serialize(), suffix. While at it I removed theBigDecimalentry from the module'sTYPE_DEFINITIONS: with it the type id came out ashugegraph:BigDecimal, which no client knows; without it the id staysgx:BigDecimalfromGraphSONXModule, 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@valueis deliberate: a number there is decoded as a double by the JS/Python clients, and this type exists to avoid exactly that; Jackson's defaultBigDecimaldeserializer andgx:BigDecimalin gremlin-python both accept a string. If you would rather keep a number in@valuefor compatibility with the previousgx:BigDecimaloutput, it is a one-line change, but then it should be said explicitly in the type's description.Test: new
unit/serializer/HugeGraphSONModuleTest(inUnitTestSuite) builds aResponseMessagewith aBigDecimal, runs it throughGraphSONMessageSerializerV1d0/V2d0/V3d0configured withioRegistries: [HugeGraphIoRegistry], checks the type prefix and the string in@value, and deserializes the response back to an equalBigDecimalfor1.5,1E-18and 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 inresults/decimal/e2e/of https://github.com/SebastianGruza/hugegraph-validation): before, every one of the 8 queries through gremlin-server withAcceptv2.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 returngx:BigDecimalwith the exact value,sum()exact to the 18th fraction digit./gremlinthrough the REST proxy (application/json, untyped) worked on both heads.