xiangfu0 commented on code in PR #19999:
URL: https://github.com/apache/pinot/pull/19999#discussion_r4343866340
##########
pinot-plugins/pinot-input-format/pinot-parquet/src/main/java/org/apache/pinot/plugin/inputformat/parquet/ParquetAvroRecordReader.java:
##########
@@ -59,7 +59,7 @@ public void init(File dataFile, @Nullable Set<String>
fieldsToRead, @Nullable Re
extractorConfig.setExtractRawTimeValues(
((ParquetRecordReaderConfig)
recordReaderConfig).isExtractRawTimeValues());
}
- _recordExtractor = new ParquetAvroRecordExtractor();
+ _recordExtractor = new
ParquetAvroRecordExtractor(ParquetUtils.getUnsignedInt32FieldNames(_dataFilePath));
Review Comment:
Minor: this opens the Parquet file once more at init, on top of
`hasAvroSchemaInFileMetadata` and the Avro reader open. Fine for a one-time
init cost, but if you want to avoid the extra round-trip you could pull the
field names off the schema the reader already reads. Not blocking.
##########
pinot-plugins/pinot-input-format/pinot-parquet/src/main/java/org/apache/pinot/plugin/inputformat/parquet/ParquetAvroRecordExtractor.java:
##########
@@ -18,19 +18,50 @@
*/
package org.apache.pinot.plugin.inputformat.parquet;
+import java.util.Set;
import org.apache.avro.Schema;
import org.apache.avro.generic.GenericFixed;
+import org.apache.avro.generic.GenericRecord;
import org.apache.pinot.plugin.inputformat.avro.AvroRecordExtractor;
+import org.apache.pinot.spi.data.readers.GenericRow;
import org.apache.pinot.spi.utils.TimestampUtils;
-/// The type matrix is inherited from [AvroRecordExtractor]; the only override
is the INT96 timestamp
-/// (which parquet-avro surfaces as `fixed(12)` with `doc = "INT96 represented
as byte[12]"`) → `Timestamp`
-/// (or `Long` epoch nanos when `extractRawTimeValues` is `true`) via
[ParquetUtils#convertInt96ToEpochNanos].
+/// The type matrix is inherited from [AvroRecordExtractor], with two
overrides:
+/// - INT96 timestamp (which parquet-avro surfaces as `fixed(12)` with `doc =
"INT96 represented as byte[12]"`) →
+/// `Timestamp` (or `Long` epoch nanos when `extractRawTimeValues` is
`true`) via
+/// [ParquetUtils#convertInt96ToEpochNanos].
+/// - Top-level unsigned 32-bit integer fields (`INTEGER(32, false)` / legacy
`UINT_32`), which parquet-avro surfaces
+/// as a signed Avro `int` → `Long` via [Integer#toUnsignedLong]. The field
names come from the Parquet file schema
+/// (see [ParquetUtils#getUnsignedInt32FieldNames]) since the Avro schema
carries no signedness. Unsigned values
+/// nested in a struct, list or map are still read as signed ints by this
reader; [ParquetNativeRecordReader]
+/// widens them at every level.
public class ParquetAvroRecordExtractor extends AvroRecordExtractor {
private static final int INT96_BYTE_SIZE = 12;
private static final String INT96_DOC = "INT96 represented as byte[12]";
+ private final Set<String> _unsignedInt32Fields;
+
+ public ParquetAvroRecordExtractor() {
+ this(Set.of());
+ }
+
+ public ParquetAvroRecordExtractor(Set<String> unsignedInt32Fields) {
+ _unsignedInt32Fields = unsignedInt32Fields;
+ }
+
+ @Override
+ public GenericRow extract(GenericRecord from, GenericRow to) {
+ super.extract(from, to);
+ for (String fieldName : _unsignedInt32Fields) {
+ Object value = to.getValue(fieldName);
+ if (value instanceof Integer) {
+ to.putValue(fieldName, Integer.toUnsignedLong((Integer) value));
Review Comment:
This widens only the top-level fields, so on the Avro reader an unsigned
value nested in a struct/list/map still comes back as a negative signed int.
You've documented it, which is good — my only worry is that it's silent: a user
whose upsert/dedup/join key lives in a nested unsigned column gets wrong values
with no error. Worth calling out in the user-facing Parquet docs (and maybe
steering such cases to the native reader) so it isn't discovered in production.
--
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]
