SebastianGruza commented on code in PR #771:
URL:
https://github.com/apache/hugegraph-toolchain/pull/771#discussion_r4154958624
##########
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:
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.
##########
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:
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`.
##########
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:
Right. `toBigDecimal` returns null for NaN and the infinities and
`isNullValue` leaves them to the plain `contains` (equal only to themselves).
Tests in `NullValuesTest`.
##########
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:
Done: `parseDecimal` rejects a null element with the conversion error
instead of an NPE; test in `testConvertDecimalList`.
--
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]
