Jackie-Jiang commented on code in PR #15591:
URL: https://github.com/apache/pinot/pull/15591#discussion_r2096596015
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImpl.java:
##########
@@ -355,7 +415,9 @@ public static class Builder {
private PartitionFunction _partitionFunction;
private Set<Integer> _partitions;
private boolean _autoGenerated;
- private Map<IndexType<?, ?, ?>, Long> _indexSizeMap = new HashMap<>();
+
+ // use non-default size to save space for most columns
+ LongArrayList _indexSizeList = new LongArrayList(2);
Review Comment:
(minor) Make it `private`
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/ColumnMetadata.java:
##########
@@ -90,7 +92,40 @@ default boolean isMinMaxValueInvalid() {
@Nullable
Set<Integer> getPartitions();
- Map<IndexType<?, ?, ?>, Long> getIndexSizeMap();
+ /**
+ * If exists, returns size of index for given index type; otherwise returns
-1.
+ * @param type index type
+ */
+ default long getIndexSizeFor(IndexType type) {
Review Comment:
If this is just for testing purpose, consider annotating it with
`@VisibleForTesting` and add javadoc warning future developers to not use it
because of the overhead
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/ColumnMetadata.java:
##########
@@ -90,7 +90,23 @@ default boolean isMinMaxValueInvalid() {
@Nullable
Set<Integer> getPartitions();
- Map<IndexType<?, ?, ?>, Long> getIndexSizeMap();
+ long getIndexSizeFor(IndexType type);
+
+ default void addIndexSize(short indexType, long size) {
Review Comment:
This is an interface, and all the APIs are read only.
Consider adding it only to `ColumnMetadataImpl.Builder`
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImpl.java:
##########
@@ -355,7 +415,9 @@ public static class Builder {
private PartitionFunction _partitionFunction;
private Set<Integer> _partitions;
private boolean _autoGenerated;
- private Map<IndexType<?, ?, ?>, Long> _indexSizeMap = new HashMap<>();
+
+ // use non-default size to save space for most columns
+ LongArrayList _indexSizeList = new LongArrayList(2);
Review Comment:
With builder pattern, we want to initialize everything in the builder, and
not modify the class once `build()` is invoked. Right now the update happens in
the main class, instead of the builder
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImpl.java:
##########
@@ -164,10 +176,58 @@ public Set<Integer> getPartitions() {
return _partitions;
}
- @Nullable
@Override
- public Map<IndexType<?, ?, ?>, Long> getIndexSizeMap() {
- return _indexSizeMap;
+ public long getIndexSizeFor(IndexType type) {
+ short indexId = IndexService.getInstance().getNumericId(type);
+ for (int i = 0, n = getNumIndexes(); i < n; i++) {
Review Comment:
(minor) Can be improved by directly looping over `_indexTypeSizeList`
--
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]
