You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "abhioncbr (via GitHub)" <gi...@apache.org> on 2023/06/30 22:16:49 UTC
[GitHub] [pinot] abhioncbr opened a new pull request, #10891: Ensure min/max value generation in the segment metadata.
abhioncbr opened a new pull request, #10891:
URL: https://github.com/apache/pinot/pull/10891
Labels:
`enhancement`
- ensuring that the min/max value is in the segment metadata.
- trimmed the string/bytes value if exceeding the length limit. Added the test for the change.
As per the [issue](https://github.com/apache/pinot/issues/10793)
cc: @Jackie-Jiang
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Ensure min/max value generation in the segment metadata. [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10891:
URL: https://github.com/apache/pinot/pull/10891#discussion_r1503177630
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/columnminmaxvalue/ColumnMinMaxValueGenerator.java:
##########
@@ -119,65 +121,126 @@ private List<String> getColumnsToAddMinMaxValue() {
private boolean needAddColumnMinMaxValueForColumn(String columnName) {
ColumnMetadata columnMetadata = _segmentMetadata.getColumnMetadataFor(columnName);
- return columnMetadata.hasDictionary() && columnMetadata.getMinValue() == null
+ return columnMetadata.getMinValue() == null
&& columnMetadata.getMaxValue() == null && !columnMetadata.isMinMaxValueInvalid();
}
private void addColumnMinMaxValueForColumn(String columnName)
throws Exception {
// Skip column without dictionary or with min/max value already set
ColumnMetadata columnMetadata = _segmentMetadata.getColumnMetadataFor(columnName);
- if (!columnMetadata.hasDictionary() || columnMetadata.getMinValue() != null
- || columnMetadata.getMaxValue() != null) {
+ if (columnMetadata.getMinValue() != null || columnMetadata.getMaxValue() != null) {
return;
}
- PinotDataBuffer dictionaryBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.dictionary());
DataType dataType = columnMetadata.getDataType().getStoredType();
- int length = columnMetadata.getCardinality();
- switch (dataType) {
- case INT:
- try (IntDictionary intDictionary = new IntDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- intDictionary.getStringValue(0), intDictionary.getStringValue(length - 1));
- }
- break;
- case LONG:
- try (LongDictionary longDictionary = new LongDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- longDictionary.getStringValue(0), longDictionary.getStringValue(length - 1));
- }
- break;
- case FLOAT:
- try (FloatDictionary floatDictionary = new FloatDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- floatDictionary.getStringValue(0), floatDictionary.getStringValue(length - 1));
- }
- break;
- case DOUBLE:
- try (DoubleDictionary doubleDictionary = new DoubleDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- doubleDictionary.getStringValue(0), doubleDictionary.getStringValue(length - 1));
- }
- break;
- case STRING:
- try (StringDictionary stringDictionary = new StringDictionary(dictionaryBuffer, length,
- columnMetadata.getColumnMaxLength())) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- stringDictionary.getStringValue(0), stringDictionary.getStringValue(length - 1));
- }
- break;
- case BYTES:
- try (BytesDictionary bytesDictionary = new BytesDictionary(dictionaryBuffer, length,
- columnMetadata.getColumnMaxLength())) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- bytesDictionary.getStringValue(0), bytesDictionary.getStringValue(length - 1));
- }
- break;
- default:
- throw new IllegalStateException("Unsupported data type: " + dataType + " for column: " + columnName);
+ if (columnMetadata.hasDictionary()) {
+ PinotDataBuffer dictionaryBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.dictionary());
+ int length = columnMetadata.getCardinality();
+ switch (dataType) {
+ case INT:
+ try (IntDictionary intDictionary = new IntDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ intDictionary.getStringValue(0), intDictionary.getStringValue(length - 1));
+ }
+ break;
+ case LONG:
+ try (LongDictionary longDictionary = new LongDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ longDictionary.getStringValue(0), longDictionary.getStringValue(length - 1));
+ }
+ break;
+ case FLOAT:
+ try (FloatDictionary floatDictionary = new FloatDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ floatDictionary.getStringValue(0), floatDictionary.getStringValue(length - 1));
+ }
+ break;
+ case DOUBLE:
+ try (DoubleDictionary doubleDictionary = new DoubleDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ doubleDictionary.getStringValue(0), doubleDictionary.getStringValue(length - 1));
+ }
+ break;
+ case STRING:
+ try (StringDictionary stringDictionary = new StringDictionary(dictionaryBuffer, length,
+ columnMetadata.getColumnMaxLength())) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ stringDictionary.getStringValue(0), stringDictionary.getStringValue(length - 1));
+ }
+ break;
+ case BYTES:
+ try (BytesDictionary bytesDictionary = new BytesDictionary(dictionaryBuffer, length,
+ columnMetadata.getColumnMaxLength())) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ bytesDictionary.getStringValue(0), bytesDictionary.getStringValue(length - 1));
+ }
+ break;
+ default:
+ throw new IllegalStateException("Unsupported data type: " + dataType + " for column: " + columnName);
+ }
+ } else if (_segmentMetadata.getSchema().getFieldSpecFor(columnName).getFieldType() == FieldSpec.FieldType.METRIC) {
+ // setting min/max for non-dictionary columns.
+ PinotDataBuffer forwardBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.forward());
+ switch (dataType) {
+ case INT:
+ try (FixedByteChunkSVForwardIndexReader rawIndexReader = new FixedByteChunkSVForwardIndexReader(forwardBuffer,
+ DataType.INT); ChunkReaderContext readerContext = rawIndexReader.createContext()) {
+ Integer[] minMaxValue = {Integer.MAX_VALUE, Integer.MIN_VALUE};
+ for (int docs = 0; docs < columnMetadata.getTotalDocs(); docs++) {
+ minMaxValue = getMinMaxValue(minMaxValue, rawIndexReader.getInt(docs, readerContext));
+ }
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ String.valueOf(minMaxValue[0]), String.valueOf(minMaxValue[1]));
+ }
+ break;
+ case LONG:
+ try (FixedByteChunkSVForwardIndexReader rawIndexReader = new FixedByteChunkSVForwardIndexReader(forwardBuffer,
+ DataType.LONG); ChunkReaderContext readerContext = rawIndexReader.createContext()) {
+ Long[] minMaxValue = {Long.MAX_VALUE, Long.MIN_VALUE};
+ for (int docs = 0; docs < columnMetadata.getTotalDocs(); docs++) {
+ minMaxValue = getMinMaxValue(minMaxValue, rawIndexReader.getLong(docs, readerContext));
+ }
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ String.valueOf(minMaxValue[0]), String.valueOf(minMaxValue[1]));
+ }
+ break;
+ case FLOAT:
+ try (FixedByteChunkSVForwardIndexReader rawIndexReader = new FixedByteChunkSVForwardIndexReader(forwardBuffer,
+ DataType.FLOAT); ChunkReaderContext readerContext = rawIndexReader.createContext()) {
+ Float[] minMaxValue = {Float.MAX_VALUE, Float.MIN_VALUE};
+ for (int docs = 0; docs < columnMetadata.getTotalDocs(); docs++) {
+ minMaxValue = getMinMaxValue(minMaxValue, rawIndexReader.getFloat(docs, readerContext));
+ }
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ String.valueOf(minMaxValue[0]), String.valueOf(minMaxValue[1]));
+ }
+ break;
+ case DOUBLE:
+ try (FixedByteChunkSVForwardIndexReader rawIndexReader = new FixedByteChunkSVForwardIndexReader(forwardBuffer,
+ DataType.DOUBLE); ChunkReaderContext readerContext = rawIndexReader.createContext()) {
+ Double[] minMaxValue = {Double.MAX_VALUE, Double.MIN_VALUE};
+ for (int docs = 0; docs < columnMetadata.getTotalDocs(); docs++) {
+ minMaxValue = getMinMaxValue(minMaxValue, rawIndexReader.getDouble(docs, readerContext));
+ }
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ String.valueOf(minMaxValue[0]), String.valueOf(minMaxValue[1]));
+ }
+ break;
+ default:
Review Comment:
@jadami10 Good catch. The intention is to handle all data types, or we will run this generator every time loading the segment. Clearly `BIG_DECIMAL` was left unhandled. It was actually not introduced in this PR as this PR only extends the handling to raw index.
+1 on making it more robust, and adding more tests to cover all these scenarios.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhioncbr commented on a diff in pull request #10891: [WIP]: ensure min/max value generation in the segment metadata.
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on code in PR #10891:
URL: https://github.com/apache/pinot/pull/10891#discussion_r1233335139
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/columnminmaxvalue/ColumnMinMaxValueGenerator.java:
##########
@@ -119,65 +121,126 @@ private List<String> getColumnsToAddMinMaxValue() {
private boolean needAddColumnMinMaxValueForColumn(String columnName) {
ColumnMetadata columnMetadata = _segmentMetadata.getColumnMetadataFor(columnName);
- return columnMetadata.hasDictionary() && columnMetadata.getMinValue() == null
+ return columnMetadata.getMinValue() == null
&& columnMetadata.getMaxValue() == null && !columnMetadata.isMinMaxValueInvalid();
}
private void addColumnMinMaxValueForColumn(String columnName)
throws Exception {
// Skip column without dictionary or with min/max value already set
ColumnMetadata columnMetadata = _segmentMetadata.getColumnMetadataFor(columnName);
- if (!columnMetadata.hasDictionary() || columnMetadata.getMinValue() != null
- || columnMetadata.getMaxValue() != null) {
+ if (columnMetadata.getMinValue() != null || columnMetadata.getMaxValue() != null) {
return;
}
- PinotDataBuffer dictionaryBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.dictionary());
DataType dataType = columnMetadata.getDataType().getStoredType();
- int length = columnMetadata.getCardinality();
- switch (dataType) {
- case INT:
- try (IntDictionary intDictionary = new IntDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- intDictionary.getStringValue(0), intDictionary.getStringValue(length - 1));
- }
- break;
- case LONG:
- try (LongDictionary longDictionary = new LongDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- longDictionary.getStringValue(0), longDictionary.getStringValue(length - 1));
- }
- break;
- case FLOAT:
- try (FloatDictionary floatDictionary = new FloatDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- floatDictionary.getStringValue(0), floatDictionary.getStringValue(length - 1));
- }
- break;
- case DOUBLE:
- try (DoubleDictionary doubleDictionary = new DoubleDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- doubleDictionary.getStringValue(0), doubleDictionary.getStringValue(length - 1));
- }
- break;
- case STRING:
- try (StringDictionary stringDictionary = new StringDictionary(dictionaryBuffer, length,
- columnMetadata.getColumnMaxLength())) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- stringDictionary.getStringValue(0), stringDictionary.getStringValue(length - 1));
- }
- break;
- case BYTES:
- try (BytesDictionary bytesDictionary = new BytesDictionary(dictionaryBuffer, length,
- columnMetadata.getColumnMaxLength())) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- bytesDictionary.getStringValue(0), bytesDictionary.getStringValue(length - 1));
- }
- break;
- default:
- throw new IllegalStateException("Unsupported data type: " + dataType + " for column: " + columnName);
+ if (columnMetadata.hasDictionary()) {
+ PinotDataBuffer dictionaryBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.dictionary());
+ int length = columnMetadata.getCardinality();
+ switch (dataType) {
+ case INT:
+ try (IntDictionary intDictionary = new IntDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ intDictionary.getStringValue(0), intDictionary.getStringValue(length - 1));
+ }
+ break;
+ case LONG:
+ try (LongDictionary longDictionary = new LongDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ longDictionary.getStringValue(0), longDictionary.getStringValue(length - 1));
+ }
+ break;
+ case FLOAT:
+ try (FloatDictionary floatDictionary = new FloatDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ floatDictionary.getStringValue(0), floatDictionary.getStringValue(length - 1));
+ }
+ break;
+ case DOUBLE:
+ try (DoubleDictionary doubleDictionary = new DoubleDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ doubleDictionary.getStringValue(0), doubleDictionary.getStringValue(length - 1));
+ }
+ break;
+ case STRING:
+ try (StringDictionary stringDictionary = new StringDictionary(dictionaryBuffer, length,
+ columnMetadata.getColumnMaxLength())) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ stringDictionary.getStringValue(0), stringDictionary.getStringValue(length - 1));
+ }
+ break;
+ case BYTES:
+ try (BytesDictionary bytesDictionary = new BytesDictionary(dictionaryBuffer, length,
+ columnMetadata.getColumnMaxLength())) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ bytesDictionary.getStringValue(0), bytesDictionary.getStringValue(length - 1));
+ }
+ break;
+ default:
+ throw new IllegalStateException("Unsupported data type: " + dataType + " for column: " + columnName);
+ }
+ } else {
+ // setting min/max for non-dictionary columns.
+ PinotDataBuffer forwardBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.forward());
Review Comment:
@Jackie-Jiang Can you please confirm my approach?
I am creating the `PinotDataBuffer` based on the forward index and then reading the value to get the min/max by iterating over the doc.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10891: Ensure min/max value generation in the segment metadata.
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10891:
URL: https://github.com/apache/pinot/pull/10891#discussion_r1243058344
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -570,12 +570,14 @@ public static void addColumnMetadataInfo(PropertiesConfiguration properties, Str
public static void addColumnMinMaxValueInfo(PropertiesConfiguration properties, String column, String minValue,
String maxValue) {
if (isValidPropertyValue(minValue)) {
- properties.setProperty(getKeyFor(column, MIN_VALUE), minValue);
+ properties.setProperty(getKeyFor(column, MIN_VALUE),
+ minValue.substring(0, Math.min(minValue.length(), METADATA_PROPERTY_LENGTH_LIMIT)));
} else {
properties.setProperty(getKeyFor(column, MIN_MAX_VALUE_INVALID), true);
}
if (isValidPropertyValue(maxValue)) {
- properties.setProperty(getKeyFor(column, MAX_VALUE), maxValue);
+ properties.setProperty(getKeyFor(column, MAX_VALUE),
+ maxValue.substring(0, Math.min(maxValue.length(), METADATA_PROPERTY_LENGTH_LIMIT)));
Review Comment:
I'd suggest creating a separate PR to address the invalid value issue if we do want to solve it. This PR can focus on generating min/max value for raw index column
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10891: Ensure min/max value generation in the segment metadata.
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10891:
URL: https://github.com/apache/pinot/pull/10891#discussion_r1248654776
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/columnminmaxvalue/ColumnMinMaxValueGenerator.java:
##########
@@ -119,65 +123,153 @@ private List<String> getColumnsToAddMinMaxValue() {
private boolean needAddColumnMinMaxValueForColumn(String columnName) {
ColumnMetadata columnMetadata = _segmentMetadata.getColumnMetadataFor(columnName);
- return columnMetadata.hasDictionary() && columnMetadata.getMinValue() == null
+ return columnMetadata.getMinValue() == null
&& columnMetadata.getMaxValue() == null && !columnMetadata.isMinMaxValueInvalid();
}
private void addColumnMinMaxValueForColumn(String columnName)
throws Exception {
// Skip column without dictionary or with min/max value already set
ColumnMetadata columnMetadata = _segmentMetadata.getColumnMetadataFor(columnName);
- if (!columnMetadata.hasDictionary() || columnMetadata.getMinValue() != null
- || columnMetadata.getMaxValue() != null) {
+ if (columnMetadata.getMinValue() != null || columnMetadata.getMaxValue() != null) {
return;
}
- PinotDataBuffer dictionaryBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.dictionary());
DataType dataType = columnMetadata.getDataType().getStoredType();
- int length = columnMetadata.getCardinality();
- switch (dataType) {
- case INT:
- try (IntDictionary intDictionary = new IntDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- intDictionary.getStringValue(0), intDictionary.getStringValue(length - 1));
- }
- break;
- case LONG:
- try (LongDictionary longDictionary = new LongDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- longDictionary.getStringValue(0), longDictionary.getStringValue(length - 1));
- }
- break;
- case FLOAT:
- try (FloatDictionary floatDictionary = new FloatDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- floatDictionary.getStringValue(0), floatDictionary.getStringValue(length - 1));
- }
- break;
- case DOUBLE:
- try (DoubleDictionary doubleDictionary = new DoubleDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- doubleDictionary.getStringValue(0), doubleDictionary.getStringValue(length - 1));
- }
- break;
- case STRING:
- try (StringDictionary stringDictionary = new StringDictionary(dictionaryBuffer, length,
- columnMetadata.getColumnMaxLength())) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- stringDictionary.getStringValue(0), stringDictionary.getStringValue(length - 1));
- }
- break;
- case BYTES:
- try (BytesDictionary bytesDictionary = new BytesDictionary(dictionaryBuffer, length,
- columnMetadata.getColumnMaxLength())) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- bytesDictionary.getStringValue(0), bytesDictionary.getStringValue(length - 1));
- }
- break;
- default:
- throw new IllegalStateException("Unsupported data type: " + dataType + " for column: " + columnName);
+ if (columnMetadata.hasDictionary()) {
+ PinotDataBuffer dictionaryBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.dictionary());
+ int length = columnMetadata.getCardinality();
+ switch (dataType) {
+ case INT:
+ try (IntDictionary intDictionary = new IntDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ intDictionary.getStringValue(0), intDictionary.getStringValue(length - 1));
+ }
+ break;
+ case LONG:
+ try (LongDictionary longDictionary = new LongDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ longDictionary.getStringValue(0), longDictionary.getStringValue(length - 1));
+ }
+ break;
+ case FLOAT:
+ try (FloatDictionary floatDictionary = new FloatDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ floatDictionary.getStringValue(0), floatDictionary.getStringValue(length - 1));
+ }
+ break;
+ case DOUBLE:
+ try (DoubleDictionary doubleDictionary = new DoubleDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ doubleDictionary.getStringValue(0), doubleDictionary.getStringValue(length - 1));
+ }
+ break;
+ case STRING:
+ try (StringDictionary stringDictionary = new StringDictionary(dictionaryBuffer, length,
Review Comment:
Nvm, I checked and it is the correct format. I somehow misread it
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhioncbr commented on a diff in pull request #10891: Ensure min/max value generation in the segment metadata.
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on code in PR #10891:
URL: https://github.com/apache/pinot/pull/10891#discussion_r1239788013
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/columnminmaxvalue/ColumnMinMaxValueGenerator.java:
##########
@@ -119,65 +121,126 @@ private List<String> getColumnsToAddMinMaxValue() {
private boolean needAddColumnMinMaxValueForColumn(String columnName) {
ColumnMetadata columnMetadata = _segmentMetadata.getColumnMetadataFor(columnName);
- return columnMetadata.hasDictionary() && columnMetadata.getMinValue() == null
+ return columnMetadata.getMinValue() == null
&& columnMetadata.getMaxValue() == null && !columnMetadata.isMinMaxValueInvalid();
}
private void addColumnMinMaxValueForColumn(String columnName)
throws Exception {
// Skip column without dictionary or with min/max value already set
ColumnMetadata columnMetadata = _segmentMetadata.getColumnMetadataFor(columnName);
- if (!columnMetadata.hasDictionary() || columnMetadata.getMinValue() != null
- || columnMetadata.getMaxValue() != null) {
+ if (columnMetadata.getMinValue() != null || columnMetadata.getMaxValue() != null) {
return;
}
- PinotDataBuffer dictionaryBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.dictionary());
DataType dataType = columnMetadata.getDataType().getStoredType();
- int length = columnMetadata.getCardinality();
- switch (dataType) {
- case INT:
- try (IntDictionary intDictionary = new IntDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- intDictionary.getStringValue(0), intDictionary.getStringValue(length - 1));
- }
- break;
- case LONG:
- try (LongDictionary longDictionary = new LongDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- longDictionary.getStringValue(0), longDictionary.getStringValue(length - 1));
- }
- break;
- case FLOAT:
- try (FloatDictionary floatDictionary = new FloatDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- floatDictionary.getStringValue(0), floatDictionary.getStringValue(length - 1));
- }
- break;
- case DOUBLE:
- try (DoubleDictionary doubleDictionary = new DoubleDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- doubleDictionary.getStringValue(0), doubleDictionary.getStringValue(length - 1));
- }
- break;
- case STRING:
- try (StringDictionary stringDictionary = new StringDictionary(dictionaryBuffer, length,
- columnMetadata.getColumnMaxLength())) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- stringDictionary.getStringValue(0), stringDictionary.getStringValue(length - 1));
- }
- break;
- case BYTES:
- try (BytesDictionary bytesDictionary = new BytesDictionary(dictionaryBuffer, length,
- columnMetadata.getColumnMaxLength())) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- bytesDictionary.getStringValue(0), bytesDictionary.getStringValue(length - 1));
- }
- break;
- default:
- throw new IllegalStateException("Unsupported data type: " + dataType + " for column: " + columnName);
+ if (columnMetadata.hasDictionary()) {
+ PinotDataBuffer dictionaryBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.dictionary());
+ int length = columnMetadata.getCardinality();
+ switch (dataType) {
+ case INT:
+ try (IntDictionary intDictionary = new IntDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ intDictionary.getStringValue(0), intDictionary.getStringValue(length - 1));
+ }
+ break;
+ case LONG:
+ try (LongDictionary longDictionary = new LongDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ longDictionary.getStringValue(0), longDictionary.getStringValue(length - 1));
+ }
+ break;
+ case FLOAT:
+ try (FloatDictionary floatDictionary = new FloatDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ floatDictionary.getStringValue(0), floatDictionary.getStringValue(length - 1));
+ }
+ break;
+ case DOUBLE:
+ try (DoubleDictionary doubleDictionary = new DoubleDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ doubleDictionary.getStringValue(0), doubleDictionary.getStringValue(length - 1));
+ }
+ break;
+ case STRING:
+ try (StringDictionary stringDictionary = new StringDictionary(dictionaryBuffer, length,
+ columnMetadata.getColumnMaxLength())) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ stringDictionary.getStringValue(0), stringDictionary.getStringValue(length - 1));
+ }
+ break;
+ case BYTES:
+ try (BytesDictionary bytesDictionary = new BytesDictionary(dictionaryBuffer, length,
+ columnMetadata.getColumnMaxLength())) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ bytesDictionary.getStringValue(0), bytesDictionary.getStringValue(length - 1));
+ }
+ break;
+ default:
+ throw new IllegalStateException("Unsupported data type: " + dataType + " for column: " + columnName);
+ }
+ } else if (_segmentMetadata.getSchema().getFieldSpecFor(columnName).getFieldType() == FieldSpec.FieldType.METRIC) {
Review Comment:
I have made the change. However, do we also need to support MV fields? If yes, the max and min will be the max and min values seen in any array or something else? Thanks
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhioncbr commented on a diff in pull request #10891: Ensure min/max value generation in the segment metadata.
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on code in PR #10891:
URL: https://github.com/apache/pinot/pull/10891#discussion_r1239791153
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -570,12 +570,14 @@ public static void addColumnMetadataInfo(PropertiesConfiguration properties, Str
public static void addColumnMinMaxValueInfo(PropertiesConfiguration properties, String column, String minValue,
String maxValue) {
if (isValidPropertyValue(minValue)) {
- properties.setProperty(getKeyFor(column, MIN_VALUE), minValue);
+ properties.setProperty(getKeyFor(column, MIN_VALUE),
+ minValue.substring(0, Math.min(minValue.length(), METADATA_PROPERTY_LENGTH_LIMIT)));
} else {
properties.setProperty(getKeyFor(column, MIN_MAX_VALUE_INVALID), true);
}
if (isValidPropertyValue(maxValue)) {
- properties.setProperty(getKeyFor(column, MAX_VALUE), maxValue);
+ properties.setProperty(getKeyFor(column, MAX_VALUE),
+ maxValue.substring(0, Math.min(maxValue.length(), METADATA_PROPERTY_LENGTH_LIMIT)));
Review Comment:
Yes, it's possible because of other reasons like
- leading or trailing whitespace character
- the presence of `,` in the values
Do we also want to tweak for them to set the min/max value?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhioncbr commented on a diff in pull request #10891: Ensure min/max value generation in the segment metadata.
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on code in PR #10891:
URL: https://github.com/apache/pinot/pull/10891#discussion_r1247371671
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/columnminmaxvalue/ColumnMinMaxValueGenerator.java:
##########
@@ -119,65 +123,153 @@ private List<String> getColumnsToAddMinMaxValue() {
private boolean needAddColumnMinMaxValueForColumn(String columnName) {
ColumnMetadata columnMetadata = _segmentMetadata.getColumnMetadataFor(columnName);
- return columnMetadata.hasDictionary() && columnMetadata.getMinValue() == null
+ return columnMetadata.getMinValue() == null
&& columnMetadata.getMaxValue() == null && !columnMetadata.isMinMaxValueInvalid();
}
private void addColumnMinMaxValueForColumn(String columnName)
throws Exception {
// Skip column without dictionary or with min/max value already set
ColumnMetadata columnMetadata = _segmentMetadata.getColumnMetadataFor(columnName);
- if (!columnMetadata.hasDictionary() || columnMetadata.getMinValue() != null
- || columnMetadata.getMaxValue() != null) {
+ if (columnMetadata.getMinValue() != null || columnMetadata.getMaxValue() != null) {
return;
}
- PinotDataBuffer dictionaryBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.dictionary());
DataType dataType = columnMetadata.getDataType().getStoredType();
- int length = columnMetadata.getCardinality();
- switch (dataType) {
- case INT:
- try (IntDictionary intDictionary = new IntDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- intDictionary.getStringValue(0), intDictionary.getStringValue(length - 1));
- }
- break;
- case LONG:
- try (LongDictionary longDictionary = new LongDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- longDictionary.getStringValue(0), longDictionary.getStringValue(length - 1));
- }
- break;
- case FLOAT:
- try (FloatDictionary floatDictionary = new FloatDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- floatDictionary.getStringValue(0), floatDictionary.getStringValue(length - 1));
- }
- break;
- case DOUBLE:
- try (DoubleDictionary doubleDictionary = new DoubleDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- doubleDictionary.getStringValue(0), doubleDictionary.getStringValue(length - 1));
- }
- break;
- case STRING:
- try (StringDictionary stringDictionary = new StringDictionary(dictionaryBuffer, length,
- columnMetadata.getColumnMaxLength())) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- stringDictionary.getStringValue(0), stringDictionary.getStringValue(length - 1));
- }
- break;
- case BYTES:
- try (BytesDictionary bytesDictionary = new BytesDictionary(dictionaryBuffer, length,
- columnMetadata.getColumnMaxLength())) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- bytesDictionary.getStringValue(0), bytesDictionary.getStringValue(length - 1));
- }
- break;
- default:
- throw new IllegalStateException("Unsupported data type: " + dataType + " for column: " + columnName);
+ if (columnMetadata.hasDictionary()) {
+ PinotDataBuffer dictionaryBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.dictionary());
+ int length = columnMetadata.getCardinality();
+ switch (dataType) {
+ case INT:
+ try (IntDictionary intDictionary = new IntDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ intDictionary.getStringValue(0), intDictionary.getStringValue(length - 1));
+ }
+ break;
+ case LONG:
+ try (LongDictionary longDictionary = new LongDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ longDictionary.getStringValue(0), longDictionary.getStringValue(length - 1));
+ }
+ break;
+ case FLOAT:
+ try (FloatDictionary floatDictionary = new FloatDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ floatDictionary.getStringValue(0), floatDictionary.getStringValue(length - 1));
+ }
+ break;
+ case DOUBLE:
+ try (DoubleDictionary doubleDictionary = new DoubleDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ doubleDictionary.getStringValue(0), doubleDictionary.getStringValue(length - 1));
+ }
+ break;
+ case STRING:
+ try (StringDictionary stringDictionary = new StringDictionary(dictionaryBuffer, length,
Review Comment:
I do not see any linting issues for this; it was like this before. But If needed, I can make the changes. Thanks
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhioncbr closed pull request #10891: Ensure min/max value generation in the segment metadata.
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr closed pull request #10891: Ensure min/max value generation in the segment metadata.
URL: https://github.com/apache/pinot/pull/10891
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10891: [WIP]: ensure min/max value generation in the segment metadata.
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10891:
URL: https://github.com/apache/pinot/pull/10891#discussion_r1237916717
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/columnminmaxvalue/ColumnMinMaxValueGenerator.java:
##########
@@ -119,65 +121,126 @@ private List<String> getColumnsToAddMinMaxValue() {
private boolean needAddColumnMinMaxValueForColumn(String columnName) {
ColumnMetadata columnMetadata = _segmentMetadata.getColumnMetadataFor(columnName);
- return columnMetadata.hasDictionary() && columnMetadata.getMinValue() == null
+ return columnMetadata.getMinValue() == null
&& columnMetadata.getMaxValue() == null && !columnMetadata.isMinMaxValueInvalid();
}
private void addColumnMinMaxValueForColumn(String columnName)
throws Exception {
// Skip column without dictionary or with min/max value already set
ColumnMetadata columnMetadata = _segmentMetadata.getColumnMetadataFor(columnName);
- if (!columnMetadata.hasDictionary() || columnMetadata.getMinValue() != null
- || columnMetadata.getMaxValue() != null) {
+ if (columnMetadata.getMinValue() != null || columnMetadata.getMaxValue() != null) {
return;
}
- PinotDataBuffer dictionaryBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.dictionary());
DataType dataType = columnMetadata.getDataType().getStoredType();
- int length = columnMetadata.getCardinality();
- switch (dataType) {
- case INT:
- try (IntDictionary intDictionary = new IntDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- intDictionary.getStringValue(0), intDictionary.getStringValue(length - 1));
- }
- break;
- case LONG:
- try (LongDictionary longDictionary = new LongDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- longDictionary.getStringValue(0), longDictionary.getStringValue(length - 1));
- }
- break;
- case FLOAT:
- try (FloatDictionary floatDictionary = new FloatDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- floatDictionary.getStringValue(0), floatDictionary.getStringValue(length - 1));
- }
- break;
- case DOUBLE:
- try (DoubleDictionary doubleDictionary = new DoubleDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- doubleDictionary.getStringValue(0), doubleDictionary.getStringValue(length - 1));
- }
- break;
- case STRING:
- try (StringDictionary stringDictionary = new StringDictionary(dictionaryBuffer, length,
- columnMetadata.getColumnMaxLength())) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- stringDictionary.getStringValue(0), stringDictionary.getStringValue(length - 1));
- }
- break;
- case BYTES:
- try (BytesDictionary bytesDictionary = new BytesDictionary(dictionaryBuffer, length,
- columnMetadata.getColumnMaxLength())) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- bytesDictionary.getStringValue(0), bytesDictionary.getStringValue(length - 1));
- }
- break;
- default:
- throw new IllegalStateException("Unsupported data type: " + dataType + " for column: " + columnName);
+ if (columnMetadata.hasDictionary()) {
+ PinotDataBuffer dictionaryBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.dictionary());
+ int length = columnMetadata.getCardinality();
+ switch (dataType) {
+ case INT:
+ try (IntDictionary intDictionary = new IntDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ intDictionary.getStringValue(0), intDictionary.getStringValue(length - 1));
+ }
+ break;
+ case LONG:
+ try (LongDictionary longDictionary = new LongDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ longDictionary.getStringValue(0), longDictionary.getStringValue(length - 1));
+ }
+ break;
+ case FLOAT:
+ try (FloatDictionary floatDictionary = new FloatDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ floatDictionary.getStringValue(0), floatDictionary.getStringValue(length - 1));
+ }
+ break;
+ case DOUBLE:
+ try (DoubleDictionary doubleDictionary = new DoubleDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ doubleDictionary.getStringValue(0), doubleDictionary.getStringValue(length - 1));
+ }
+ break;
+ case STRING:
+ try (StringDictionary stringDictionary = new StringDictionary(dictionaryBuffer, length,
+ columnMetadata.getColumnMaxLength())) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ stringDictionary.getStringValue(0), stringDictionary.getStringValue(length - 1));
+ }
+ break;
+ case BYTES:
+ try (BytesDictionary bytesDictionary = new BytesDictionary(dictionaryBuffer, length,
+ columnMetadata.getColumnMaxLength())) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ bytesDictionary.getStringValue(0), bytesDictionary.getStringValue(length - 1));
+ }
+ break;
+ default:
+ throw new IllegalStateException("Unsupported data type: " + dataType + " for column: " + columnName);
+ }
+ } else if (_segmentMetadata.getSchema().getFieldSpecFor(columnName).getFieldType() == FieldSpec.FieldType.METRIC) {
Review Comment:
Why metric only?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/columnminmaxvalue/ColumnMinMaxValueGenerator.java:
##########
@@ -119,65 +121,126 @@ private List<String> getColumnsToAddMinMaxValue() {
private boolean needAddColumnMinMaxValueForColumn(String columnName) {
ColumnMetadata columnMetadata = _segmentMetadata.getColumnMetadataFor(columnName);
- return columnMetadata.hasDictionary() && columnMetadata.getMinValue() == null
+ return columnMetadata.getMinValue() == null
&& columnMetadata.getMaxValue() == null && !columnMetadata.isMinMaxValueInvalid();
}
private void addColumnMinMaxValueForColumn(String columnName)
throws Exception {
// Skip column without dictionary or with min/max value already set
ColumnMetadata columnMetadata = _segmentMetadata.getColumnMetadataFor(columnName);
- if (!columnMetadata.hasDictionary() || columnMetadata.getMinValue() != null
- || columnMetadata.getMaxValue() != null) {
+ if (columnMetadata.getMinValue() != null || columnMetadata.getMaxValue() != null) {
return;
}
- PinotDataBuffer dictionaryBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.dictionary());
DataType dataType = columnMetadata.getDataType().getStoredType();
- int length = columnMetadata.getCardinality();
- switch (dataType) {
- case INT:
- try (IntDictionary intDictionary = new IntDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- intDictionary.getStringValue(0), intDictionary.getStringValue(length - 1));
- }
- break;
- case LONG:
- try (LongDictionary longDictionary = new LongDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- longDictionary.getStringValue(0), longDictionary.getStringValue(length - 1));
- }
- break;
- case FLOAT:
- try (FloatDictionary floatDictionary = new FloatDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- floatDictionary.getStringValue(0), floatDictionary.getStringValue(length - 1));
- }
- break;
- case DOUBLE:
- try (DoubleDictionary doubleDictionary = new DoubleDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- doubleDictionary.getStringValue(0), doubleDictionary.getStringValue(length - 1));
- }
- break;
- case STRING:
- try (StringDictionary stringDictionary = new StringDictionary(dictionaryBuffer, length,
- columnMetadata.getColumnMaxLength())) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- stringDictionary.getStringValue(0), stringDictionary.getStringValue(length - 1));
- }
- break;
- case BYTES:
- try (BytesDictionary bytesDictionary = new BytesDictionary(dictionaryBuffer, length,
- columnMetadata.getColumnMaxLength())) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- bytesDictionary.getStringValue(0), bytesDictionary.getStringValue(length - 1));
- }
- break;
- default:
- throw new IllegalStateException("Unsupported data type: " + dataType + " for column: " + columnName);
+ if (columnMetadata.hasDictionary()) {
+ PinotDataBuffer dictionaryBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.dictionary());
+ int length = columnMetadata.getCardinality();
+ switch (dataType) {
+ case INT:
+ try (IntDictionary intDictionary = new IntDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ intDictionary.getStringValue(0), intDictionary.getStringValue(length - 1));
+ }
+ break;
+ case LONG:
+ try (LongDictionary longDictionary = new LongDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ longDictionary.getStringValue(0), longDictionary.getStringValue(length - 1));
+ }
+ break;
+ case FLOAT:
+ try (FloatDictionary floatDictionary = new FloatDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ floatDictionary.getStringValue(0), floatDictionary.getStringValue(length - 1));
+ }
+ break;
+ case DOUBLE:
+ try (DoubleDictionary doubleDictionary = new DoubleDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ doubleDictionary.getStringValue(0), doubleDictionary.getStringValue(length - 1));
+ }
+ break;
+ case STRING:
+ try (StringDictionary stringDictionary = new StringDictionary(dictionaryBuffer, length,
+ columnMetadata.getColumnMaxLength())) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ stringDictionary.getStringValue(0), stringDictionary.getStringValue(length - 1));
+ }
+ break;
+ case BYTES:
+ try (BytesDictionary bytesDictionary = new BytesDictionary(dictionaryBuffer, length,
+ columnMetadata.getColumnMaxLength())) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ bytesDictionary.getStringValue(0), bytesDictionary.getStringValue(length - 1));
+ }
+ break;
+ default:
+ throw new IllegalStateException("Unsupported data type: " + dataType + " for column: " + columnName);
+ }
+ } else if (_segmentMetadata.getSchema().getFieldSpecFor(columnName).getFieldType() == FieldSpec.FieldType.METRIC) {
+ // setting min/max for non-dictionary columns.
+ PinotDataBuffer forwardBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.forward());
+ switch (dataType) {
+ case INT:
+ try (FixedByteChunkSVForwardIndexReader rawIndexReader = new FixedByteChunkSVForwardIndexReader(forwardBuffer,
+ DataType.INT); ChunkReaderContext readerContext = rawIndexReader.createContext()) {
+ Integer[] minMaxValue = {Integer.MAX_VALUE, Integer.MIN_VALUE};
+ for (int docs = 0; docs < columnMetadata.getTotalDocs(); docs++) {
+ minMaxValue = getMinMaxValue(minMaxValue, rawIndexReader.getInt(docs, readerContext));
+ }
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ String.valueOf(minMaxValue[0]), String.valueOf(minMaxValue[1]));
+ }
+ break;
+ case LONG:
+ try (FixedByteChunkSVForwardIndexReader rawIndexReader = new FixedByteChunkSVForwardIndexReader(forwardBuffer,
+ DataType.LONG); ChunkReaderContext readerContext = rawIndexReader.createContext()) {
+ Long[] minMaxValue = {Long.MAX_VALUE, Long.MIN_VALUE};
+ for (int docs = 0; docs < columnMetadata.getTotalDocs(); docs++) {
+ minMaxValue = getMinMaxValue(minMaxValue, rawIndexReader.getLong(docs, readerContext));
+ }
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ String.valueOf(minMaxValue[0]), String.valueOf(minMaxValue[1]));
+ }
+ break;
+ case FLOAT:
+ try (FixedByteChunkSVForwardIndexReader rawIndexReader = new FixedByteChunkSVForwardIndexReader(forwardBuffer,
+ DataType.FLOAT); ChunkReaderContext readerContext = rawIndexReader.createContext()) {
+ Float[] minMaxValue = {Float.MAX_VALUE, Float.MIN_VALUE};
+ for (int docs = 0; docs < columnMetadata.getTotalDocs(); docs++) {
+ minMaxValue = getMinMaxValue(minMaxValue, rawIndexReader.getFloat(docs, readerContext));
+ }
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ String.valueOf(minMaxValue[0]), String.valueOf(minMaxValue[1]));
+ }
+ break;
+ case DOUBLE:
+ try (FixedByteChunkSVForwardIndexReader rawIndexReader = new FixedByteChunkSVForwardIndexReader(forwardBuffer,
+ DataType.DOUBLE); ChunkReaderContext readerContext = rawIndexReader.createContext()) {
+ Double[] minMaxValue = {Double.MAX_VALUE, Double.MIN_VALUE};
+ for (int docs = 0; docs < columnMetadata.getTotalDocs(); docs++) {
+ minMaxValue = getMinMaxValue(minMaxValue, rawIndexReader.getDouble(docs, readerContext));
+ }
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ String.valueOf(minMaxValue[0]), String.valueOf(minMaxValue[1]));
+ }
+ break;
+ default:
Review Comment:
I think we also want to support `STRING` and `BYTES`
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -570,12 +570,14 @@ public static void addColumnMetadataInfo(PropertiesConfiguration properties, Str
public static void addColumnMinMaxValueInfo(PropertiesConfiguration properties, String column, String minValue,
String maxValue) {
if (isValidPropertyValue(minValue)) {
- properties.setProperty(getKeyFor(column, MIN_VALUE), minValue);
+ properties.setProperty(getKeyFor(column, MIN_VALUE),
+ minValue.substring(0, Math.min(minValue.length(), METADATA_PROPERTY_LENGTH_LIMIT)));
} else {
properties.setProperty(getKeyFor(column, MIN_MAX_VALUE_INVALID), true);
}
if (isValidPropertyValue(maxValue)) {
- properties.setProperty(getKeyFor(column, MAX_VALUE), maxValue);
+ properties.setProperty(getKeyFor(column, MAX_VALUE),
+ maxValue.substring(0, Math.min(maxValue.length(), METADATA_PROPERTY_LENGTH_LIMIT)));
Review Comment:
Seems we still have invalid values. In that case, that's not fix the length issue now
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #10891: [WIP]: ensure min/max value generation in the segment metadata.
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10891:
URL: https://github.com/apache/pinot/pull/10891#issuecomment-1586004862
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/10891?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#10891](https://app.codecov.io/gh/apache/pinot/pull/10891?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (73eff90) into [master](https://app.codecov.io/gh/apache/pinot/commit/86b3b3e1c5bad5092f357358888ff28903ab05df?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (86b3b3e) will **increase** coverage by `0.00%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10891 +/- ##
=========================================
Coverage 0.11% 0.11%
=========================================
Files 2186 2132 -54
Lines 117493 114988 -2505
Branches 17762 17459 -303
=========================================
Hits 137 137
+ Misses 117336 114831 -2505
Partials 20 20
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1temurin11 | `?` | |
| integration1temurin17 | `?` | |
| integration1temurin20 | `?` | |
| integration2temurin11 | `?` | |
| integration2temurin17 | `?` | |
| integration2temurin20 | `?` | |
| unittests1temurin11 | `?` | |
| unittests1temurin17 | `?` | |
| unittests1temurin20 | `?` | |
| unittests2temurin11 | `?` | |
| unittests2temurin17 | `?` | |
| unittests2temurin20 | `0.11% <0.00%> (ø)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/pinot/pull/10891?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...ment/creator/impl/SegmentColumnarIndexCreator.java](https://app.codecov.io/gh/apache/pinot/pull/10891?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9TZWdtZW50Q29sdW1uYXJJbmRleENyZWF0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
... and [56 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/10891/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhioncbr commented on a diff in pull request #10891: Ensure min/max value generation in the segment metadata.
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on code in PR #10891:
URL: https://github.com/apache/pinot/pull/10891#discussion_r1244511327
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -570,12 +570,14 @@ public static void addColumnMetadataInfo(PropertiesConfiguration properties, Str
public static void addColumnMinMaxValueInfo(PropertiesConfiguration properties, String column, String minValue,
String maxValue) {
if (isValidPropertyValue(minValue)) {
- properties.setProperty(getKeyFor(column, MIN_VALUE), minValue);
+ properties.setProperty(getKeyFor(column, MIN_VALUE),
+ minValue.substring(0, Math.min(minValue.length(), METADATA_PROPERTY_LENGTH_LIMIT)));
} else {
properties.setProperty(getKeyFor(column, MIN_MAX_VALUE_INVALID), true);
}
if (isValidPropertyValue(maxValue)) {
- properties.setProperty(getKeyFor(column, MAX_VALUE), maxValue);
+ properties.setProperty(getKeyFor(column, MAX_VALUE),
+ maxValue.substring(0, Math.min(maxValue.length(), METADATA_PROPERTY_LENGTH_LIMIT)));
Review Comment:
Sure. I am going to raise a separate PR for invalid values. Thanks
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang merged pull request #10891: Ensure min/max value generation in the segment metadata.
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #10891:
URL: https://github.com/apache/pinot/pull/10891
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10891: [WIP]: ensure min/max value generation in the segment metadata.
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10891:
URL: https://github.com/apache/pinot/pull/10891#discussion_r1237916617
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/columnminmaxvalue/ColumnMinMaxValueGenerator.java:
##########
@@ -119,65 +121,126 @@ private List<String> getColumnsToAddMinMaxValue() {
private boolean needAddColumnMinMaxValueForColumn(String columnName) {
ColumnMetadata columnMetadata = _segmentMetadata.getColumnMetadataFor(columnName);
- return columnMetadata.hasDictionary() && columnMetadata.getMinValue() == null
+ return columnMetadata.getMinValue() == null
&& columnMetadata.getMaxValue() == null && !columnMetadata.isMinMaxValueInvalid();
}
private void addColumnMinMaxValueForColumn(String columnName)
throws Exception {
// Skip column without dictionary or with min/max value already set
ColumnMetadata columnMetadata = _segmentMetadata.getColumnMetadataFor(columnName);
- if (!columnMetadata.hasDictionary() || columnMetadata.getMinValue() != null
- || columnMetadata.getMaxValue() != null) {
+ if (columnMetadata.getMinValue() != null || columnMetadata.getMaxValue() != null) {
return;
}
- PinotDataBuffer dictionaryBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.dictionary());
DataType dataType = columnMetadata.getDataType().getStoredType();
- int length = columnMetadata.getCardinality();
- switch (dataType) {
- case INT:
- try (IntDictionary intDictionary = new IntDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- intDictionary.getStringValue(0), intDictionary.getStringValue(length - 1));
- }
- break;
- case LONG:
- try (LongDictionary longDictionary = new LongDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- longDictionary.getStringValue(0), longDictionary.getStringValue(length - 1));
- }
- break;
- case FLOAT:
- try (FloatDictionary floatDictionary = new FloatDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- floatDictionary.getStringValue(0), floatDictionary.getStringValue(length - 1));
- }
- break;
- case DOUBLE:
- try (DoubleDictionary doubleDictionary = new DoubleDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- doubleDictionary.getStringValue(0), doubleDictionary.getStringValue(length - 1));
- }
- break;
- case STRING:
- try (StringDictionary stringDictionary = new StringDictionary(dictionaryBuffer, length,
- columnMetadata.getColumnMaxLength())) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- stringDictionary.getStringValue(0), stringDictionary.getStringValue(length - 1));
- }
- break;
- case BYTES:
- try (BytesDictionary bytesDictionary = new BytesDictionary(dictionaryBuffer, length,
- columnMetadata.getColumnMaxLength())) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- bytesDictionary.getStringValue(0), bytesDictionary.getStringValue(length - 1));
- }
- break;
- default:
- throw new IllegalStateException("Unsupported data type: " + dataType + " for column: " + columnName);
+ if (columnMetadata.hasDictionary()) {
+ PinotDataBuffer dictionaryBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.dictionary());
+ int length = columnMetadata.getCardinality();
+ switch (dataType) {
+ case INT:
+ try (IntDictionary intDictionary = new IntDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ intDictionary.getStringValue(0), intDictionary.getStringValue(length - 1));
+ }
+ break;
+ case LONG:
+ try (LongDictionary longDictionary = new LongDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ longDictionary.getStringValue(0), longDictionary.getStringValue(length - 1));
+ }
+ break;
+ case FLOAT:
+ try (FloatDictionary floatDictionary = new FloatDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ floatDictionary.getStringValue(0), floatDictionary.getStringValue(length - 1));
+ }
+ break;
+ case DOUBLE:
+ try (DoubleDictionary doubleDictionary = new DoubleDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ doubleDictionary.getStringValue(0), doubleDictionary.getStringValue(length - 1));
+ }
+ break;
+ case STRING:
+ try (StringDictionary stringDictionary = new StringDictionary(dictionaryBuffer, length,
+ columnMetadata.getColumnMaxLength())) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ stringDictionary.getStringValue(0), stringDictionary.getStringValue(length - 1));
+ }
+ break;
+ case BYTES:
+ try (BytesDictionary bytesDictionary = new BytesDictionary(dictionaryBuffer, length,
+ columnMetadata.getColumnMaxLength())) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ bytesDictionary.getStringValue(0), bytesDictionary.getStringValue(length - 1));
+ }
+ break;
+ default:
+ throw new IllegalStateException("Unsupported data type: " + dataType + " for column: " + columnName);
+ }
+ } else {
+ // setting min/max for non-dictionary columns.
+ PinotDataBuffer forwardBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.forward());
Review Comment:
The approach is correct. We want to loop over all values from the forward index and gather the min/max value
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Ensure min/max value generation in the segment metadata. [pinot]
Posted by "jadami10 (via GitHub)" <gi...@apache.org>.
jadami10 commented on code in PR #10891:
URL: https://github.com/apache/pinot/pull/10891#discussion_r1503229608
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/columnminmaxvalue/ColumnMinMaxValueGenerator.java:
##########
@@ -119,65 +121,126 @@ private List<String> getColumnsToAddMinMaxValue() {
private boolean needAddColumnMinMaxValueForColumn(String columnName) {
ColumnMetadata columnMetadata = _segmentMetadata.getColumnMetadataFor(columnName);
- return columnMetadata.hasDictionary() && columnMetadata.getMinValue() == null
+ return columnMetadata.getMinValue() == null
&& columnMetadata.getMaxValue() == null && !columnMetadata.isMinMaxValueInvalid();
}
private void addColumnMinMaxValueForColumn(String columnName)
throws Exception {
// Skip column without dictionary or with min/max value already set
ColumnMetadata columnMetadata = _segmentMetadata.getColumnMetadataFor(columnName);
- if (!columnMetadata.hasDictionary() || columnMetadata.getMinValue() != null
- || columnMetadata.getMaxValue() != null) {
+ if (columnMetadata.getMinValue() != null || columnMetadata.getMaxValue() != null) {
return;
}
- PinotDataBuffer dictionaryBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.dictionary());
DataType dataType = columnMetadata.getDataType().getStoredType();
- int length = columnMetadata.getCardinality();
- switch (dataType) {
- case INT:
- try (IntDictionary intDictionary = new IntDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- intDictionary.getStringValue(0), intDictionary.getStringValue(length - 1));
- }
- break;
- case LONG:
- try (LongDictionary longDictionary = new LongDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- longDictionary.getStringValue(0), longDictionary.getStringValue(length - 1));
- }
- break;
- case FLOAT:
- try (FloatDictionary floatDictionary = new FloatDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- floatDictionary.getStringValue(0), floatDictionary.getStringValue(length - 1));
- }
- break;
- case DOUBLE:
- try (DoubleDictionary doubleDictionary = new DoubleDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- doubleDictionary.getStringValue(0), doubleDictionary.getStringValue(length - 1));
- }
- break;
- case STRING:
- try (StringDictionary stringDictionary = new StringDictionary(dictionaryBuffer, length,
- columnMetadata.getColumnMaxLength())) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- stringDictionary.getStringValue(0), stringDictionary.getStringValue(length - 1));
- }
- break;
- case BYTES:
- try (BytesDictionary bytesDictionary = new BytesDictionary(dictionaryBuffer, length,
- columnMetadata.getColumnMaxLength())) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- bytesDictionary.getStringValue(0), bytesDictionary.getStringValue(length - 1));
- }
- break;
- default:
- throw new IllegalStateException("Unsupported data type: " + dataType + " for column: " + columnName);
+ if (columnMetadata.hasDictionary()) {
+ PinotDataBuffer dictionaryBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.dictionary());
+ int length = columnMetadata.getCardinality();
+ switch (dataType) {
+ case INT:
+ try (IntDictionary intDictionary = new IntDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ intDictionary.getStringValue(0), intDictionary.getStringValue(length - 1));
+ }
+ break;
+ case LONG:
+ try (LongDictionary longDictionary = new LongDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ longDictionary.getStringValue(0), longDictionary.getStringValue(length - 1));
+ }
+ break;
+ case FLOAT:
+ try (FloatDictionary floatDictionary = new FloatDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ floatDictionary.getStringValue(0), floatDictionary.getStringValue(length - 1));
+ }
+ break;
+ case DOUBLE:
+ try (DoubleDictionary doubleDictionary = new DoubleDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ doubleDictionary.getStringValue(0), doubleDictionary.getStringValue(length - 1));
+ }
+ break;
+ case STRING:
+ try (StringDictionary stringDictionary = new StringDictionary(dictionaryBuffer, length,
+ columnMetadata.getColumnMaxLength())) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ stringDictionary.getStringValue(0), stringDictionary.getStringValue(length - 1));
+ }
+ break;
+ case BYTES:
+ try (BytesDictionary bytesDictionary = new BytesDictionary(dictionaryBuffer, length,
+ columnMetadata.getColumnMaxLength())) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ bytesDictionary.getStringValue(0), bytesDictionary.getStringValue(length - 1));
+ }
+ break;
+ default:
+ throw new IllegalStateException("Unsupported data type: " + dataType + " for column: " + columnName);
+ }
+ } else if (_segmentMetadata.getSchema().getFieldSpecFor(columnName).getFieldType() == FieldSpec.FieldType.METRIC) {
+ // setting min/max for non-dictionary columns.
+ PinotDataBuffer forwardBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.forward());
+ switch (dataType) {
+ case INT:
+ try (FixedByteChunkSVForwardIndexReader rawIndexReader = new FixedByteChunkSVForwardIndexReader(forwardBuffer,
+ DataType.INT); ChunkReaderContext readerContext = rawIndexReader.createContext()) {
+ Integer[] minMaxValue = {Integer.MAX_VALUE, Integer.MIN_VALUE};
+ for (int docs = 0; docs < columnMetadata.getTotalDocs(); docs++) {
+ minMaxValue = getMinMaxValue(minMaxValue, rawIndexReader.getInt(docs, readerContext));
+ }
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ String.valueOf(minMaxValue[0]), String.valueOf(minMaxValue[1]));
+ }
+ break;
+ case LONG:
+ try (FixedByteChunkSVForwardIndexReader rawIndexReader = new FixedByteChunkSVForwardIndexReader(forwardBuffer,
+ DataType.LONG); ChunkReaderContext readerContext = rawIndexReader.createContext()) {
+ Long[] minMaxValue = {Long.MAX_VALUE, Long.MIN_VALUE};
+ for (int docs = 0; docs < columnMetadata.getTotalDocs(); docs++) {
+ minMaxValue = getMinMaxValue(minMaxValue, rawIndexReader.getLong(docs, readerContext));
+ }
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ String.valueOf(minMaxValue[0]), String.valueOf(minMaxValue[1]));
+ }
+ break;
+ case FLOAT:
+ try (FixedByteChunkSVForwardIndexReader rawIndexReader = new FixedByteChunkSVForwardIndexReader(forwardBuffer,
+ DataType.FLOAT); ChunkReaderContext readerContext = rawIndexReader.createContext()) {
+ Float[] minMaxValue = {Float.MAX_VALUE, Float.MIN_VALUE};
+ for (int docs = 0; docs < columnMetadata.getTotalDocs(); docs++) {
+ minMaxValue = getMinMaxValue(minMaxValue, rawIndexReader.getFloat(docs, readerContext));
+ }
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ String.valueOf(minMaxValue[0]), String.valueOf(minMaxValue[1]));
+ }
+ break;
+ case DOUBLE:
+ try (FixedByteChunkSVForwardIndexReader rawIndexReader = new FixedByteChunkSVForwardIndexReader(forwardBuffer,
+ DataType.DOUBLE); ChunkReaderContext readerContext = rawIndexReader.createContext()) {
+ Double[] minMaxValue = {Double.MAX_VALUE, Double.MIN_VALUE};
+ for (int docs = 0; docs < columnMetadata.getTotalDocs(); docs++) {
+ minMaxValue = getMinMaxValue(minMaxValue, rawIndexReader.getDouble(docs, readerContext));
+ }
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ String.valueOf(minMaxValue[0]), String.valueOf(minMaxValue[1]));
+ }
+ break;
+ default:
Review Comment:
I think we're somewhat uniquely affected because of our use of metric aggregation which requires noDictionary.
previously, this code had
```
if (!columnMetadata.hasDictionary() || columnMetadata.getMinValue() != null
|| columnMetadata.getMaxValue() != null) {
return;
}
```
this PR removed the `hasDictionary` check and extended the min/max populating to those columns. But it also carried over the exception. Even for the previous branch, do we need to throw an exception for unsupported data types? It seems we can just not set the min/max.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10891: Ensure min/max value generation in the segment metadata.
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10891:
URL: https://github.com/apache/pinot/pull/10891#discussion_r1243059714
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/columnminmaxvalue/ColumnMinMaxValueGenerator.java:
##########
@@ -119,65 +121,126 @@ private List<String> getColumnsToAddMinMaxValue() {
private boolean needAddColumnMinMaxValueForColumn(String columnName) {
ColumnMetadata columnMetadata = _segmentMetadata.getColumnMetadataFor(columnName);
- return columnMetadata.hasDictionary() && columnMetadata.getMinValue() == null
+ return columnMetadata.getMinValue() == null
&& columnMetadata.getMaxValue() == null && !columnMetadata.isMinMaxValueInvalid();
}
private void addColumnMinMaxValueForColumn(String columnName)
throws Exception {
// Skip column without dictionary or with min/max value already set
ColumnMetadata columnMetadata = _segmentMetadata.getColumnMetadataFor(columnName);
- if (!columnMetadata.hasDictionary() || columnMetadata.getMinValue() != null
- || columnMetadata.getMaxValue() != null) {
+ if (columnMetadata.getMinValue() != null || columnMetadata.getMaxValue() != null) {
return;
}
- PinotDataBuffer dictionaryBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.dictionary());
DataType dataType = columnMetadata.getDataType().getStoredType();
- int length = columnMetadata.getCardinality();
- switch (dataType) {
- case INT:
- try (IntDictionary intDictionary = new IntDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- intDictionary.getStringValue(0), intDictionary.getStringValue(length - 1));
- }
- break;
- case LONG:
- try (LongDictionary longDictionary = new LongDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- longDictionary.getStringValue(0), longDictionary.getStringValue(length - 1));
- }
- break;
- case FLOAT:
- try (FloatDictionary floatDictionary = new FloatDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- floatDictionary.getStringValue(0), floatDictionary.getStringValue(length - 1));
- }
- break;
- case DOUBLE:
- try (DoubleDictionary doubleDictionary = new DoubleDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- doubleDictionary.getStringValue(0), doubleDictionary.getStringValue(length - 1));
- }
- break;
- case STRING:
- try (StringDictionary stringDictionary = new StringDictionary(dictionaryBuffer, length,
- columnMetadata.getColumnMaxLength())) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- stringDictionary.getStringValue(0), stringDictionary.getStringValue(length - 1));
- }
- break;
- case BYTES:
- try (BytesDictionary bytesDictionary = new BytesDictionary(dictionaryBuffer, length,
- columnMetadata.getColumnMaxLength())) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- bytesDictionary.getStringValue(0), bytesDictionary.getStringValue(length - 1));
- }
- break;
- default:
- throw new IllegalStateException("Unsupported data type: " + dataType + " for column: " + columnName);
+ if (columnMetadata.hasDictionary()) {
+ PinotDataBuffer dictionaryBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.dictionary());
+ int length = columnMetadata.getCardinality();
+ switch (dataType) {
+ case INT:
+ try (IntDictionary intDictionary = new IntDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ intDictionary.getStringValue(0), intDictionary.getStringValue(length - 1));
+ }
+ break;
+ case LONG:
+ try (LongDictionary longDictionary = new LongDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ longDictionary.getStringValue(0), longDictionary.getStringValue(length - 1));
+ }
+ break;
+ case FLOAT:
+ try (FloatDictionary floatDictionary = new FloatDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ floatDictionary.getStringValue(0), floatDictionary.getStringValue(length - 1));
+ }
+ break;
+ case DOUBLE:
+ try (DoubleDictionary doubleDictionary = new DoubleDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ doubleDictionary.getStringValue(0), doubleDictionary.getStringValue(length - 1));
+ }
+ break;
+ case STRING:
+ try (StringDictionary stringDictionary = new StringDictionary(dictionaryBuffer, length,
+ columnMetadata.getColumnMaxLength())) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ stringDictionary.getStringValue(0), stringDictionary.getStringValue(length - 1));
+ }
+ break;
+ case BYTES:
+ try (BytesDictionary bytesDictionary = new BytesDictionary(dictionaryBuffer, length,
+ columnMetadata.getColumnMaxLength())) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ bytesDictionary.getStringValue(0), bytesDictionary.getStringValue(length - 1));
+ }
+ break;
+ default:
+ throw new IllegalStateException("Unsupported data type: " + dataType + " for column: " + columnName);
+ }
+ } else if (_segmentMetadata.getSchema().getFieldSpecFor(columnName).getFieldType() == FieldSpec.FieldType.METRIC) {
Review Comment:
Good point. For MV fields, we want to collect the min/max value of all the elements within the array. I.e. loop over the values and collect the min/max
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10891: Ensure min/max value generation in the segment metadata.
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10891:
URL: https://github.com/apache/pinot/pull/10891#discussion_r1245946338
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -83,7 +83,7 @@ public class SegmentColumnarIndexCreator implements SegmentCreator {
// TODO Refactor class name to match interface name
private static final Logger LOGGER = LoggerFactory.getLogger(SegmentColumnarIndexCreator.class);
// Allow at most 512 characters for the metadata property
- private static final int METADATA_PROPERTY_LENGTH_LIMIT = 512;
+ static final int METADATA_PROPERTY_LENGTH_LIMIT = 512;
Review Comment:
Let's revert this change and we can discuss it under #10990
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/columnminmaxvalue/ColumnMinMaxValueGenerator.java:
##########
@@ -119,65 +123,153 @@ private List<String> getColumnsToAddMinMaxValue() {
private boolean needAddColumnMinMaxValueForColumn(String columnName) {
ColumnMetadata columnMetadata = _segmentMetadata.getColumnMetadataFor(columnName);
- return columnMetadata.hasDictionary() && columnMetadata.getMinValue() == null
+ return columnMetadata.getMinValue() == null
Review Comment:
(nit) reformat
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/columnminmaxvalue/ColumnMinMaxValueGenerator.java:
##########
@@ -119,65 +123,153 @@ private List<String> getColumnsToAddMinMaxValue() {
private boolean needAddColumnMinMaxValueForColumn(String columnName) {
ColumnMetadata columnMetadata = _segmentMetadata.getColumnMetadataFor(columnName);
- return columnMetadata.hasDictionary() && columnMetadata.getMinValue() == null
+ return columnMetadata.getMinValue() == null
&& columnMetadata.getMaxValue() == null && !columnMetadata.isMinMaxValueInvalid();
}
private void addColumnMinMaxValueForColumn(String columnName)
throws Exception {
// Skip column without dictionary or with min/max value already set
ColumnMetadata columnMetadata = _segmentMetadata.getColumnMetadataFor(columnName);
- if (!columnMetadata.hasDictionary() || columnMetadata.getMinValue() != null
- || columnMetadata.getMaxValue() != null) {
+ if (columnMetadata.getMinValue() != null || columnMetadata.getMaxValue() != null) {
return;
}
- PinotDataBuffer dictionaryBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.dictionary());
DataType dataType = columnMetadata.getDataType().getStoredType();
- int length = columnMetadata.getCardinality();
- switch (dataType) {
- case INT:
- try (IntDictionary intDictionary = new IntDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- intDictionary.getStringValue(0), intDictionary.getStringValue(length - 1));
- }
- break;
- case LONG:
- try (LongDictionary longDictionary = new LongDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- longDictionary.getStringValue(0), longDictionary.getStringValue(length - 1));
- }
- break;
- case FLOAT:
- try (FloatDictionary floatDictionary = new FloatDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- floatDictionary.getStringValue(0), floatDictionary.getStringValue(length - 1));
- }
- break;
- case DOUBLE:
- try (DoubleDictionary doubleDictionary = new DoubleDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- doubleDictionary.getStringValue(0), doubleDictionary.getStringValue(length - 1));
- }
- break;
- case STRING:
- try (StringDictionary stringDictionary = new StringDictionary(dictionaryBuffer, length,
- columnMetadata.getColumnMaxLength())) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- stringDictionary.getStringValue(0), stringDictionary.getStringValue(length - 1));
- }
- break;
- case BYTES:
- try (BytesDictionary bytesDictionary = new BytesDictionary(dictionaryBuffer, length,
- columnMetadata.getColumnMaxLength())) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- bytesDictionary.getStringValue(0), bytesDictionary.getStringValue(length - 1));
- }
- break;
- default:
- throw new IllegalStateException("Unsupported data type: " + dataType + " for column: " + columnName);
+ if (columnMetadata.hasDictionary()) {
+ PinotDataBuffer dictionaryBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.dictionary());
+ int length = columnMetadata.getCardinality();
+ switch (dataType) {
+ case INT:
+ try (IntDictionary intDictionary = new IntDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ intDictionary.getStringValue(0), intDictionary.getStringValue(length - 1));
+ }
+ break;
+ case LONG:
+ try (LongDictionary longDictionary = new LongDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ longDictionary.getStringValue(0), longDictionary.getStringValue(length - 1));
+ }
+ break;
+ case FLOAT:
+ try (FloatDictionary floatDictionary = new FloatDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ floatDictionary.getStringValue(0), floatDictionary.getStringValue(length - 1));
+ }
+ break;
+ case DOUBLE:
+ try (DoubleDictionary doubleDictionary = new DoubleDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ doubleDictionary.getStringValue(0), doubleDictionary.getStringValue(length - 1));
+ }
+ break;
+ case STRING:
+ try (StringDictionary stringDictionary = new StringDictionary(dictionaryBuffer, length,
Review Comment:
(code format) Does this follow [Pinot Style](https://docs.pinot.apache.org/developers/developers-and-contributors/code-setup#setup-ide)?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/columnminmaxvalue/ColumnMinMaxValueGenerator.java:
##########
@@ -119,65 +123,153 @@ private List<String> getColumnsToAddMinMaxValue() {
private boolean needAddColumnMinMaxValueForColumn(String columnName) {
ColumnMetadata columnMetadata = _segmentMetadata.getColumnMetadataFor(columnName);
- return columnMetadata.hasDictionary() && columnMetadata.getMinValue() == null
+ return columnMetadata.getMinValue() == null
&& columnMetadata.getMaxValue() == null && !columnMetadata.isMinMaxValueInvalid();
}
private void addColumnMinMaxValueForColumn(String columnName)
throws Exception {
// Skip column without dictionary or with min/max value already set
ColumnMetadata columnMetadata = _segmentMetadata.getColumnMetadataFor(columnName);
- if (!columnMetadata.hasDictionary() || columnMetadata.getMinValue() != null
- || columnMetadata.getMaxValue() != null) {
+ if (columnMetadata.getMinValue() != null || columnMetadata.getMaxValue() != null) {
return;
}
- PinotDataBuffer dictionaryBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.dictionary());
DataType dataType = columnMetadata.getDataType().getStoredType();
- int length = columnMetadata.getCardinality();
- switch (dataType) {
- case INT:
- try (IntDictionary intDictionary = new IntDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- intDictionary.getStringValue(0), intDictionary.getStringValue(length - 1));
- }
- break;
- case LONG:
- try (LongDictionary longDictionary = new LongDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- longDictionary.getStringValue(0), longDictionary.getStringValue(length - 1));
- }
- break;
- case FLOAT:
- try (FloatDictionary floatDictionary = new FloatDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- floatDictionary.getStringValue(0), floatDictionary.getStringValue(length - 1));
- }
- break;
- case DOUBLE:
- try (DoubleDictionary doubleDictionary = new DoubleDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- doubleDictionary.getStringValue(0), doubleDictionary.getStringValue(length - 1));
- }
- break;
- case STRING:
- try (StringDictionary stringDictionary = new StringDictionary(dictionaryBuffer, length,
- columnMetadata.getColumnMaxLength())) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- stringDictionary.getStringValue(0), stringDictionary.getStringValue(length - 1));
- }
- break;
- case BYTES:
- try (BytesDictionary bytesDictionary = new BytesDictionary(dictionaryBuffer, length,
- columnMetadata.getColumnMaxLength())) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- bytesDictionary.getStringValue(0), bytesDictionary.getStringValue(length - 1));
- }
- break;
- default:
- throw new IllegalStateException("Unsupported data type: " + dataType + " for column: " + columnName);
+ if (columnMetadata.hasDictionary()) {
+ PinotDataBuffer dictionaryBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.dictionary());
+ int length = columnMetadata.getCardinality();
+ switch (dataType) {
+ case INT:
+ try (IntDictionary intDictionary = new IntDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ intDictionary.getStringValue(0), intDictionary.getStringValue(length - 1));
+ }
+ break;
+ case LONG:
+ try (LongDictionary longDictionary = new LongDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ longDictionary.getStringValue(0), longDictionary.getStringValue(length - 1));
+ }
+ break;
+ case FLOAT:
+ try (FloatDictionary floatDictionary = new FloatDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ floatDictionary.getStringValue(0), floatDictionary.getStringValue(length - 1));
+ }
+ break;
+ case DOUBLE:
+ try (DoubleDictionary doubleDictionary = new DoubleDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ doubleDictionary.getStringValue(0), doubleDictionary.getStringValue(length - 1));
+ }
+ break;
+ case STRING:
+ try (StringDictionary stringDictionary = new StringDictionary(dictionaryBuffer, length,
+ columnMetadata.getColumnMaxLength())) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ stringDictionary.getStringValue(0), stringDictionary.getStringValue(length - 1));
+ }
+ break;
+ case BYTES:
+ try (BytesDictionary bytesDictionary = new BytesDictionary(dictionaryBuffer, length,
+ columnMetadata.getColumnMaxLength())) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ bytesDictionary.getStringValue(0), bytesDictionary.getStringValue(length - 1));
+ }
+ break;
+ default:
+ throw new IllegalStateException("Unsupported data type: " + dataType + " for column: " + columnName);
+ }
+ } else if (_segmentMetadata.getSchema().getFieldSpecFor(columnName).isSingleValueField()) {
+ // setting min/max for non-dictionary columns.
+ PinotDataBuffer forwardBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.forward());
+ switch (dataType) {
+ case INT:
+ try (FixedByteChunkSVForwardIndexReader rawIndexReader = new FixedByteChunkSVForwardIndexReader(forwardBuffer,
+ DataType.INT); ChunkReaderContext readerContext = rawIndexReader.createContext()) {
+ Integer[] minMaxValue = {Integer.MAX_VALUE, Integer.MIN_VALUE};
+ for (int docs = 0; docs < columnMetadata.getTotalDocs(); docs++) {
+ minMaxValue = getMinMaxValue(minMaxValue, rawIndexReader.getInt(docs, readerContext));
+ }
Review Comment:
Suggest directly comparing the primitive value to avoid the boxing/unboxing. Boxing/unboxing has quite expensive overhead. Same for other types
```suggestion
int min = Integer.MAX_VALUE;
int max = Integer.MIN_VALUE;
int numDocs = columnMetadata.getTotalDocs();
for (int i = 0; i < numDocs; i++) {
int value = rawIndexReader.getInt(docs, readerContext);
min = Math.min(min, value);
max = Math.max(max, value);
}
```
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhioncbr commented on pull request #10891: Ensure min/max value generation in the segment metadata.
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on PR #10891:
URL: https://github.com/apache/pinot/pull/10891#issuecomment-1615296482
@Jackie-Jiang, please have a look. Thanks
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhioncbr commented on a diff in pull request #10891: [WIP]: ensure min/max value generation in the segment metadata.
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on code in PR #10891:
URL: https://github.com/apache/pinot/pull/10891#discussion_r1233335139
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/columnminmaxvalue/ColumnMinMaxValueGenerator.java:
##########
@@ -119,65 +121,126 @@ private List<String> getColumnsToAddMinMaxValue() {
private boolean needAddColumnMinMaxValueForColumn(String columnName) {
ColumnMetadata columnMetadata = _segmentMetadata.getColumnMetadataFor(columnName);
- return columnMetadata.hasDictionary() && columnMetadata.getMinValue() == null
+ return columnMetadata.getMinValue() == null
&& columnMetadata.getMaxValue() == null && !columnMetadata.isMinMaxValueInvalid();
}
private void addColumnMinMaxValueForColumn(String columnName)
throws Exception {
// Skip column without dictionary or with min/max value already set
ColumnMetadata columnMetadata = _segmentMetadata.getColumnMetadataFor(columnName);
- if (!columnMetadata.hasDictionary() || columnMetadata.getMinValue() != null
- || columnMetadata.getMaxValue() != null) {
+ if (columnMetadata.getMinValue() != null || columnMetadata.getMaxValue() != null) {
return;
}
- PinotDataBuffer dictionaryBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.dictionary());
DataType dataType = columnMetadata.getDataType().getStoredType();
- int length = columnMetadata.getCardinality();
- switch (dataType) {
- case INT:
- try (IntDictionary intDictionary = new IntDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- intDictionary.getStringValue(0), intDictionary.getStringValue(length - 1));
- }
- break;
- case LONG:
- try (LongDictionary longDictionary = new LongDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- longDictionary.getStringValue(0), longDictionary.getStringValue(length - 1));
- }
- break;
- case FLOAT:
- try (FloatDictionary floatDictionary = new FloatDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- floatDictionary.getStringValue(0), floatDictionary.getStringValue(length - 1));
- }
- break;
- case DOUBLE:
- try (DoubleDictionary doubleDictionary = new DoubleDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- doubleDictionary.getStringValue(0), doubleDictionary.getStringValue(length - 1));
- }
- break;
- case STRING:
- try (StringDictionary stringDictionary = new StringDictionary(dictionaryBuffer, length,
- columnMetadata.getColumnMaxLength())) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- stringDictionary.getStringValue(0), stringDictionary.getStringValue(length - 1));
- }
- break;
- case BYTES:
- try (BytesDictionary bytesDictionary = new BytesDictionary(dictionaryBuffer, length,
- columnMetadata.getColumnMaxLength())) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- bytesDictionary.getStringValue(0), bytesDictionary.getStringValue(length - 1));
- }
- break;
- default:
- throw new IllegalStateException("Unsupported data type: " + dataType + " for column: " + columnName);
+ if (columnMetadata.hasDictionary()) {
+ PinotDataBuffer dictionaryBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.dictionary());
+ int length = columnMetadata.getCardinality();
+ switch (dataType) {
+ case INT:
+ try (IntDictionary intDictionary = new IntDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ intDictionary.getStringValue(0), intDictionary.getStringValue(length - 1));
+ }
+ break;
+ case LONG:
+ try (LongDictionary longDictionary = new LongDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ longDictionary.getStringValue(0), longDictionary.getStringValue(length - 1));
+ }
+ break;
+ case FLOAT:
+ try (FloatDictionary floatDictionary = new FloatDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ floatDictionary.getStringValue(0), floatDictionary.getStringValue(length - 1));
+ }
+ break;
+ case DOUBLE:
+ try (DoubleDictionary doubleDictionary = new DoubleDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ doubleDictionary.getStringValue(0), doubleDictionary.getStringValue(length - 1));
+ }
+ break;
+ case STRING:
+ try (StringDictionary stringDictionary = new StringDictionary(dictionaryBuffer, length,
+ columnMetadata.getColumnMaxLength())) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ stringDictionary.getStringValue(0), stringDictionary.getStringValue(length - 1));
+ }
+ break;
+ case BYTES:
+ try (BytesDictionary bytesDictionary = new BytesDictionary(dictionaryBuffer, length,
+ columnMetadata.getColumnMaxLength())) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ bytesDictionary.getStringValue(0), bytesDictionary.getStringValue(length - 1));
+ }
+ break;
+ default:
+ throw new IllegalStateException("Unsupported data type: " + dataType + " for column: " + columnName);
+ }
+ } else {
+ // setting min/max for non-dictionary columns.
+ PinotDataBuffer forwardBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.forward());
Review Comment:
@Jackie-Jiang Can you please confirm my approach?
I am creating the `PinotDataBuffer` based on the forward index and then reading the by iterating over the doc.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10891: [WIP]: ensure min/max value generation in the segment metadata.
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10891:
URL: https://github.com/apache/pinot/pull/10891#discussion_r1232863110
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -570,12 +570,14 @@ public static void addColumnMetadataInfo(PropertiesConfiguration properties, Str
public static void addColumnMinMaxValueInfo(PropertiesConfiguration properties, String column, String minValue,
String maxValue) {
if (isValidPropertyValue(minValue)) {
- properties.setProperty(getKeyFor(column, MIN_VALUE), minValue);
+ properties.setProperty(getKeyFor(column, MIN_VALUE),
+ minValue.substring(0, Math.min(minValue.length(), METADATA_PROPERTY_LENGTH_LIMIT)));
} else {
properties.setProperty(getKeyFor(column, MIN_MAX_VALUE_INVALID), true);
}
if (isValidPropertyValue(maxValue)) {
- properties.setProperty(getKeyFor(column, MAX_VALUE), maxValue);
+ properties.setProperty(getKeyFor(column, MAX_VALUE),
+ maxValue.substring(0, Math.min(maxValue.length(), METADATA_PROPERTY_LENGTH_LIMIT)));
Review Comment:
We cannot directly use substring as the max value because it is smaller than the actual max value and can cause false positive pruning. We want to pick a string that is larger than the actual max value
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10891: Ensure min/max value generation in the segment metadata.
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10891:
URL: https://github.com/apache/pinot/pull/10891#discussion_r1248658614
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/columnminmaxvalue/ColumnMinMaxValueGenerator.java:
##########
@@ -119,65 +126,257 @@ private List<String> getColumnsToAddMinMaxValue() {
private boolean needAddColumnMinMaxValueForColumn(String columnName) {
ColumnMetadata columnMetadata = _segmentMetadata.getColumnMetadataFor(columnName);
- return columnMetadata.hasDictionary() && columnMetadata.getMinValue() == null
- && columnMetadata.getMaxValue() == null && !columnMetadata.isMinMaxValueInvalid();
+ return columnMetadata.getMinValue() == null && columnMetadata.getMaxValue() == null
+ && !columnMetadata.isMinMaxValueInvalid();
}
private void addColumnMinMaxValueForColumn(String columnName)
throws Exception {
- // Skip column without dictionary or with min/max value already set
+ // Skip column with min/max value already set
ColumnMetadata columnMetadata = _segmentMetadata.getColumnMetadataFor(columnName);
- if (!columnMetadata.hasDictionary() || columnMetadata.getMinValue() != null
- || columnMetadata.getMaxValue() != null) {
+ if (columnMetadata.getMinValue() != null || columnMetadata.getMaxValue() != null) {
return;
}
- PinotDataBuffer dictionaryBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.dictionary());
DataType dataType = columnMetadata.getDataType().getStoredType();
- int length = columnMetadata.getCardinality();
- switch (dataType) {
- case INT:
- try (IntDictionary intDictionary = new IntDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- intDictionary.getStringValue(0), intDictionary.getStringValue(length - 1));
- }
- break;
- case LONG:
- try (LongDictionary longDictionary = new LongDictionary(dictionaryBuffer, length)) {
+ if (columnMetadata.hasDictionary()) {
+ PinotDataBuffer dictionaryBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.dictionary());
+ int length = columnMetadata.getCardinality();
+ switch (dataType) {
+ case INT:
+ try (IntDictionary intDictionary = new IntDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ intDictionary.getStringValue(0), intDictionary.getStringValue(length - 1));
+ }
+ break;
+ case LONG:
+ try (LongDictionary longDictionary = new LongDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ longDictionary.getStringValue(0), longDictionary.getStringValue(length - 1));
+ }
+ break;
+ case FLOAT:
+ try (FloatDictionary floatDictionary = new FloatDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ floatDictionary.getStringValue(0), floatDictionary.getStringValue(length - 1));
+ }
+ break;
+ case DOUBLE:
+ try (DoubleDictionary doubleDictionary = new DoubleDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ doubleDictionary.getStringValue(0), doubleDictionary.getStringValue(length - 1));
+ }
+ break;
+ case STRING:
+ try (StringDictionary stringDictionary = new StringDictionary(dictionaryBuffer, length,
+ columnMetadata.getColumnMaxLength())) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ stringDictionary.getStringValue(0), stringDictionary.getStringValue(length - 1));
+ }
+ break;
+ case BYTES:
+ try (BytesDictionary bytesDictionary = new BytesDictionary(dictionaryBuffer, length,
+ columnMetadata.getColumnMaxLength())) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ bytesDictionary.getStringValue(0), bytesDictionary.getStringValue(length - 1));
+ }
+ break;
+ default:
+ throw new IllegalStateException("Unsupported data type: " + dataType + " for column: " + columnName);
+ }
+ } else {
+ // setting min/max for non-dictionary columns.
+ int numDocs = columnMetadata.getTotalDocs();
+ boolean isSingleValueField = _segmentMetadata.getSchema().getFieldSpecFor(columnName).isSingleValueField();
+ PinotDataBuffer forwardBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.forward());
+ switch (dataType) {
+ case INT: {
+ int min = Integer.MAX_VALUE;
+ int max = Integer.MIN_VALUE;
+ if (isSingleValueField) {
+ FixedByteChunkSVForwardIndexReader rawIndexReader = new FixedByteChunkSVForwardIndexReader(forwardBuffer,
+ DataType.INT);
+ ChunkReaderContext readerContext = rawIndexReader.createContext();
Review Comment:
We want to close both the reader and the context. You may use the try-with-resource block
```suggestion
try (FixedByteChunkSVForwardIndexReader rawIndexReader = new FixedByteChunkSVForwardIndexReader(forwardBuffer,
DataType.INT);
ChunkReaderContext readerContext = rawIndexReader.createContext()
) {
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/columnminmaxvalue/ColumnMinMaxValueGenerator.java:
##########
@@ -119,65 +126,257 @@ private List<String> getColumnsToAddMinMaxValue() {
private boolean needAddColumnMinMaxValueForColumn(String columnName) {
ColumnMetadata columnMetadata = _segmentMetadata.getColumnMetadataFor(columnName);
- return columnMetadata.hasDictionary() && columnMetadata.getMinValue() == null
- && columnMetadata.getMaxValue() == null && !columnMetadata.isMinMaxValueInvalid();
+ return columnMetadata.getMinValue() == null && columnMetadata.getMaxValue() == null
+ && !columnMetadata.isMinMaxValueInvalid();
}
private void addColumnMinMaxValueForColumn(String columnName)
throws Exception {
- // Skip column without dictionary or with min/max value already set
+ // Skip column with min/max value already set
ColumnMetadata columnMetadata = _segmentMetadata.getColumnMetadataFor(columnName);
- if (!columnMetadata.hasDictionary() || columnMetadata.getMinValue() != null
- || columnMetadata.getMaxValue() != null) {
+ if (columnMetadata.getMinValue() != null || columnMetadata.getMaxValue() != null) {
return;
}
- PinotDataBuffer dictionaryBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.dictionary());
DataType dataType = columnMetadata.getDataType().getStoredType();
- int length = columnMetadata.getCardinality();
- switch (dataType) {
- case INT:
- try (IntDictionary intDictionary = new IntDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- intDictionary.getStringValue(0), intDictionary.getStringValue(length - 1));
- }
- break;
- case LONG:
- try (LongDictionary longDictionary = new LongDictionary(dictionaryBuffer, length)) {
+ if (columnMetadata.hasDictionary()) {
+ PinotDataBuffer dictionaryBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.dictionary());
+ int length = columnMetadata.getCardinality();
+ switch (dataType) {
+ case INT:
+ try (IntDictionary intDictionary = new IntDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ intDictionary.getStringValue(0), intDictionary.getStringValue(length - 1));
+ }
+ break;
+ case LONG:
+ try (LongDictionary longDictionary = new LongDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ longDictionary.getStringValue(0), longDictionary.getStringValue(length - 1));
+ }
+ break;
+ case FLOAT:
+ try (FloatDictionary floatDictionary = new FloatDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ floatDictionary.getStringValue(0), floatDictionary.getStringValue(length - 1));
+ }
+ break;
+ case DOUBLE:
+ try (DoubleDictionary doubleDictionary = new DoubleDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ doubleDictionary.getStringValue(0), doubleDictionary.getStringValue(length - 1));
+ }
+ break;
+ case STRING:
+ try (StringDictionary stringDictionary = new StringDictionary(dictionaryBuffer, length,
+ columnMetadata.getColumnMaxLength())) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ stringDictionary.getStringValue(0), stringDictionary.getStringValue(length - 1));
+ }
+ break;
+ case BYTES:
+ try (BytesDictionary bytesDictionary = new BytesDictionary(dictionaryBuffer, length,
+ columnMetadata.getColumnMaxLength())) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ bytesDictionary.getStringValue(0), bytesDictionary.getStringValue(length - 1));
+ }
+ break;
+ default:
+ throw new IllegalStateException("Unsupported data type: " + dataType + " for column: " + columnName);
+ }
+ } else {
+ // setting min/max for non-dictionary columns.
+ int numDocs = columnMetadata.getTotalDocs();
+ boolean isSingleValueField = _segmentMetadata.getSchema().getFieldSpecFor(columnName).isSingleValueField();
+ PinotDataBuffer forwardBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.forward());
+ switch (dataType) {
+ case INT: {
+ int min = Integer.MAX_VALUE;
+ int max = Integer.MIN_VALUE;
+ if (isSingleValueField) {
+ FixedByteChunkSVForwardIndexReader rawIndexReader = new FixedByteChunkSVForwardIndexReader(forwardBuffer,
+ DataType.INT);
+ ChunkReaderContext readerContext = rawIndexReader.createContext();
+ for (int docs = 0; docs < numDocs; docs++) {
Review Comment:
(nit) use `i` or `docId`
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Ensure min/max value generation in the segment metadata. [pinot]
Posted by "jadami10 (via GitHub)" <gi...@apache.org>.
jadami10 commented on code in PR #10891:
URL: https://github.com/apache/pinot/pull/10891#discussion_r1503020298
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/columnminmaxvalue/ColumnMinMaxValueGenerator.java:
##########
@@ -119,65 +121,126 @@ private List<String> getColumnsToAddMinMaxValue() {
private boolean needAddColumnMinMaxValueForColumn(String columnName) {
ColumnMetadata columnMetadata = _segmentMetadata.getColumnMetadataFor(columnName);
- return columnMetadata.hasDictionary() && columnMetadata.getMinValue() == null
+ return columnMetadata.getMinValue() == null
&& columnMetadata.getMaxValue() == null && !columnMetadata.isMinMaxValueInvalid();
}
private void addColumnMinMaxValueForColumn(String columnName)
throws Exception {
// Skip column without dictionary or with min/max value already set
ColumnMetadata columnMetadata = _segmentMetadata.getColumnMetadataFor(columnName);
- if (!columnMetadata.hasDictionary() || columnMetadata.getMinValue() != null
- || columnMetadata.getMaxValue() != null) {
+ if (columnMetadata.getMinValue() != null || columnMetadata.getMaxValue() != null) {
return;
}
- PinotDataBuffer dictionaryBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.dictionary());
DataType dataType = columnMetadata.getDataType().getStoredType();
- int length = columnMetadata.getCardinality();
- switch (dataType) {
- case INT:
- try (IntDictionary intDictionary = new IntDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- intDictionary.getStringValue(0), intDictionary.getStringValue(length - 1));
- }
- break;
- case LONG:
- try (LongDictionary longDictionary = new LongDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- longDictionary.getStringValue(0), longDictionary.getStringValue(length - 1));
- }
- break;
- case FLOAT:
- try (FloatDictionary floatDictionary = new FloatDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- floatDictionary.getStringValue(0), floatDictionary.getStringValue(length - 1));
- }
- break;
- case DOUBLE:
- try (DoubleDictionary doubleDictionary = new DoubleDictionary(dictionaryBuffer, length)) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- doubleDictionary.getStringValue(0), doubleDictionary.getStringValue(length - 1));
- }
- break;
- case STRING:
- try (StringDictionary stringDictionary = new StringDictionary(dictionaryBuffer, length,
- columnMetadata.getColumnMaxLength())) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- stringDictionary.getStringValue(0), stringDictionary.getStringValue(length - 1));
- }
- break;
- case BYTES:
- try (BytesDictionary bytesDictionary = new BytesDictionary(dictionaryBuffer, length,
- columnMetadata.getColumnMaxLength())) {
- SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
- bytesDictionary.getStringValue(0), bytesDictionary.getStringValue(length - 1));
- }
- break;
- default:
- throw new IllegalStateException("Unsupported data type: " + dataType + " for column: " + columnName);
+ if (columnMetadata.hasDictionary()) {
+ PinotDataBuffer dictionaryBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.dictionary());
+ int length = columnMetadata.getCardinality();
+ switch (dataType) {
+ case INT:
+ try (IntDictionary intDictionary = new IntDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ intDictionary.getStringValue(0), intDictionary.getStringValue(length - 1));
+ }
+ break;
+ case LONG:
+ try (LongDictionary longDictionary = new LongDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ longDictionary.getStringValue(0), longDictionary.getStringValue(length - 1));
+ }
+ break;
+ case FLOAT:
+ try (FloatDictionary floatDictionary = new FloatDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ floatDictionary.getStringValue(0), floatDictionary.getStringValue(length - 1));
+ }
+ break;
+ case DOUBLE:
+ try (DoubleDictionary doubleDictionary = new DoubleDictionary(dictionaryBuffer, length)) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ doubleDictionary.getStringValue(0), doubleDictionary.getStringValue(length - 1));
+ }
+ break;
+ case STRING:
+ try (StringDictionary stringDictionary = new StringDictionary(dictionaryBuffer, length,
+ columnMetadata.getColumnMaxLength())) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ stringDictionary.getStringValue(0), stringDictionary.getStringValue(length - 1));
+ }
+ break;
+ case BYTES:
+ try (BytesDictionary bytesDictionary = new BytesDictionary(dictionaryBuffer, length,
+ columnMetadata.getColumnMaxLength())) {
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ bytesDictionary.getStringValue(0), bytesDictionary.getStringValue(length - 1));
+ }
+ break;
+ default:
+ throw new IllegalStateException("Unsupported data type: " + dataType + " for column: " + columnName);
+ }
+ } else if (_segmentMetadata.getSchema().getFieldSpecFor(columnName).getFieldType() == FieldSpec.FieldType.METRIC) {
+ // setting min/max for non-dictionary columns.
+ PinotDataBuffer forwardBuffer = _segmentWriter.getIndexFor(columnName, StandardIndexes.forward());
+ switch (dataType) {
+ case INT:
+ try (FixedByteChunkSVForwardIndexReader rawIndexReader = new FixedByteChunkSVForwardIndexReader(forwardBuffer,
+ DataType.INT); ChunkReaderContext readerContext = rawIndexReader.createContext()) {
+ Integer[] minMaxValue = {Integer.MAX_VALUE, Integer.MIN_VALUE};
+ for (int docs = 0; docs < columnMetadata.getTotalDocs(); docs++) {
+ minMaxValue = getMinMaxValue(minMaxValue, rawIndexReader.getInt(docs, readerContext));
+ }
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ String.valueOf(minMaxValue[0]), String.valueOf(minMaxValue[1]));
+ }
+ break;
+ case LONG:
+ try (FixedByteChunkSVForwardIndexReader rawIndexReader = new FixedByteChunkSVForwardIndexReader(forwardBuffer,
+ DataType.LONG); ChunkReaderContext readerContext = rawIndexReader.createContext()) {
+ Long[] minMaxValue = {Long.MAX_VALUE, Long.MIN_VALUE};
+ for (int docs = 0; docs < columnMetadata.getTotalDocs(); docs++) {
+ minMaxValue = getMinMaxValue(minMaxValue, rawIndexReader.getLong(docs, readerContext));
+ }
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ String.valueOf(minMaxValue[0]), String.valueOf(minMaxValue[1]));
+ }
+ break;
+ case FLOAT:
+ try (FixedByteChunkSVForwardIndexReader rawIndexReader = new FixedByteChunkSVForwardIndexReader(forwardBuffer,
+ DataType.FLOAT); ChunkReaderContext readerContext = rawIndexReader.createContext()) {
+ Float[] minMaxValue = {Float.MAX_VALUE, Float.MIN_VALUE};
+ for (int docs = 0; docs < columnMetadata.getTotalDocs(); docs++) {
+ minMaxValue = getMinMaxValue(minMaxValue, rawIndexReader.getFloat(docs, readerContext));
+ }
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ String.valueOf(minMaxValue[0]), String.valueOf(minMaxValue[1]));
+ }
+ break;
+ case DOUBLE:
+ try (FixedByteChunkSVForwardIndexReader rawIndexReader = new FixedByteChunkSVForwardIndexReader(forwardBuffer,
+ DataType.DOUBLE); ChunkReaderContext readerContext = rawIndexReader.createContext()) {
+ Double[] minMaxValue = {Double.MAX_VALUE, Double.MIN_VALUE};
+ for (int docs = 0; docs < columnMetadata.getTotalDocs(); docs++) {
+ minMaxValue = getMinMaxValue(minMaxValue, rawIndexReader.getDouble(docs, readerContext));
+ }
+ SegmentColumnarIndexCreator.addColumnMinMaxValueInfo(_segmentProperties, columnName,
+ String.valueOf(minMaxValue[0]), String.valueOf(minMaxValue[1]));
+ }
+ break;
+ default:
Review Comment:
our attempted upgrade broke here because `BIG_DECIMAL` isn't supported. I'm curious why we choose to throw an error for an unsupported data type when we were seemingly ok with this data not being populated.
But more importantly, there's some wider issue here that we're missing entire data types, and no testing is catching it.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org