raghavyadav01 commented on code in PR #19608:
URL: https://github.com/apache/pinot/pull/19608#discussion_r4118367542
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/MutableKeyColumn.java:
##########
Review Comment:
Confirmed and fixed in c10fc97 — `getValue` now branches on shape and reads
an MV key through `getDictIdMV`. Reachable exactly as you describe, through
`getMapValue` → `ProjectionBlock`, so it took out `SELECT` of the column and
the seal path.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/MutableOpenStructIndex.java:
##########
@@ -294,7 +373,9 @@ public ColumnMetadata getColumnMetadata(String key) {
}
FieldSpec spec = _childFieldSpecs.get(key);
if (spec == null) {
- spec = new DimensionFieldSpec(key, col.getStoredType(), true);
+ // Shape comes from the column, not a fixed single-value assumption: a
key holding lists has a multi-value
+ // forward index, and metadata that disagreed with it would tell the
query planner the wrong thing.
+ spec = new DimensionFieldSpec(key, col.getStoredType(),
col.isSingleValue());
}
return new SimpleColumnMetadata(spec, _capacity);
Review Comment:
Confirmed and fixed in c10fc97. `SimpleColumnMetadata` gains an overload
carrying the real max-MV length, defaulting to `UNAVAILABLE` so the MAP caller
is untouched, and `MutableOpenStructIndex` passes
`MutableKeyColumn.MAX_NUM_MULTI_VALUES` for a multi-value key. Your read of why
it only surfaced now is right: the shape fix is what first sends these keys
down the MV scan branch.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/MutableKeyColumn.java:
##########
@@ -168,6 +191,26 @@ public void setValue(int docId, Object value) {
_lastIndexedDocId = docId;
}
+ /// Indexes a list of values at `docId`. Elements must already be coerced to
the stored type. Throws
+ /// [IllegalArgumentException] when the list is longer than
[#MAX_NUM_MULTI_VALUES]; the caller drops and meters
+ /// it, the same as a value that cannot be coerced.
+ public void setValues(int docId, Object[] values) {
+ int[] dictIds = new int[values.length];
+ for (int i = 0; i < values.length; i++) {
+ dictIds[i] = _dictionary.index(values[i]);
Review Comment:
Confirmed and fixed in c10fc97 — the length check now runs before the
dictionary is touched.
Worth recording the blast radius for anyone reading later: the orphaned
dictIds are unreferenced, so they inflate distinct-value counts and seal-time
cardinality rather than mis-mapping any row. Still wrong, just not corrupting.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/MutableKeyColumn.java:
##########
@@ -243,6 +286,25 @@ public void readDictIds(int[] docIds, int length, int[]
dictIdBuffer, ForwardInd
}
}
+ @Override
+ public int getDictIdMV(int docId, int[] dictIdBuffer,
ForwardIndexReaderContext context) {
+ if (docId > _lastIndexedDocId) {
Review Comment:
Confirmed and fixed in c10fc97 — both overloads now consult
`_presenceBitmap` instead of only the watermark, exactly as you suggested.
The class comment was the giveaway: it says in-range holes need no guard
because chunks are zero-initialized and read as dictId 0. True for the flat
single-value array, false for a multi-value index that stores a length header —
a hole reads as zero values, i.e. an empty list, which is a value no document
held and no sealed segment produces.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/openstruct/OpenStructColumnSplitter.java:
##########
@@ -196,62 +212,133 @@ public Set<String> classify() {
private void addMap(@Nullable Map<String, Object> map) {
if (map != null && !map.isEmpty()) {
- for (Map.Entry<String, Object> entry : map.entrySet()) {
- String key = entry.getKey();
- Object rawValue = entry.getValue();
- if (rawValue == null) {
- continue;
- }
- if (_config.isIgnoredKey(key)) {
- _ignoredKeyDropCount++;
- continue;
- }
- FieldSpec keySpec = _childFieldSpecs.get(key);
- DataType valueType;
- if (keySpec != null) {
- valueType = keySpec.getDataType();
+ OpenStructKeyFlattener.flatten(map, _maxNestedKeyDepth, this::addEntry);
+ }
+ _numDocs++;
+ }
+
+ /// Accumulates one flat key of the current document. `container` marks a
key whose value is a nested object
+ /// rendered as JSON text; see [#classify()] for why those are held out of
automatic dense selection.
+ private void addEntry(String key, @Nullable Object rawValue, boolean
container) {
+ if (rawValue == null) {
+ return;
+ }
+ if (_config.isIgnoredKey(key)) {
+ _ignoredKeyDropCount++;
+ return;
+ }
+ if (container) {
+ _containerKeys.add(key);
+ }
+ FieldSpec keySpec = _childFieldSpecs.get(key);
+ // Shape is decided by the first value the key presents and then sticks,
exactly as its type does. A
+ // collection arriving on a key whose shape is already scalar is handled
as any other value it cannot
+ // represent -- stringified on a STRING key, a coercion failure on a typed
one -- rather than reshaping a
+ // column other documents already wrote to.
+ boolean multiValueKey;
+ if (_presenceBitmaps.containsKey(key)) {
+ multiValueKey = _multiValueKeys.contains(key);
+ } else {
+ // A declaration decides the shape in both directions -- a key declared
single-value stays single-value
+ // even when its values are collections, because the declaration is what
the user asked for. Only an
+ // undeclared key takes its shape from the data.
+ multiValueKey = keySpec != null
+ ? !keySpec.isSingleValueField()
+ : OpenStructTypeInference.asMultiValue(rawValue) != null;
+ if (multiValueKey) {
+ _multiValueKeys.add(key);
Review Comment:
Confirmed and fixed in c10fc97 — the shape is now recorded only alongside a
stored value, so the empty-list row returns before `_multiValueKeys.add` and
both tiers take their shape from the first row that actually stores something.
I went with your first option rather than giving the mutable index a side
table, since the splitter returning early is what already makes the two paths
agree.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/SparseKeyDataSource.java:
##########
@@ -153,6 +187,187 @@ public byte[] getBytes(int docId,
ForwardIndexReaderContext context) {
}, FieldSpec.DEFAULT_DIMENSION_NULL_VALUE_OF_BYTES);
Review Comment:
Confirmed and fixed in c10fc97 — both now route through
`declaredOr(byte[].class, ...)` like the other twelve.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/ItemTransformFunction.java:
##########
@@ -121,13 +122,71 @@ public long[] transformToLongValuesSV(ValueBlock
valueBlock) {
return valueBlock.getBlockValueSet(_keyPath).getLongValuesSV();
}
+ @Override
+ public float[] transformToFloatValuesSV(ValueBlock valueBlock) {
+ return valueBlock.getBlockValueSet(_keyPath).getFloatValuesSV();
+ }
+
@Override
public double[] transformToDoubleValuesSV(ValueBlock valueBlock) {
return valueBlock.getBlockValueSet(_keyPath).getDoubleValuesSV();
}
+ /// Without this the base class has no way to produce a BIG_DECIMAL: its
conversion switch widens INT, LONG, FLOAT,
+ /// DOUBLE, STRING and BYTES into one, but a key whose own type is already
BIG_DECIMAL matches no case and throws
+ /// `Cannot read SV BIG_DECIMAL as BIG_DECIMAL`. Reading it straight off the
key's value source is both the fix and
+ /// the cheaper path.
+ @Override
+ public BigDecimal[] transformToBigDecimalValuesSV(ValueBlock valueBlock) {
+ return valueBlock.getBlockValueSet(_keyPath).getBigDecimalValuesSV();
+ }
+
@Override
public String[] transformToStringValuesSV(ValueBlock valueBlock) {
return valueBlock.getBlockValueSet(_keyPath).getStringValuesSV();
}
+
+ @Override
+ public byte[][] transformToBytesValuesSV(ValueBlock valueBlock) {
+ return valueBlock.getBlockValueSet(_keyPath).getBytesValuesSV();
+ }
+
+ // A key can hold a list, in which case its value source is multi-value and
the engine asks for the values that
+ // way. The result metadata above already reports the key's own shape, so
these are the reads that shape implies;
+ // without them a multi-value key would be storable and describable but not
readable.
+
+ @Override
+ public int[][] transformToDictIdsMV(ValueBlock valueBlock) {
+ return valueBlock.getBlockValueSet(_keyPath).getDictionaryIdsMV();
+ }
+
+ @Override
+ public int[][] transformToIntValuesMV(ValueBlock valueBlock) {
+ return valueBlock.getBlockValueSet(_keyPath).getIntValuesMV();
+ }
+
+ @Override
+ public long[][] transformToLongValuesMV(ValueBlock valueBlock) {
+ return valueBlock.getBlockValueSet(_keyPath).getLongValuesMV();
+ }
+
+ @Override
+ public float[][] transformToFloatValuesMV(ValueBlock valueBlock) {
+ return valueBlock.getBlockValueSet(_keyPath).getFloatValuesMV();
+ }
+
+ @Override
+ public double[][] transformToDoubleValuesMV(ValueBlock valueBlock) {
+ return valueBlock.getBlockValueSet(_keyPath).getDoubleValuesMV();
+ }
+
+ @Override
+ public String[][] transformToStringValuesMV(ValueBlock valueBlock) {
+ return valueBlock.getBlockValueSet(_keyPath).getStringValuesMV();
+ }
+
+ @Override
+ public byte[][][] transformToBytesValuesMV(ValueBlock valueBlock) {
+ return valueBlock.getBlockValueSet(_keyPath).getBytesValuesMV();
Review Comment:
Confirmed and fixed in c10fc97 — added, with a note on why the base class
cannot serve it (its no-dictionary switch widens other types into BIG_DECIMAL
but has no case for a key already declared BIG_DECIMAL).
--
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]
