Jackie-Jiang commented on code in PR #10352:
URL: https://github.com/apache/pinot/pull/10352#discussion_r1122197569
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/IndexLoadingConfig.java:
##########
@@ -386,11 +387,15 @@ public void setSortedColumn(String sortedColumn) {
}
public Set<String> getInvertedIndexColumns() {
- return _invertedIndexColumns;
+ return unmodifiable(_invertedIndexColumns);
}
public Set<String> getRangeIndexColumns() {
- return _rangeIndexColumns;
+ return unmodifiable(_rangeIndexColumns);
+ }
+
+ public void addRangeIndexColumn(String... columns) {
+ _rangeIndexColumns.addAll(Arrays.asList(columns));
Review Comment:
Suggest making at always taking one column, and return a boolean (whether it
is added)
```suggestion
public boolean addRangeIndexColumn(String column) {
_rangeIndexColumns.add(column);
```
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/SegmentGeneratorConfig.java:
##########
@@ -249,7 +250,13 @@ public SegmentGeneratorConfig(TableConfig tableConfig,
Schema schema) {
}
public Map<String, Map<String, String>> getColumnProperties() {
- return _columnProperties;
+ HashMap<String, Map<String, String>> copy = new HashMap<>();
Review Comment:
Making it immutable during read time can be quite expensive. We can make it
immutable during construction time
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/IndexLoadingConfig.java:
##########
@@ -410,35 +415,55 @@ public FSTType getFSTIndexType() {
* @return a set containing names of text index columns
*/
public Set<String> getTextIndexColumns() {
- return _textIndexColumns;
+ return unmodifiable(_textIndexColumns);
}
public Set<String> getFSTIndexColumns() {
- return _fstIndexColumns;
+ return unmodifiable(_fstIndexColumns);
}
public Map<String, JsonIndexConfig> getJsonIndexConfigs() {
- return _jsonIndexConfigs;
+ return unmodifiable(_jsonIndexConfigs);
}
public Map<String, H3IndexConfig> getH3IndexConfigs() {
- return _h3IndexConfigs;
+ return unmodifiable(_h3IndexConfigs);
}
public Map<String, Map<String, String>> getColumnProperties() {
- return _columnProperties;
+ return unmodifiable(_columnProperties);
}
public void setColumnProperties(Map<String, Map<String, String>>
columnProperties) {
- _columnProperties = columnProperties;
+ _columnProperties = new HashMap<>(columnProperties);
}
/**
* For tests only.
*/
@VisibleForTesting
public void setInvertedIndexColumns(Set<String> invertedIndexColumns) {
- _invertedIndexColumns = invertedIndexColumns;
+ _invertedIndexColumns = new HashSet<>(invertedIndexColumns);
+ }
+
+ @VisibleForTesting
+ public void addInvertedIndexColumns(String... invertedIndexColumns) {
+ _invertedIndexColumns.addAll(Arrays.asList(invertedIndexColumns));
+ }
Review Comment:
Same for other places. If user needs batch add, they can use the next method
```suggestion
public boolean addInvertedIndexColumn(String column) {
return _invertedIndexColumns.add(column);
}
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/IndexLoadingConfig.java:
##########
@@ -410,35 +415,55 @@ public FSTType getFSTIndexType() {
* @return a set containing names of text index columns
*/
public Set<String> getTextIndexColumns() {
- return _textIndexColumns;
+ return unmodifiable(_textIndexColumns);
}
public Set<String> getFSTIndexColumns() {
- return _fstIndexColumns;
+ return unmodifiable(_fstIndexColumns);
}
public Map<String, JsonIndexConfig> getJsonIndexConfigs() {
- return _jsonIndexConfigs;
+ return unmodifiable(_jsonIndexConfigs);
}
public Map<String, H3IndexConfig> getH3IndexConfigs() {
- return _h3IndexConfigs;
+ return unmodifiable(_h3IndexConfigs);
}
public Map<String, Map<String, String>> getColumnProperties() {
- return _columnProperties;
+ return unmodifiable(_columnProperties);
}
public void setColumnProperties(Map<String, Map<String, String>>
columnProperties) {
- _columnProperties = columnProperties;
+ _columnProperties = new HashMap<>(columnProperties);
}
/**
* For tests only.
*/
@VisibleForTesting
public void setInvertedIndexColumns(Set<String> invertedIndexColumns) {
- _invertedIndexColumns = invertedIndexColumns;
+ _invertedIndexColumns = new HashSet<>(invertedIndexColumns);
+ }
+
+ @VisibleForTesting
+ public void addInvertedIndexColumns(String... invertedIndexColumns) {
+ _invertedIndexColumns.addAll(Arrays.asList(invertedIndexColumns));
+ }
+
+ @VisibleForTesting
+ public void addInvertedIndexColumns(Collection<String> invertedIndexColumns)
{
+ _invertedIndexColumns.addAll(invertedIndexColumns);
+ }
+
+ @VisibleForTesting
+ public void removeInvertedIndexColumns(String... invertedIndexColumns) {
+ Arrays.asList(invertedIndexColumns).forEach(_invertedIndexColumns::remove);
+ }
Review Comment:
Same for other places
```suggestion
public boolean removeInvertedIndexColumn(String column) {
return _invertedIndexColumns.remove(column);
}
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/IndexLoadingConfig.java:
##########
@@ -386,11 +387,15 @@ public void setSortedColumn(String sortedColumn) {
}
public Set<String> getInvertedIndexColumns() {
- return _invertedIndexColumns;
+ return unmodifiable(_invertedIndexColumns);
}
public Set<String> getRangeIndexColumns() {
- return _rangeIndexColumns;
+ return unmodifiable(_rangeIndexColumns);
+ }
+
+ public void addRangeIndexColumn(String... columns) {
+ _rangeIndexColumns.addAll(Arrays.asList(columns));
Review Comment:
Seems we have another `addRangeIndexColumns` below
--
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]
