xiangfu0 commented on code in PR #18870:
URL: https://github.com/apache/pinot/pull/18870#discussion_r3553935231
##########
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:
Changed to use the stored type for both: `dataType.getStoredType() == STRING
|| dataType.getStoredType() == BYTES`.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
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:
Unified — dropped the UUID special case; both BYTES and UUID now sort via
`ByteArray.compare` (unsigned byte-wise), the identical ordering
`UuidUtils.compare` produced for 16-byte values.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
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:
Reverted — restored the blank line.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
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:
Reverted to `assert dataType.isFixedWidth();` (kept `dataType` rather than
`storedType` so UUID — stored type BYTES but logically fixed-width — still
passes).
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
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:
Removed the `case UUID:` — this switch is on the stored type (UUID maps to
BYTES), so the label was unreachable.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
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:
Agreed — using the storage type here (and the BOOLEAN/TIMESTAMP handling) is
a pre-existing issue orthogonal to UUID support; will address it in a separate
bugfix.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
--
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]
