Jackie-Jiang commented on code in PR #16845:
URL: https://github.com/apache/pinot/pull/16845#discussion_r2403624093
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/IndexingConfig.java:
##########
@@ -65,6 +65,10 @@ public class IndexingConfig extends BaseJsonConfig {
private boolean _nullHandlingEnabled;
private boolean _columnMajorSegmentBuilderEnabled = true;
private boolean _skipSegmentPreprocess;
+ // If set to true, uses NoDictColumnStatisticsCollector for stats collection
for no-dictionary columns.
+ // Once we are confident about the stability of
NoDictColumnStatisticsCollector,
+ // we can enable it by default and deprecate this config
+ private boolean _optimiseNoDictStatsCollection = false;
Review Comment:
We usually use `optimize` over `optimise`. Let's keep it consistent. Same
for other places
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/stats/MapColumnPreIndexStatsCollector.java:
##########
@@ -51,16 +54,24 @@ public class MapColumnPreIndexStatsCollector extends
AbstractColumnStatisticsCol
private final Object2ObjectOpenHashMap<String,
AbstractColumnStatisticsCollector> _keyStats =
new Object2ObjectOpenHashMap<>(INITIAL_HASH_SET_SIZE);
private final Map<String, Integer> _keyFrequencies = new
Object2ObjectOpenHashMap<>(INITIAL_HASH_SET_SIZE);
- private String[] _sortedValues;
+ private String[] _sortedKeys;
private int _minLength = Integer.MAX_VALUE;
private int _maxLength = 0;
private boolean _sealed = false;
private ComplexFieldSpec _colFieldSpec;
+ private boolean _createNoDictCollectorsForKeys = false;
public MapColumnPreIndexStatsCollector(String column, StatsCollectorConfig
statsCollectorConfig) {
super(column, statsCollectorConfig);
_sorted = false;
_colFieldSpec = (ComplexFieldSpec)
statsCollectorConfig.getFieldSpecForColumn(column);
+ Map<String, FieldIndexConfigs> indexConfigsByCol =
FieldIndexConfigsUtil.createIndexConfigsByColName(
Review Comment:
@raghavyadav01 For MAP type, do we have config for each key column? Do we
apply the same setting to all child columns?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/stats/NoDictColumnStatisticsCollector.java:
##########
@@ -0,0 +1,232 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.segment.creator.impl.stats;
+
+import com.clearspring.analytics.stream.cardinality.HyperLogLogPlus;
+import java.math.BigDecimal;
+import java.nio.charset.StandardCharsets;
+import org.apache.commons.lang3.NotImplementedException;
+import org.apache.pinot.segment.spi.creator.StatsCollectorConfig;
+import org.apache.pinot.spi.utils.BigDecimalUtils;
+import org.apache.pinot.spi.utils.ByteArray;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Column statistics collector for no-dictionary columns that avoids storing
unique values and thus reduces memory
+ * Behavior:
+ * - getUniqueValuesSet() throws NotImplementedException
+ * - getCardinality() returns approximate cardinality using HLL++
+ * - Doesn't handle cases where values are of different types (e.g. int and
long). This is expected.
+ * Individual type collectors (e.g. IntColumnPreIndexStatsCollector) also
don't handle this case.
+ * At this point in the Pinot process, the type consistency of a key should
already be enforced.
+ * So if such a case is encountered, it will be raised as an exception
during collect()
+ * Doesn't handle MAP data type as MapColumnPreIndexStatsCollector is
optimized for no-dictionary collection
+ */
+@SuppressWarnings({"rawtypes"})
+public class NoDictColumnStatisticsCollector extends
AbstractColumnStatisticsCollector {
+ private static final Logger LOGGER =
LoggerFactory.getLogger(NoDictColumnStatisticsCollector.class);
+ private Comparable _minValue;
+ private Comparable _maxValue;
+ private int _minLength = Integer.MAX_VALUE;
+ private int _maxLength = -1; // default return value is -1
+ private int _maxRowLength = -1; // default return value is -1
+ private boolean _sealed = false;
+ // HLL Plus generally returns approximate cardinality >= actual cardinality
which is desired
+ private final HyperLogLogPlus _hllPlus;
+
+ public NoDictColumnStatisticsCollector(String column, StatsCollectorConfig
statsCollectorConfig) {
+ super(column, statsCollectorConfig);
+ // Use default p and sp; can be made configurable via StatsCollectorConfig
later if needed
+ _hllPlus = new HyperLogLogPlus(
+ CommonConstants.Helix.DEFAULT_HYPERLOGLOG_PLUS_P,
+ CommonConstants.Helix.DEFAULT_HYPERLOGLOG_PLUS_SP);
+ LOGGER.info("Initialized NoDictColumnStatisticsCollector for column: {}",
column);
+ }
+
+ @Override
+ public void collect(Object entry) {
+ assert !_sealed;
+ if (entry instanceof Object[]) {
+ Object[] values = (Object[]) entry;
+ int rowLength = 0;
+ for (Object value : values) {
+ if (value instanceof BigDecimal) {
+ // BigDecimalColumnPreIndexStatsCollector doesn't support multi-value
Review Comment:
To be more accurate, Pinot doesn't support multi-value BigDecimal as of now
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/stats/NoDictColumnStatisticsCollector.java:
##########
@@ -0,0 +1,232 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.segment.creator.impl.stats;
+
+import com.clearspring.analytics.stream.cardinality.HyperLogLogPlus;
+import java.math.BigDecimal;
+import java.nio.charset.StandardCharsets;
+import org.apache.commons.lang3.NotImplementedException;
+import org.apache.pinot.segment.spi.creator.StatsCollectorConfig;
+import org.apache.pinot.spi.utils.BigDecimalUtils;
+import org.apache.pinot.spi.utils.ByteArray;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Column statistics collector for no-dictionary columns that avoids storing
unique values and thus reduces memory
+ * Behavior:
+ * - getUniqueValuesSet() throws NotImplementedException
+ * - getCardinality() returns approximate cardinality using HLL++
+ * - Doesn't handle cases where values are of different types (e.g. int and
long). This is expected.
+ * Individual type collectors (e.g. IntColumnPreIndexStatsCollector) also
don't handle this case.
+ * At this point in the Pinot process, the type consistency of a key should
already be enforced.
+ * So if such a case is encountered, it will be raised as an exception
during collect()
+ * Doesn't handle MAP data type as MapColumnPreIndexStatsCollector is
optimized for no-dictionary collection
+ */
+@SuppressWarnings({"rawtypes"})
+public class NoDictColumnStatisticsCollector extends
AbstractColumnStatisticsCollector {
+ private static final Logger LOGGER =
LoggerFactory.getLogger(NoDictColumnStatisticsCollector.class);
+ private Comparable _minValue;
+ private Comparable _maxValue;
+ private int _minLength = Integer.MAX_VALUE;
+ private int _maxLength = -1; // default return value is -1
+ private int _maxRowLength = -1; // default return value is -1
+ private boolean _sealed = false;
+ // HLL Plus generally returns approximate cardinality >= actual cardinality
which is desired
+ private final HyperLogLogPlus _hllPlus;
+
+ public NoDictColumnStatisticsCollector(String column, StatsCollectorConfig
statsCollectorConfig) {
+ super(column, statsCollectorConfig);
+ // Use default p and sp; can be made configurable via StatsCollectorConfig
later if needed
+ _hllPlus = new HyperLogLogPlus(
+ CommonConstants.Helix.DEFAULT_HYPERLOGLOG_PLUS_P,
+ CommonConstants.Helix.DEFAULT_HYPERLOGLOG_PLUS_SP);
+ LOGGER.info("Initialized NoDictColumnStatisticsCollector for column: {}",
column);
+ }
+
+ @Override
+ public void collect(Object entry) {
+ assert !_sealed;
+ if (entry instanceof Object[]) {
+ Object[] values = (Object[]) entry;
+ int rowLength = 0;
+ for (Object value : values) {
+ if (value instanceof BigDecimal) {
+ // BigDecimalColumnPreIndexStatsCollector doesn't support multi-value
+ throw new UnsupportedOperationException();
+ }
+ updateMinMax(value);
+ updateHllPlus(value);
+ int len = getValueLength(value);
+ _minLength = Math.min(_minLength, len);
+ _maxLength = Math.max(_maxLength, len);
+ rowLength += len;
+ }
+ _maxNumberOfMultiValues = Math.max(_maxNumberOfMultiValues,
values.length);
+ _maxRowLength = Math.max(_maxRowLength, rowLength);
+ updateTotalNumberOfEntries(values);
+ } else if (entry instanceof int[] || entry instanceof long[]
+ || entry instanceof float[] || entry instanceof double[]) {
+ // Native multi-value types don't require length calculation because
they're not variable-length
+ int length;
+ if (entry instanceof int[]) {
+ int[] values = (int[]) entry;
+ for (int value : values) {
+ updateMinMax(value);
+ updateHllPlus(value);
+ }
+ length = values.length;
+ } else if (entry instanceof long[]) {
+ long[] values = (long[]) entry;
+ for (long value : values) {
+ updateMinMax(value);
+ updateHllPlus(value);
+ }
+ length = values.length;
+ } else if (entry instanceof float[]) {
+ float[] values = (float[]) entry;
+ for (float value : values) {
+ updateMinMax(value);
+ updateHllPlus(value);
+ }
+ length = values.length;
+ } else {
+ double[] values = (double[]) entry;
+ for (double value : values) {
+ updateMinMax(value);
+ updateHllPlus(value);
+ }
+ length = values.length;
+ }
+ _maxNumberOfMultiValues = Math.max(_maxNumberOfMultiValues, length);
+ updateTotalNumberOfEntries(length);
+ } else {
+ Comparable value = toComparable(entry);
+ addressSorted(value);
+ updateMinMax(entry);
+ updateHllPlus(entry);
+ int len = getValueLength(entry);
+ _minLength = Math.min(_minLength, len);
+ _maxLength = Math.max(_maxLength, len);
+ if (isPartitionEnabled()) {
+ updatePartition(value.toString());
+ }
+ _maxRowLength = Math.max(_maxRowLength, len);
+ _totalNumberOfEntries++;
+ }
+ }
+
+ private void updateMinMax(Object value) {
+ Comparable comp = toComparable(value);
+ if (_minValue == null || comp.compareTo(_minValue) < 0) {
+ _minValue = comp;
+ }
+ if (_maxValue == null || comp.compareTo(_maxValue) > 0) {
+ _maxValue = comp;
+ }
+ }
+
+ private Comparable toComparable(Object value) {
+ if (value instanceof byte[]) {
+ return new ByteArray((byte[]) value);
+ }
+ if (value instanceof Comparable) {
+ return (Comparable) value;
+ }
+ throw new IllegalStateException("Unsupported value type " +
value.getClass());
+ }
+
+ private int getValueLength(Object value) {
+ if (value instanceof byte[]) {
+ return ((byte[]) value).length;
+ }
+ if (value instanceof CharSequence) {
+ return ((CharSequence)
value).toString().getBytes(StandardCharsets.UTF_8).length;
+ }
+ if (value instanceof BigDecimal) {
+ return BigDecimalUtils.byteSize((BigDecimal) value);
+ }
+ if (value instanceof Number) {
+ return 8; // fixed-width approximation as it's not actually required for
numeric fields which are of fixed length
+ }
+ throw new IllegalStateException("Unsupported value type " +
value.getClass());
+ }
+
+ @Override
+ public Object getMinValue() {
+ if (_sealed) {
+ return _minValue;
+ }
+ throw new IllegalStateException("you must seal the collector first before
asking for min value");
+ }
+
+ @Override
+ public Object getMaxValue() {
+ if (_sealed) {
+ return _maxValue;
+ }
+ throw new IllegalStateException("you must seal the collector first before
asking for max value");
+ }
+
+ @Override
+ public Object getUniqueValuesSet() {
+ throw new NotImplementedException("getUniqueValuesSet is not supported in
NoDictColumnStatisticsCollector");
Review Comment:
Let's return `null` here (same as `MutableNoDictionaryColStatistics`). We
should also annotate the return value as `@Nullable` for the interface
##########
pinot-controller/src/test/resources/memory_estimation/table-config.json:
##########
@@ -36,7 +36,8 @@
"stream.kafka.decoder.class.name":
"com.linkedin.pinot.v2.server.LiKafkaDecoder",
"stream.kafka.topic.name": "UserGeneratedContentGestureCountEvent",
"streamType": "kafka"
- }
+ },
+ "optimiseNoDictStatsCollection": false
Review Comment:
(nit) Seems not needed
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/ColumnIndexCreationInfo.java:
##########
@@ -64,11 +65,23 @@ public Object getMax() {
}
public Object getSortedUniqueElementsArray() {
- return _columnStatistics.getUniqueValuesSet();
+ try {
+ return _columnStatistics.getUniqueValuesSet();
+ } catch (NotImplementedException e) {
+ return null;
+ }
}
public int getDistinctValueCount() {
- Object uniqueValArray = _columnStatistics.getUniqueValuesSet();
+ Object uniqueValArray;
Review Comment:
I think we can always `return _columnStatistics.getCardinality();`
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/stats/NoDictColumnStatisticsCollector.java:
##########
@@ -0,0 +1,232 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.segment.creator.impl.stats;
+
+import com.clearspring.analytics.stream.cardinality.HyperLogLogPlus;
+import java.math.BigDecimal;
+import java.nio.charset.StandardCharsets;
+import org.apache.commons.lang3.NotImplementedException;
+import org.apache.pinot.segment.spi.creator.StatsCollectorConfig;
+import org.apache.pinot.spi.utils.BigDecimalUtils;
+import org.apache.pinot.spi.utils.ByteArray;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Column statistics collector for no-dictionary columns that avoids storing
unique values and thus reduces memory
+ * Behavior:
+ * - getUniqueValuesSet() throws NotImplementedException
+ * - getCardinality() returns approximate cardinality using HLL++
+ * - Doesn't handle cases where values are of different types (e.g. int and
long). This is expected.
+ * Individual type collectors (e.g. IntColumnPreIndexStatsCollector) also
don't handle this case.
+ * At this point in the Pinot process, the type consistency of a key should
already be enforced.
+ * So if such a case is encountered, it will be raised as an exception
during collect()
+ * Doesn't handle MAP data type as MapColumnPreIndexStatsCollector is
optimized for no-dictionary collection
+ */
+@SuppressWarnings({"rawtypes"})
+public class NoDictColumnStatisticsCollector extends
AbstractColumnStatisticsCollector {
Review Comment:
Suggest splitting this class based on the column type. Mixing all the data
types in one class can make it error prone, also less efficient
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
