Skip to content

feat(client): add the DECIMAL (BigDecimal) property data type - #771

Open
SebastianGruza wants to merge 7 commits into
apache:masterfrom
SebastianGruza:feat/client-decimal-datatype
Open

SebastianGruza wants to merge 7 commits into
apache:masterfrom
SebastianGruza:feat/client-decimal-datatype

Conversation

@SebastianGruza

@SebastianGruza SebastianGruza commented Sep 20, 2026 •

Copy link
Copy Markdown

Purpose of the PR

Toolchain side of apache/hugegraph#3209 (issue apache/hugegraph#3206): the server gains DataType.DECIMAL, an exact arbitrary-precision property type meant for amounts that go through batch update_strategies: SUM. Without this change the Java client cannot declare such a key (asDecimal()), a BigDecimal sent through Jackson lands as a JSON number that the server reads as a double, the loader cannot convert a decimal column, and Hubble cannot export such a schema to Groovy.

The PR is self-contained: it compiles and tests against today's server, and the API test only switches on when the server knows DECIMAL.

Main Changes

hugegraph-client

  • DataType.DECIMAL(12, "decimal", BigDecimal.class) with isDecimal() and valueToDecimal() (the same conversion rules as the server: BigDecimal and integral numbers exactly, float/double through their shortest representation, decimal strings) in the public enum and in the direct-serializer copy; PropertyKey.Builder.asDecimal(). Code 12, as in #3209.
  • A BigDecimal is serialized as a plain JSON number (1.10, never 1.1E+2) in request bodies (JsonUtilCommon, registered in RestClient) and in query parameters (the client's JsonUtil); the server side of #3209 reads JSON fractions as BigDecimal, so every digit reaches a DECIMAL key, while numeric keys keep narrowing the number as they always did. Values read back are the string the server returns; new BigDecimal(String) restores them exactly. This matches how the client treats DATE (long) and UUID (string) today: no schema-driven conversion on read.
  • BytesBuffer.writeProperty writes the server's DECIMAL layout (unscaled two's-complement bytes + scale) for the direct loaders (HBase).
  • Drive-by fix: serializer/direct/struct/DataType could never be initialised, because its static block filled the code table before the table was created (NPE on first use). Nothing referenced it until the new unit test did; the initialisers are now ordered.
  • Tests: DecimalDataTypeTest (unit, 5) and DecimalPropertyApiTest (API, 6, guarded by Assume so a server without DECIMAL skips the class instead of failing). Two assertions in BatchUpdateElementApiTest now accept the message the server produces after #3209 (batch values are normalised to the key's data type before the strategy runs, so Date, Date instead of Date, String); the assertion checks the prefix and passes on both servers.

hugegraph-loader, hugegraph-spark-connector

  • decimal columns are converted to BigDecimal through the client's DataType.valueToDecimal (with the server's 128/128 bounds), with error messages in the style of the rest of DataTypeUtil; the loader's JSON parser reads fractions as BigDecimal and list elements are converted one by one; DataTypeUtilTest (unit, 3).

hugegraph-hubble

  • The Groovy schema export emits .asDecimal() (one line in GroovySchemaCompatibility).

Verifying these changes

  • Need tests and can be verified as follows:
what result
client UnitTestSuite (JDK 11) 77/77
client DecimalPropertyApiTest against a server built from #3209 (rocksdb, auth off, default BaseClientTest settings) 6/6
loader UnitTestSuite 13/13
apache-rat:check, checkstyle:check (client, loader, spark) clean
compile of loader, spark-connector, hubble-be against the modified client clean

What the API test covers on the wire: a data_type: DECIMAL key through the API and SchemaManager; a vertex with new BigDecimal("12345678901234567890.10") read back as the same plain string through the vertex API, the driver and Gremlin; 1E-18, 42, uint256 max and an integer literal stay exact; "1,10" is rejected with 400; batch SUM gives 0.3 for 0.1 + 0.2 and keeps the 18th fraction digit on a 21-digit value; a range index on a decimal key is rejected with 400.

Does this PR potentially affect the following parts?

  • Nope
  • Dependencies (add/update license info)
  • Modify configurations
  • The public API (a new DataType value and a new builder method; a BigDecimal is now serialized as a plain JSON number in toPlainString() form instead of Jackson's scientific notation, still a number, so existing numeric keys behave as before)
  • Other affects (typed here)

Documentation Status

  • Doc - TODO (the data types page in hugegraph-doc, together with the docs for #3209; separate PR)
  • Doc - Done
  • Doc - No Need

Companion of apache/hugegraph#3209 (issue apache/hugegraph#3206): the
server gains `DataType.DECIMAL`, an exact arbitrary-precision property
type meant for amounts that go through batch `update_strategies: SUM`.
This makes the toolchain able to declare and carry such values.

hugegraph-client
- `DataType.DECIMAL(12, "decimal", BigDecimal.class)` with `isDecimal()`
  and `valueToDecimal()` (same conversion rules as the server) in the
  public enum and in the direct-serializer copy; `PropertyKey.Builder
  .asDecimal()`.
- A `BigDecimal` is serialized as a plain string ("1.10", never
  "1.1E+2") in request bodies and query parameters: a JSON number is
  parsed as a double on the server side and would lose precision and
  trailing zeros. Values read back are the plain string the server
  returns; `new BigDecimal(String)` restores them exactly.
- `BytesBuffer.writeProperty` writes the server's DECIMAL layout
  (unscaled two's-complement bytes + scale) for the direct loaders.
- `serializer/direct/struct/DataType` could never be initialised (its
  code table was filled before it was created); ordered the statics.
- Tests: `DecimalDataTypeTest` (unit) and `DecimalPropertyApiTest`
  (API, skipped with `Assume` while the CI server has no DECIMAL yet).
  Two `BatchUpdateElementApiTest` assertions now accept the message the
  server produces once it normalises batch values to the property type.

hugegraph-loader / hugegraph-spark-connector
- `decimal` columns convert to `BigDecimal` (`DataTypeUtilTest`).

hugegraph-hubble
- Groovy schema export emits `.asDecimal()`.

@bitflicker64 bitflicker64 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: no. Summary: The DECIMAL enum entry, asDecimal(), the Groovy export and the direct-serializer layout line up with apache/hugegraph#3209 (code 12, unscaled bytes plus VInt scale). Three gaps remain: the global string serializer changes what every BigDecimal sent to numeric keys looks like, the loader's JSON source rounds decimals through double before parseDecimal sees them, and the client conversion skips the server's precision and scale bounds, which the HBase direct path relies on. Evidence: gh pr diff 771 at e828355; AbstractRestClient serializes bodies through JsonUtilCommon.toJson; server DataType.valueToNumber returns null for a non-numeric-typed String other than Infinity/NaN; a default ObjectMapper reads {"amount": 12345678901234567890.10} as Double 1.2345678901234567E19, which new BigDecimal(v.toString()) turns into 12345678901234567000; #3209 adds DataType.checkDecimalBounds (128 digits, scale 128) on the server; CI on this head is action_required, so there is no CI signal.

// Request bodies: a decimal goes as a plain string, see JsonUtil
SimpleModule decimals = new SimpleModule();
decimals.addSerializer(BigDecimal.class, new BigDecimalSerializer());
JsonUtilCommon.registerModule(decimals);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 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".

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thanks: a client feeding JDBC NUMERIC into an asDouble() key would have got a 400. I changed the approach in a57545f: a BigDecimal is no longer a string but a plain JSON number in toPlainString() form (1000, not 1E+3; every digit). For numeric keys nothing changes against any server, since valueToNumber accepts any Number; for DECIMAL the exactness comes from the server side: in apache/hugegraph#3209 (0146849b) Jersey now reads JSON fractions as BigDecimal (ObjectMapperResolver, USE_BIG_DECIMAL_FOR_FLOATS), so a 39-digit literal reaches a DECIMAL key intact while a DOUBLE key narrows it to a double as before. VertexApiTest.testCreateWithBigDecimalOnDoubleKey writes new BigDecimal("1.5") to a DOUBLE key and reads 1.5 back; it runs in the regular ApiTestSuite, i.e. against the 1.7.0 server in CI as well. A Gremlin binding holding a BigDecimal stays a number. There is no public-behaviour change left to note in the release notes; the PR description's "public API" section is updated accordingly.

private static BigDecimal parseDecimal(String key, Object rawValue) {
BigDecimal decimal;
try {
decimal = DataType.DECIMAL.valueToDecimal(rawValue);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done the way you suggest: the loader's JsonUtil.MAPPER has USE_BIG_DECIMAL_FOR_FLOATS, so JsonLineParser hands fractions over as BigDecimal and parseNumber narrows them for DOUBLE/FLOAT/INT/LONG keys as before (integer literals still arrive as Integer/Long). Lists: parseMultiValues now converts collection elements one by one through parseSingleValue when not all of them already have the right type, so [1.10, 2, 3E-18] yields three BigDecimals instead of "must be String type"; my earlier test asserted that failure and is corrected. DataTypeUtilTest.testConvertDecimalFromJsonLine parses a line with a 39-digit value, a mixed list and a DOUBLE key. I kept accepting Double input for DECIMAL keys (shortest round-trip representation, the same rule as the server) rather than rejecting it: after this change a Double no longer comes out of the JSON parser at all.

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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: both valueToDecimal copies end in checkDecimalBounds with the #3209 constants (DECIMAL_MAX_PRECISION = 128, DECIMAL_MAX_SCALE = 128), so BytesBuffer.writeProperty rejects 1E+999999999 before anything reaches HBase. DecimalDataTypeTest.testDecimalBounds covers 128 digits and 1E±128 as the boundary, 129 digits, 1E-129 and 1E+999999999 on both copies and on writeProperty.

…unds, keep loader JSON decimals exact

Review round 1 of the DECIMAL companion:

- A BigDecimal is written as a plain JSON number (every digit, never
  E-notation) instead of a string. Numeric keys keep accepting it (the
  server narrows any Number), so `vertex.property("weight",
  new BigDecimal("1.5"))` on a DOUBLE key works against every server, and
  a server that reads fractions as BigDecimal (apache/hugegraph#3209)
  stores a DECIMAL value exactly. Covered by
  `VertexApiTest.testCreateWithBigDecimalOnDoubleKey` (runs on the CI
  server) and `DecimalDataTypeTest`.
- Both client `valueToDecimal` copies apply the server's bounds (128
  significant digits, scale 128 either way), so the HBase direct path
  cannot store a value such as 1E+999999999; unit-tested on both copies
  and on `BytesBuffer.writeProperty`.
- The loader's JSON line parser reads fractions as BigDecimal
  (`USE_BIG_DECIMAL_FOR_FLOATS`), so a decimal column keeps digits a
  double would drop; collection elements of another type (a JSON integer
  in a decimal list) are converted one by one. `DataTypeUtilTest` covers
  a 39-digit JSON value, a mixed list and DOUBLE narrowing.
@SebastianGruza

Copy link
Copy Markdown
Author

Round 1 in a57545f: BigDecimal goes out as a plain JSON number instead of a string (numeric keys work against every server; DECIMAL gets every digit once the server reads fractions as BigDecimal, apache/hugegraph#3209 0146849b), the 128/128 bounds are applied in both valueToDecimal copies and in BytesBuffer, and the loader parses JSON without a detour through double and converts list elements one by one. Tests: client unit 78/78, loader unit 14/14, DecimalPropertyApiTest 6/6 and testCreateWithBigDecimalOnDoubleKey against a server built from 0146849b; checkstyle clean. The "public API" section of the description is updated: no behaviour change for existing types.

@bitflicker64 bitflicker64 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: no. Summary: The round-1 fixes hold at a57545f. BigDecimal now goes out as a plain JSON number, both valueToDecimal copies apply the 128/128 bounds, and the direct-serializer layout and code 12 match apache/hugegraph#3209 at 0146849b. Two minor points remain. The loader-wide USE_BIG_DECIMAL_FOR_FLOATS changes the text stored for JSON fractions loaded into TEXT keys, and the DECIMAL enum comment still says values are sent as strings. Evidence: git diff 3b385c3d a57545f3 (full head diff, 19 files); #3209 BytesBuffer.writeProperty/readProperty DECIMAL case and HugeGraphSONModule.BigDecimalSerializer (writes a string) read against the client; a Jackson 2.12.3 program parsing {"a":1.50,"b":1e-7,"c":12345678901.0} with and without USE_BIG_DECIMAL_FOR_FLOATS; the CI workflows on this head are all action_required, so there is no CI signal.

* 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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thanks: that would have been a silent id change on reload. Done in the form you propose: the TEXT branch of parseSingleValue formats a BigDecimal through Double.toString(((BigDecimal) value).doubleValue()), i.e. exactly the string the Double path produced. testConvertDecimalFromJsonLine now has a TEXT case with your three literals plus an integer: 1.50 -> "1.5", 1e-7 -> "1.0E-7", 12345678901.0 -> "1.2345678901E10", 7 -> "7". The comment above the flag now speaks about all target types.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 This comment says a DECIMAL value is "sent and received as a plain decimal string in JSON". a57545f 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".

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: "sent as a plain JSON number (BigDecimal.toPlainString(), every digit), returned by the server as a plain decimal string; new BigDecimal(String) restores it exactly".

… DECIMAL wire format

Review round 2 of the DECIMAL companion:

- The loader parses JSON fractions as BigDecimal since a57545f; a
  fraction loaded into a TEXT key printed differently ("1.50" instead of
  "1.5"), which would change primary-key ids on a reload. The TEXT branch
  now formats a BigDecimal through Double.toString, the string the Double
  path produced; DataTypeUtilTest pins 1.50, 1e-7, 12345678901.0 and an
  integer.
- The public DataType.DECIMAL comment describes the actual wire format:
  sent as a plain JSON number, returned by the server as a plain decimal
  string.
@SebastianGruza

Copy link
Copy Markdown
Author

Round 2 in 56fcad0: a JSON fraction loaded into a TEXT key yields the same string as before the BigDecimal parsing (test with four literals), and the DataType.DECIMAL comment describes the actual wire format. Loader unit 14/14, client unit 78/78, checkstyle clean.

@bitflicker64 bitflicker64 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: no. Summary: The DECIMAL enum entry, asDecimal(), the plain-number serializer, the 128/128 bounds and the direct-serializer layout match apache/hugegraph#3209 at 0146849b, and the round-2 changes answer the earlier comments. One regression came in with round 2: the TEXT branch of DataTypeUtil.parseSingleValue now formats every BigDecimal through double, and JDBC sources hand BigDecimal to it for every DECIMAL/NUMERIC column, so existing JDBC-to-TEXT loads store different strings and PK-derived vertex ids change. Evidence: git diff 3b385c3d..56fcad0e over the exact head; JDBCFetcher and RowFetcher pass ResultSet.getObject(i) through unchanged; a JDK run of Double.toString(new BigDecimal(s).doubleValue()) gives 12345 -> 12345.0, 12.50 -> 12.5, 12345678901234567890.12 -> 1.2345678901234567E19, where the old value.toString() kept the input; every workflow run on this head is action_required, so there is no CI signal.

// JSON fractions are parsed as BigDecimal (see JsonUtil);
// a TEXT key keeps the string the double path produced
// ("1.5", not "1.50"), so existing ids do not change
return Double.toString(((BigDecimal) value).doubleValue());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ This branch runs for every BigDecimal, not only for the ones the JSON parser now produces. JDBC sources are the common other producer: JDBCFetcher and RowFetcher put ResultSet.getObject(i) straight into the line, and MySQL/PostgreSQL DECIMAL/NUMERIC and every Oracle NUMBER column come back as BigDecimal. Before this PR those values reached the value instanceof Number check below and kept value.toString().

Output for a TEXT key, old vs new (checked on a JDK):

JDBC value before after
12345 (Oracle NUMBER(10)) 12345 12345.0
12.50 (DECIMAL(10,2)) 12.50 12.5
12345678901234567890.12 12345678901234567890.12 1.2345678901234567E19

So a JDBC column mapped to a TEXT key now stores different strings, the last one lossy, and when that key is a primary key a reload creates new vertex ids next to the old ones. This is the same silent id change the round-2 fix was meant to prevent for JSON, moved to JDBC.

Please apply the double formatting only when the value came from the JSON parser, for example when source is a FileSource/HDFSSource or KafkaSource whose format() is JSON, and keep value.toString() for everything else. A DataTypeUtilTest case that converts new BigDecimal("12.50") for a TEXT key with a JDBCSource and expects "12.50" would pin it.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thanks: I moved the same problem from JSON to JDBC. Done the way you suggest: the TEXT branch formats a BigDecimal through double only when the value came from the JSON parser (FileSource/HDFSSource with format() == JSON, or KafkaSource with getFormat() == JSON, helper fromJsonParser); every other BigDecimal, including JDBC, keeps value.toString() as before this PR. DataTypeUtilTest.testConvertJdbcDecimalToTextKeepsItsText converts your three values from a JDBCSource: 12.50 -> "12.50", 12345 -> "12345", 12345678901234567890.12 unchanged; the JSON case from round 2 now uses a source with format: JSON explicitly.

@codecov

codecov Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.37500% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 46.42%. Comparing base (b066b80) to head (5255714).
⚠️ Report is 244 commits behind head on master.

Files with missing lines Patch % Lines
...e/hugegraph/serializer/direct/struct/DataType.java 65.38% 5 Missing and 4 partials ⚠️
...pache/hugegraph/loader/builder/ElementBuilder.java 89.65% 2 Missing and 1 partial ⚠️
...org/apache/hugegraph/loader/util/DataTypeUtil.java 95.74% 0 Missing and 2 partials ⚠️
...che/hugegraph/serializer/BigDecimalSerializer.java 87.50% 0 Missing and 1 partial ⚠️
.../apache/hugegraph/structure/constant/DataType.java 96.00% 0 Missing and 1 partial ⚠️
...raph/service/schema/GroovySchemaCompatibility.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             master     #771       +/-   ##
=============================================
- Coverage     62.49%   46.42%   -16.08%     
- Complexity     1903     4485     +2582     
=============================================
  Files           262      604      +342     
  Lines          9541    29509    +19968     
  Branches        886     3464     +2578     
=============================================
+ Hits           5963    13699     +7736     
- Misses         3190    14464    +11274     
- Partials        388     1346      +958     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…hen it came from the JSON parser

Review round 3: JDBC sources hand BigDecimal for every DECIMAL/NUMERIC
column, and the round-2 TEXT branch formatted those through double too
(12.50 -> 12.5, long values lossy), changing primary-key ids on reload.
The double formatting now applies only to sources parsed as JSON
(FileSource/HDFSSource and KafkaSource with format JSON); any other
BigDecimal keeps its own text. DataTypeUtilTest pins the JDBC case.
@SebastianGruza

Copy link
Copy Markdown
Author

Round 3 in 268af65: the double formatting of a BigDecimal for a TEXT key applies only to sources parsed as JSON; JDBC values keep their own text (test with a JDBCSource). Loader unit 15/15, checkstyle clean. A note on codecov: codecov/project shows a drop from 62% to 46%, but that is the set of jobs that uploaded a report on this head versus the base, not a coverage change in this PR; the patch is at 85%.

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: yes. Summary: DECIMAL bounds can be bypassed and plain JSON serialization can exhaust client memory; loader numeric null matching also changes. Evidence: static trace through DataType bounds, REST serialization, loader mapping/conversion, and API test setup.

@Override
public void serialize(BigDecimal value, JsonGenerator generator,
SerializerProvider provider) throws IOException {
generator.writeNumber(value.toPlainString());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ This shared request serializer materializes value.toPlainString() for every BigDecimal without checking the DECIMAL bounds or output length. new BigDecimal("1E+999999999") needs roughly a billion plain characters and can exhaust the client heap before the server rejects it. Use a bounded or scientific JSON representation, or reject out-of-range values before expansion.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. exactString(): the plain form only while the value is within the DECIMAL bounds (128 digits, scale within ±128), toString() beyond them, i.e. the scientific form: still an exact JSON number, and 1E+999999999 stays eleven characters for the server to reject on its side. The same rule as HugeGraphSONModule in apache/hugegraph#3209. testSerializerDoesNotExpandOutOfBoundsValues: 1E+3 -> 1000, 1E-128 plain, 1E+999999999 and 1E-129 scientific, through JsonUtil.toJson.

}

public static BigDecimal checkDecimalBounds(BigDecimal decimal) {
int scale = Math.abs(decimal.scale());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Math.abs(Integer.MIN_VALUE) remains negative, so this check accepts new BigDecimal(BigInteger.ONE, Integer.MIN_VALUE); the direct BytesBuffer.writeProperty path can then persist a DECIMAL outside the stated ±128 scale bound. Compare scale directly to both limits here and in the duplicate check in serializer/direct/struct/DataType.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, the same slip I fixed on the server side in round 3. Both copies compare the scale to both limits directly. testDecimalBounds checks new BigDecimal(BigInteger.ONE, Integer.MIN_VALUE) and MAX_VALUE on both copies and on BytesBuffer.writeProperty.

return values;
if (values instanceof Collection) {
// JSON file should not parse again
if (checkCollectionDataType(key, (Collection<?>) values, dataType)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ This new collection fast path returns an already typed BigDecimal list, but the DECIMAL checkDataType branch only checks the class and never applies the 128/128 bound used by scalar parseDecimal. Thus [1E+129] passes loader conversion while the same scalar is rejected. Validate every DECIMAL element before returning the collection.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. The DECIMAL branch of checkDataType counts an element as already typed only when it is a BigDecimal within the bounds; otherwise the list goes through the element-by-element conversion and parseDecimal rejects it like the scalar. testConvertDecimalList: [1E+129] and [1E+129, 2] rejected.

* 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);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ This mapper change also parses numeric null_values configuration and JSON rows as BigDecimal. ElementBuilder.retainField() uses Set.contains, so null_values: [1.0] no longer matches a JSON field 1.00 (BigDecimal.equals is scale-sensitive), although the previous Double values matched. Normalize numeric null sentinels or compare them numerically.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. ElementBuilder.isNullValue: contains first, then for numbers a comparison by value through BigDecimal.compareTo (both sides are BigDecimal now from the same mapper; Long/Double values from JDBC or CSV match too). New NullValuesTest in the loader UnitTestSuite: a mapping with null_values: [1.0, ""] parsed through JsonUtil, a JSON row 1.00 matches, 1.5 does not, "" matches, 1L and 1.0d match, "1.0" does not.

try {
propertyKeyAPI.create(amount);
} catch (ServerException e) {
Assume.assumeTrue("The server has no DECIMAL data type: " +

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 This assumption catches every ServerException while creating the DECIMAL key and skips the whole class, including failures from a server that supports DECIMAL. A regression or unrelated server error can therefore make the API tests appear skipped. Skip only the known unsupported-type response and rethrow other errors.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: the skip applies only when the message is the enum rejection during deserialization (DataType, "DECIMAL", "not one of the values accepted", which is exactly what 1.7.0 returns); any other ServerException propagates. Checked against a 1.7.0 server on the lab: 1 skipped with that message.

…ctly, check list elements, match numeric null values by value

- BigDecimalSerializer writes the plain form only within the DECIMAL bounds
  (128 digits, scale within +-128) and the scientific form beyond them, so a
  value such as 1E+999999999 is not expanded on the client before the server
  rejects it.
- checkDecimalBounds compares the scale to both limits directly in both
  DataType copies: Math.abs(Integer.MIN_VALUE) stays negative.
- Loader: a BigDecimal list element counts as already typed only within the
  bounds; otherwise it goes through parseDecimal and is rejected like the
  scalar.
- Loader: numeric null_values match a field by value (BigDecimal.equals is
  scale sensitive; both sides are BigDecimal since the mapper change).
- DecimalPropertyApiTest skips only on the unsupported-enum response of a
  server without the type and rethrows anything else.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@SebastianGruza

Copy link
Copy Markdown
Author

Round 4 in 4ca9ce2: the plain form only within the DECIMAL bounds (scientific beyond them, no expansion on the client), the scale compared directly in both copies, DECIMAL list elements checked against the bounds, numeric null_values matched by value, the API test skips only on the response of a server without the type. Client unit 7/7, loader UnitTestSuite 16/16, DecimalPropertyApiTest 1 skipped on 1.7.0 and 6/6 against a server built from apache/hugegraph#3209 head 0bb7724a; checkstyle clean. Docs: apache/hugegraph-doc#504.

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: yes. Summary: Existing loader mappings for JSON numeric values can stop matching after the parser preserves decimal scale, changing mapped properties or IDs; the inline comments cover additional conversion edge cases. Evidence: Mapping keys are built with String.valueOf(value) and looked up by exact string key.

* 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);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Blocking: yes. Summary: This changes JSON fractions from Double to scale-preserving BigDecimal, so a value like 1.00 no longer matches an existing mapping key "1.0"; preserve the prior canonical string for mapping lookup. Evidence: ElementBuilder.mappingValue() uses String.valueOf(fieldValue), then ElementMapping.mappingValue() performs an exact Map.get(rawValue).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, the same effect as with TEXT keys in round 2, and it gets the same recipe. DataTypeUtil.mappingKey(value, source): for a BigDecimal from a source parsed as JSON the lookup key is the string the double path produced (1.00 -> "1.0", 1e-7 -> "1.0E-7"), any other value goes through String.valueOf as before; ElementBuilder.mappingValue uses the helper. Test NullValuesTest.testJsonFractionMappingKey: a JSON row with 1.00, 1e-7, -0.0, 7, "1.00", and the same BigDecimal from a CSV/JDBC source keeping its own text.

// ("1.5", not "1.50"), so existing ids do not change.
// A BigDecimal from any other source (JDBC DECIMAL/NUMERIC)
// keeps its own text, as before
return Double.toString(((BigDecimal) value).doubleValue());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Blocking: no. Summary: JSON -0.0 now becomes TEXT "0.0", which can change an existing text value or ID; preserve signed zero when converting JSON BigDecimal values. Evidence: USE_BIG_DECIMAL_FOR_FLOATS loses the negative-zero sign and this branch converts through doubleValue() before Double.toString().

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. JsonValueDeser (registered for Object.class on the loader's mapper, extends UntypedObjectDeserializer.Vanilla) returns fractions as BigDecimal but -0.0 as Double -0.0, since BigDecimal cannot carry the sign of zero; a TEXT key and an id keep "-0.0", a DECIMAL key gets zero. Covered in testConvertDecimalFromJsonLine.

return new BigDecimal((BigInteger) number);
}
if (number instanceof Double || number instanceof Float) {
return new BigDecimal(number.toString());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Blocking: no. Summary: A JDBC Float/Double NaN or Infinity throws during null-value filtering when it is not itself listed as null; handle non-finite numbers before constructing BigDecimal. Evidence: retainField() calls this conversion for nullable keys, and new BigDecimal(number.toString()) rejects those values.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. toBigDecimal returns null for NaN and the infinities and isNullValue leaves them to the plain contains (equal only to themselves). Tests in NullValuesTest.

}
E.checkArgument(decimal != null,
"Failed to convert value(key='%s') '%s'(%s) to Decimal",
key, rawValue, rawValue.getClass());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Blocking: no. Summary: A null element in a DECIMAL collection raises NullPointerException instead of a conversion error; reject null explicitly or build the error without dereferencing it. Evidence: valueToDecimal(null) returns null, then this error argument evaluates rawValue.getClass().

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: parseDecimal rejects a null element with the conversion error instead of an NPE; test in testConvertDecimalList.

…e non-finite numbers and null decimal elements

- JsonValueDeser: untyped JSON fractions stay BigDecimal except a negative
  zero, which BigDecimal cannot carry; it stays a Double so TEXT values and
  ids keep "-0.0".
- DataTypeUtil.mappingKey: a JSON fraction looks up the mapping by the
  string the double path produced ("1.0" for 1.00), so existing mappings
  keep matching; any other BigDecimal keeps its own text.
- isNullValue: a NaN or infinity from JDBC is not converted to BigDecimal
  (equal only to itself through contains()).
- parseDecimal rejects a null element with a conversion error.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@SebastianGruza

Copy link
Copy Markdown
Author

Round 5 in 991a97f: the mapping lookup key of a JSON fraction is the string it had before BigDecimal (as with TEXT keys), a negative zero keeps its sign through a custom untyped deserializer, NaN/Infinity from JDBC no longer break the null-value filter, a null element in a DECIMAL list is a conversion error. Loader UnitTestSuite 17/17, checkstyle clean, DecimalPropertyApiTest 6/6 against a server built from apache/hugegraph#3209 d7eb80c8.

@bitflicker64 bitflicker64 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: yes. Summary: The round 5 mapping-key change makes the loader round a JSON decimal through double whenever the struct has any value_mapping, so a DECIMAL column loses digits without an error. Vertex ids built from a JSON fraction with CUSTOMIZE_STRING also change from the old Double text to the BigDecimal text. The client side (code 12, asDecimal(), the plain-number serializer, the 128/128 bounds, and the direct-serializer layout) matches apache/hugegraph#3209 at d7eb80c8. Evidence: full diff 3b385c3..991a97f. ElementMapping.mappingValue() returns the lookup string when no entry matches, and ElementBuilder.customizeId() and olapVertexId() call idValue.toString(). A jshell run with Jackson 2.12.3 and USE_BIG_DECIMAL_FOR_FLOATS gives 12345678901234567890.10, then mapping key 1.2345678901234567E19, then 12345678901234567000 at the DECIMAL key. BigDecimal 1.50 and 1e2 print as 1.50 and 1E+2, where Double printed 1.5 and 100.0.

return fieldValue;
}
String fieldStrValue = String.valueOf(fieldValue);
String fieldStrValue = DataTypeUtil.mappingKey(fieldValue, this.struct.input());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important: With any value_mapping in the struct, this line now sends a DECIMAL column through double, and the column loses digits without an error.

ElementMapping.mappingValue() starts from mappingValue = rawValue and replaces it only on a match. When nothing matches, the result is the lookup string itself. mappingKey() now builds that string with Double.toString(value.doubleValue()). Every property field goes through mappingValue() (lines 539, 611, 702 and others), including fields that have no mapping entry. So a struct that maps only city still turns amount into a rounded string. parseDecimal then accepts that string as a valid decimal.

I checked this in jshell with Jackson 2.12.3 and USE_BIG_DECIMAL_FOR_FLOATS:

JSON literal key from mappingKey value at the DECIMAL key
12345678901234567890.10 1.2345678901234567E19 12345678901234567000
0.123456789012345678 0.12345678901234568 0.12345678901234568

Before round 5 this path used String.valueOf, which kept every digit of a BigDecimal. NullValuesTest.testJsonFractionMappingKey checks the lookup key only. No test sends a mapped struct into a DECIMAL key.

Requested change: use the double text only to look up the map. When no entry matches, return the original fieldValue, not the lookup string. Please add a test with a value_mapping on another field and a 20-digit JSON decimal loaded into a DECIMAL key.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, thanks for the jshell table. The double text is now used only for the lookup: ElementMapping.mappedValue() returns null when no entry matches, and DataTypeUtil.mapValue() then returns the original field value itself (an identity check, not the lookup string). Test testMappedStructKeepsUnmappedDecimal: a struct with a value_mapping on city and a 20-digit JSON decimal loaded into a DECIMAL key, expected exactly 12345678901234567890.10.

/**
* The string a JSON fraction had before it was read as BigDecimal: the
* shortest double representation ("1.5" for 1.50, "1.0E-7" for 1e-7).
* TEXT values and mapping lookups keep the keys they always had.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important: TEXT values and mapping keys now keep their old strings, but vertex ids built from a JSON fraction still change.

ElementBuilder.customizeId() builds a CUSTOMIZE_STRING id with idValue.toString() (line 354). olapVertexId() does the same for CUSTOMIZE_STRING and PRIMARY_KEY (line 840). Edge endpoints go through customizeId() too, because EdgeBuilder builds source and target through VertexKVPairs. With no value_mapping, idValue is the raw BigDecimal from JsonValueDeser, so it never passes through jsonNumberText.

JSON id literal id before this PR (Double) id at this head (BigDecimal)
1.50 1.5 1.50
1e2 100.0 1E+2
1e-7 1.0E-7 1E-7

A reload of an existing JSON file creates new vertices next to the old ones, and edges point at ids that no longer match. Integer literals are not affected because they still parse as Integer or Long.

Requested change: format a JSON BigDecimal with jsonNumberText wherever the loader turns a field value into a string id. That covers customizeId() and olapVertexId(), and splitField() for flat ids. Please add a test that loads a CUSTOMIZE_STRING id of 1.50 from a JSON row and expects 1.5.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. New DataTypeUtil.idText(value, source) formats a JSON BigDecimal through jsonNumberText; customizeId(), olapVertexId() and splitField() use it. Test testJsonFractionIdText: a CUSTOMIZE_STRING id of 1.50 from a JSON row gives 1.5, 1e2 gives 100.0, as before the PR.

… string ids from JSON fractions keep the double text

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@SebastianGruza

Copy link
Copy Markdown
Author

Round 6 in 5255714: a mapping lookup never replaces an unmapped value (the double text is used only to find the entry), and string ids built from JSON fractions keep the former double text. Loader 19/19, checkstyle clean. Against server 371275c02 on the lab, DecimalPropertyApiTest passes 6/6 on rocksdb and on hstore.

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: yes. Summary: Numeric mapping outputs can change existing text values and IDs, and the serializer rejects the server-accepted 1E+128 boundary after plain expansion. Evidence: exact-head review of DataTypeUtil, JsonValueDeser, ElementMapping, BigDecimalSerializer, and apache/hugegraph#3209 PropertiesDeserializer/DataType bounds.

} else if (dataType.isDecimal()) {
return parseDecimal(key, value);
} else if (dataType.isText()) {
if (value instanceof BigDecimal && fromJsonParser(source)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Blocking: yes. Summary: Fractional replacement values in value_mapping are parsed from the JSON struct as BigDecimal, but this branch decides legacy text formatting from the row source. With CSV/JDBC input, a configured 1.50 therefore reaches TEXT conversion or idText() as "1.50" instead of the previous "1.5", changing existing text values and CUSTOMIZE_STRING IDs. Evidence: JsonValueDeser returns BigDecimal, ElementMapping.mappedValue() returns the configured object, and fromJsonParser(source) is false for non-JSON row sources. Please preserve legacy numeric text for mapped TEXT/ID outputs while keeping DECIMAL outputs exact.

int scale = value.scale();
if (value.precision() <= DataType.DECIMAL_MAX_PRECISION &&
scale >= -DataType.DECIMAL_MAX_SCALE && scale <= DataType.DECIMAL_MAX_SCALE) {
return value.toPlainString();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Blocking: yes. Summary: 1E+128 passes this bound check (precision 1, scale -128), but toPlainString() emits a 129-digit integer; the server reads integer tokens as BigInteger and rejects precision 129. Evidence: the client DataType.valueToDecimal() accepts 1E+128, while apache/hugegraph#3209 PropertiesDeserializer uses getNumberValue() for VALUE_NUMBER_INT and server DataType.checkDecimalBounds() enforces the 128-digit cap. Please preserve exponent notation whenever plain expansion exceeds the server precision limit and add an upper-bound REST serialization test.

@bitflicker64 bitflicker64 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: yes. Summary: The round-6 mapValue change returns unmapped field values in their raw type instead of the string the loader used before, so a JSON boolean or JDBC timestamp loaded into a TEXT key fails whenever the struct has any value_mapping. The two open findings from imbajin at this head (mapped replacement values as BigDecimal, and the 1E+128 plain expansion) also reproduce statically and are not repeated here. Evidence: diff of ElementBuilder.mappingValue against 3b385c3, a temporary loader unit test at 5255714 (JDK 11, mvn -o -pl hugegraph-loader -am test) showing the TEXT conversion error, and apache/hugegraph#3209 at 2034d240 for the server side.

return fieldValue;
}
Object mapped = mapping.mappedValue(fieldName, mappingKey(fieldValue, source), caseSensitive);
return mapped != null ? mapped : fieldValue;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important: An unmapped field in a struct that has any value_mapping now keeps its raw type, so loads that worked before fail on TEXT keys.

Before this PR, ElementBuilder.mappingValue() called String.valueOf(fieldValue) and ElementMapping.mappingValue() returned that string when no entry matched. So once a struct had a value_mapping on any field, every other field reached DataTypeUtil.convert as a String. Here the unmapped case returns the original object. For a value that is neither a String nor a Number, such as a JSON true or a JDBC java.sql.Timestamp, the TEXT branch of parseSingleValue does not match and checkDataType rejects it.

I ran this at 5255714 with a struct that maps only city and a JSON row {"city":1,"active":true}, converting active for a TEXT key:

  • mapValue returns Boolean true
  • DataTypeUtil.convert throws The value(key='active') 'true'(class java.lang.Boolean) is not match with data type TEXT and can't convert to it
  • the pre-PR input String.valueOf(true) converts to "true"

An existing mapping that loads booleans or JDBC timestamps into TEXT keys next to a value_mapping stops loading after the upgrade.

Requested change: keep the raw object only where the round-6 fix needs it (a BigDecimal, so a DECIMAL column keeps every digit), and return String.valueOf(fieldValue) for every other unmapped value as before. For example: if (mapped != null) return mapped; return fieldValue instanceof BigDecimal ? fieldValue : String.valueOf(fieldValue);. Please add a NullValuesTest case that converts an unmapped JSON boolean to a TEXT key in a struct that has a value_mapping.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

client hugegraph-client hubble hugegraph-hubble loader hugegraph-loader spark

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants