bitflicker64 commented on code in PR #771:
URL: 
https://github.com/apache/hugegraph-toolchain/pull/771#discussion_r4175879282


##########
hugegraph-loader/src/main/java/org/apache/hugegraph/loader/util/DataTypeUtil.java:
##########
@@ -328,6 +354,86 @@ 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;
+        }
+    }
+
+    /**
+     * 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.
+     */
+    public static String jsonNumberText(BigDecimal value) {
+        return Double.toString(value.doubleValue());
+    }
+
+    /** The lookup key of a field value in the mapping's value map. */
+    public static String mappingKey(Object fieldValue, InputSource source) {
+        if (fieldValue instanceof BigDecimal && fromJsonParser(source)) {
+            return jsonNumberText((BigDecimal) fieldValue);
+        }
+        return String.valueOf(fieldValue);
+    }
+
+    /**
+     * The mapped value when the mapping has an entry for this field value,
+     * else the original value: the lookup key is only a key, so a field
+     * without a mapping entry (e.g. a DECIMAL column of a struct that maps
+     * another field) keeps every digit.
+     */
+    public static Object mapValue(ElementMapping mapping, String fieldName, 
Object fieldValue,
+                                  InputSource source, boolean caseSensitive) {
+        if (mapping.mappingValues().isEmpty()) {
+            return fieldValue;
+        }
+        Object mapped = mapping.mappedValue(fieldName, mappingKey(fieldValue, 
source), caseSensitive);
+        return mapped != null ? mapped : fieldValue;

Review Comment:
   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 5255714c 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`.



-- 
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]

Reply via email to