Jackie-Jiang commented on code in PR #18870:
URL: https://github.com/apache/pinot/pull/18870#discussion_r3553639832
##########
pinot-plugins/pinot-input-format/pinot-avro-base/src/main/java/org/apache/pinot/plugin/inputformat/avro/AvroSchemaUtil.java:
##########
@@ -79,7 +110,8 @@ public static boolean isPrimitiveType(Schema.Type avroType) {
public static ObjectNode toAvroSchemaJsonObject(FieldSpec fieldSpec) {
ObjectNode jsonSchema = JsonUtils.newObjectNode();
jsonSchema.put("name", fieldSpec.getName());
- switch (fieldSpec.getDataType().getStoredType()) {
+ DataType dataType = fieldSpec.getDataType();
+ switch (dataType.getStoredType()) {
Review Comment:
I feel this is a bug. We should use original type instead of storage type.
BOOLEAN and TIMESTAMP are not handled correctly as well. Worth a separate bugfix
##########
pinot-core/src/main/java/org/apache/pinot/core/segment/processing/genericrow/GenericRowDeserializer.java:
##########
@@ -164,6 +164,16 @@ public void deserialize(long offset, GenericRow buffer) {
multiValue[j] = new String(stringBytes, UTF_8);
}
break;
+ case BYTES:
Review Comment:
Suggest making a separate fix just for this and `BIG_DECIMAL` gap. It is a
bug fix orthogonal to UUID support
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/BytesDictionary.java:
##########
@@ -30,7 +30,6 @@
* Extension of {@link BaseImmutableDictionary} that implements immutable
dictionary for byte[] type.
*/
public class BytesDictionary extends BaseImmutableDictionary {
-
Review Comment:
(nit) Revert
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java:
##########
@@ -1447,8 +1472,13 @@ private int[] getSortedDocIdsWithRawForwardIndex(String
column, IndexContainer i
IntArrays.quickSort(docIds, (d1, d2) ->
forwardIndex.getString(d1).compareTo(forwardIndex.getString(d2)));
break;
case BYTES:
- IntArrays.quickSort(docIds,
- (d1, d2) -> ByteArray.compare(forwardIndex.getBytes(d1),
forwardIndex.getBytes(d2)));
+ if (dataType == DataType.UUID) {
+ IntArrays.quickSort(docIds,
+ (d1, d2) -> UuidUtils.compare(forwardIndex.getBytes(d1),
forwardIndex.getBytes(d2)));
+ } else {
+ IntArrays.quickSort(docIds,
+ (d1, d2) -> ByteArray.compare(forwardIndex.getBytes(d1),
forwardIndex.getBytes(d2)));
+ }
Review Comment:
The sorting for BYTES and UUID should be the same
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java:
##########
@@ -753,16 +747,39 @@ private DedupRecordInfo getDedupRecordInfo(GenericRow
row) {
}
private RecordInfo getRecordInfo(GenericRow row, int docId) {
- PrimaryKey primaryKey = row.getPrimaryKey(_schema.getPrimaryKeyColumns());
+ PrimaryKey primaryKey = getPrimaryKey(row);
Comparable comparisonValue = getComparisonValue(row);
boolean deleteRecord = _deleteRecordColumn != null &&
BooleanUtils.toBoolean(row.getValue(_deleteRecordColumn));
return new RecordInfo(primaryKey, docId, comparisonValue, deleteRecord);
}
+ private PrimaryKey getPrimaryKey(GenericRow row) {
+ List<String> primaryKeyColumns = _schema.getPrimaryKeyColumns();
+ int numPrimaryKeyColumns = primaryKeyColumns.size();
+ Object[] values = new Object[numPrimaryKeyColumns];
+ for (int i = 0; i < numPrimaryKeyColumns; i++) {
+ String primaryKeyColumn = primaryKeyColumns.get(i);
+ Object value = row.getValue(primaryKeyColumn);
+ DataType dataType =
_schema.getFieldSpecFor(primaryKeyColumn).getDataType();
+ values[i] = normalizePrimaryKeyValue(value, dataType, primaryKeyColumn);
+ }
+ return new PrimaryKey(values);
+ }
+
+ private Object normalizePrimaryKeyValue(@Nullable Object value, DataType
dataType, String column) {
Review Comment:
This special handling doesn't seem correct. Why do we need it?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/columnminmaxvalue/ColumnMinMaxValueGenerator.java:
##########
@@ -360,26 +360,28 @@ private void
addColumnMinMaxValueWithoutDictionary(ColumnMetadata columnMetadata
break;
}
case BYTES: {
+ // ByteArray.compare is unsigned byte-wise lexicographic; for
canonical 16-byte big-endian UUIDs this is
+ // identical to UuidUtils.compare's unsigned 64-bit-word ordering,
so a single comparator is sufficient.
byte[] min = null;
byte[] max = null;
if (isSingleValue) {
for (int docId = 0; docId < numDocs; docId++) {
byte[] value = rawIndexReader.getBytes(docId, readerContext);
- if (min == null || ByteArray.compare(value, min) > 0) {
+ if (min == null || ByteArray.compare(value, min) < 0) {
Review Comment:
Wow, are we always storing the min as max and max as min? Make a separate PR
for this fix
##########
pinot-core/src/main/java/org/apache/pinot/core/util/SegmentProcessorAvroUtils.java:
##########
@@ -57,20 +61,50 @@ public static GenericData.Record
convertGenericRowToAvroRecord(GenericRow generi
*/
public static GenericData.Record convertGenericRowToAvroRecord(GenericRow
genericRow,
GenericData.Record reusableRecord, Set<String> fields) {
+ Schema avroSchema = reusableRecord.getSchema();
for (String field : fields) {
Object value = genericRow.getValue(field);
if (value instanceof Object[]) {
- reusableRecord.put(field, Arrays.asList((Object[]) value));
+ Schema.Field avroField = avroSchema.getField(field);
+ if (avroField != null && isUuidArrayLogicalType(avroField.schema())) {
+ // MV UUID columns are emitted with an Avro
array<string{logicalType:uuid}> schema; convert each
+ // 16-byte element to its canonical UUID string so
GenericDatumWriter accepts the record.
Review Comment:
Can we register the `Conversion` instead? I feel that is cleaner, and aligns
with the internal Pinot storage format
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/creator/BloomFilterCreator.java:
##########
@@ -33,6 +34,8 @@ public interface BloomFilterCreator extends IndexCreator {
default void add(Object value, int dictId) {
if (getDataType() == FieldSpec.DataType.BYTES) {
add(BytesUtils.toHexString((byte[]) value));
+ } else if (getDataType() == FieldSpec.DataType.UUID) {
Review Comment:
Is the value already converted into canonical format (i.e. `byte[]`)? If so,
shall we just put the storage format (hex encoded)
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java:
##########
@@ -586,7 +580,7 @@ private boolean isNoDictionaryColumn(FieldIndexConfigs
indexConfigs, FieldSpec f
// So to continue aggregating metrics for such cases, we will create
dictionary even
// if the column is part of noDictionary set from table config
if (fieldSpec instanceof DimensionFieldSpec && isAggregateMetricsEnabled()
&& (dataType == STRING
- || dataType == BYTES)) {
+ || dataType.getStoredType() == BYTES)) {
Review Comment:
I think we need to use stored type for both STRING and BYTES
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/column/DefaultNullValueVirtualColumnProvider.java:
##########
@@ -70,6 +70,7 @@ public Dictionary buildDictionary(VirtualColumnContext
context) {
case STRING:
return new ConstantValueStringDictionary((String)
fieldSpec.getDefaultNullValue());
case BYTES:
+ case UUID:
Review Comment:
Not needed
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexType.java:
##########
@@ -402,13 +403,15 @@ public MutableIndex
createMutableIndex(MutableIndexContext context, ForwardIndex
}
} else {
// TODO: Add support for variable width (bytes, string, big decimal)
MV RAW column types
- assert storedType.isFixedWidth();
+ Preconditions.checkState(dataType.isFixedWidth(), "Unsupported stored
type: %s for no-dictionary MV column: %s",
Review Comment:
(minor) Don't change this. `assert` means it is unreachable without overhead
--
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]
