Jackie-Jiang commented on code in PR #18976:
URL: https://github.com/apache/pinot/pull/18976#discussion_r4383602578
##########
pinot-plugins/pinot-input-format/pinot-json/src/main/java/org/apache/pinot/plugin/inputformat/json/format/JacksonPayloadParser.java:
##########
@@ -109,17 +115,24 @@ public boolean parse(byte[] payload, int offset, int
length, GenericRow destinat
/// Materializes the value at the parser's current token in
[JSONRecordExtractor]'s converted shape. Scalars
/// are read straight off the parser rather than through databind, which
allocates a fresh
/// `DeserializationContext` per `readValue` call. `getNumberValue()` keeps
the binary formats' `Float`
- /// (never upcast to `Double`) and text JSON's `Double`, matching Jackson's
untyped materialization.
+ /// (never upcast to `Double`). Text-backed formats use `getDecimalValue()`
for floats so decimal literals
+ /// are not rounded to `Double` before Pinot's schema conversion.
@Nullable
private Object readValue(JsonParser parser, JsonToken valueToken)
throws IOException {
switch (valueToken) {
case VALUE_STRING:
return parser.getText();
case VALUE_NUMBER_INT:
- case VALUE_NUMBER_FLOAT:
// Oversized ints widen to BigDecimal via the shared contract (Pinot
has no BigInteger type)
return JSONRecordExtractor.convert(parser.getNumberValue());
+ case VALUE_NUMBER_FLOAT:
+ // JSON text stores decimals as decimal *text*. Jackson's default
getNumberValue() rounds that text
+ // to Double. Avro and Protobuf extractors do not have this problem:
they carry typed numeric
+ // values (Avro's decimal logical type is already BigDecimal). Binary
JSON formats encode IEEE
+ // floats natively, so they keep getNumberValue().
+ return JSONRecordExtractor.convert(
+ _preserveDecimalFloats ? parser.getDecimalValue() :
parser.getNumberValue());
Review Comment:
[P1] Preserve false when converting decimal zero to BOOLEAN
For a BOOLEAN schema column, `{"flag":0.0}` previously decoded to `Double`
and converted to false. This now produces `BigDecimal("0.0")`, but
`PinotDataType.BIG_DECIMAL.toBoolean()` uses `!value.equals(BigDecimal.ZERO)`.
Because `BigDecimal.equals` compares scale, the value is silently stored as
true. Batch ingestion is affected as well.
Please change the BigDecimal boolean conversion to numerical zero comparison
(`signum() != 0` or `compareTo(BigDecimal.ZERO) != 0`) alongside this change,
and cover `0.0`, `-0.0`, and exponent-form zero through decoding and schema
conversion.
##########
pinot-plugins/pinot-input-format/pinot-json/src/main/java/org/apache/pinot/plugin/inputformat/json/JSONRecordReader.java:
##########
@@ -49,7 +49,7 @@ private void init()
throws IOException {
_inputStream = RecordReaderUtils.getBufferedInputStream(_dataFile);
try {
- _iterator = JsonUtils.DEFAULT_READER.forType(new
TypeReference<Map<String, Object>>() {
+ _iterator = JsonUtils.READER_WITH_BIG_DECIMAL.forType(new
TypeReference<Map<String, Object>>() {
}).readValues(_inputStream);
Review Comment:
[P1] Preserve mixed numeric arrays when converting to MV STRING
An input such as `{"v":[1.5,2]}` previously ingested into a multi-value
STRING column as `["1.5","2"]`. With this reader, the array contains a
BigDecimal followed by an Integer. `DataTypeTransformerUtils.transformValue()`
chooses `BIG_DECIMAL_ARRAY` from the first element, and
`PinotDataType.toStringArray()` applies `BIG_DECIMAL.toString()` to every
element. The cast of the Integer second element to BigDecimal throws
`ClassCastException`, rejecting the row or nulling the column depending on
error handling. The previous `DOUBLE.toString()` simply called
`value.toString()`, so this input worked. Streaming ingestion has the same
regression.
Please dispatch STRING-array conversion using each element's actual type,
preserving `toPlainString()` for BigDecimal elements, and add mixed-array
regression coverage.
##########
pinot-plugins/pinot-input-format/pinot-json/src/main/java/org/apache/pinot/plugin/inputformat/json/format/TextJsonPayloadParser.java:
##########
@@ -22,11 +22,12 @@
/// Parses UTF-8 text JSON, the historical
[org.apache.pinot.plugin.inputformat.json.JSONMessageDecoder]
-/// behavior. Uses the same Jackson map/value materialization contract as the
binary JSON parsers.
+/// format. Floating literals are `BigDecimal` so high-precision decimals
survive ingestion; this is
+/// always on because it is the correct behavior for decimal text, not an
opt-in.
class TextJsonPayloadParser extends JacksonPayloadParser {
TextJsonPayloadParser() {
- super(JsonUtils.DEFAULT_READER);
+ super(JsonUtils.READER_WITH_BIG_DECIMAL);
Review Comment:
[P1] Keep built-in nullIf transforms matching decimal values
For JSON `{"metric":1.5}`, an ingestion transform `nullIf(metric, 1.5)`
previously returned null. The source metric now becomes BigDecimal while the
expression literal remains Double. `FunctionInvoker` leaves the Object
arguments unchanged, and `ObjectFunctions.nullIf()` compares them using
`equals`, so it returns 1.5 instead of null. Expression transforms run before
schema conversion, so a DOUBLE target column does not prevent this regression.
Comparing source fields containing `1.5` and `1.50` also stops returning null
because BigDecimal equality includes scale.
Please make built-in `nullIf` compare numeric operands by value and add
ingestion-transform coverage before changing the default decoded type.
--
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]
