imbajin commented on code in PR #771:
URL:
https://github.com/apache/hugegraph-toolchain/pull/771#discussion_r4150239515
##########
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:
⚠️ 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).
##########
hugegraph-loader/src/main/java/org/apache/hugegraph/loader/util/DataTypeUtil.java:
##########
@@ -200,7 +202,17 @@ private static Object parseSingleValue(String key, Object
rawValue,
}
} else if (dataType.isUUID()) {
return parseUUID(key, value);
+ } else if (dataType.isDecimal()) {
+ return parseDecimal(key, value);
} else if (dataType.isText()) {
+ if (value instanceof BigDecimal && fromJsonParser(source)) {
+ // 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.
+ // A BigDecimal from any other source (JDBC DECIMAL/NUMERIC)
+ // keeps its own text, as before
+ return Double.toString(((BigDecimal) value).doubleValue());
Review Comment:
⚠️ 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().
##########
hugegraph-loader/src/main/java/org/apache/hugegraph/loader/builder/ElementBuilder.java:
##########
@@ -212,7 +214,42 @@ protected boolean retainField(String fieldName, Object
fieldValue) {
}
return !nullableKeys.contains(mappedKey) ||
- !nullValues.contains(fieldValue);
+ !isNullValue(nullValues, fieldValue);
+ }
+
+ /**
+ * Numbers are matched by value: JSON rows and the mapping's null_values
+ * are both parsed as BigDecimal now, whose equals() is scale-sensitive
+ * (1.0 vs 1.00); before, both were Double and matched.
+ */
+ public static boolean isNullValue(Set<Object> nullValues, Object
fieldValue) {
+ if (nullValues.contains(fieldValue)) {
+ return true;
+ }
+ if (!(fieldValue instanceof Number)) {
+ return false;
+ }
+ BigDecimal value = toBigDecimal((Number) fieldValue);
+ for (Object nullValue : nullValues) {
+ if (nullValue instanceof Number &&
+ value.compareTo(toBigDecimal((Number) nullValue)) == 0) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ private static BigDecimal toBigDecimal(Number number) {
+ if (number instanceof BigDecimal) {
+ return (BigDecimal) number;
+ }
+ if (number instanceof BigInteger) {
+ return new BigDecimal((BigInteger) number);
+ }
+ if (number instanceof Double || number instanceof Float) {
+ return new BigDecimal(number.toString());
Review Comment:
⚠️ 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.
##########
hugegraph-loader/src/main/java/org/apache/hugegraph/loader/util/DataTypeUtil.java:
##########
@@ -328,6 +353,40 @@ private static Number parseNumber(String key, Object value,
}
}
+ private static boolean fromJsonParser(InputSource source) {
+ if (source instanceof FileSource) {
+ return ((FileSource) source).format() == FileFormat.JSON;
+ }
+ if (source instanceof KafkaSource) {
+ return ((KafkaSource) source).getFormat() == FileFormat.JSON;
+ }
+ return false;
+ }
+
+ private static boolean withinDecimalBounds(BigDecimal decimal) {
+ try {
+ DataType.checkDecimalBounds(decimal);
+ return true;
+ } catch (IllegalArgumentException e) {
+ return false;
+ }
+ }
+
+ private static BigDecimal parseDecimal(String key, Object rawValue) {
+ BigDecimal decimal;
+ try {
+ decimal = DataType.DECIMAL.valueToDecimal(rawValue);
+ } catch (IllegalArgumentException e) {
+ throw new IllegalArgumentException(String.format(
+ "Failed to convert value(key='%s') '%s'(%s) to Decimal",
+ key, rawValue, rawValue.getClass()), e);
+ }
+ E.checkArgument(decimal != null,
+ "Failed to convert value(key='%s') '%s'(%s) to
Decimal",
+ key, rawValue, rawValue.getClass());
Review Comment:
🧹 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().
--
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]
