Jackie-Jiang commented on code in PR #17269:
URL: https://github.com/apache/pinot/pull/17269#discussion_r2632875697
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/IndexType.java:
##########
@@ -119,6 +119,13 @@ default IR getIndexReader(ColumnIndexContainer
indexContainer) {
IndexHandler createIndexHandler(SegmentDirectory segmentDirectory,
Map<String, FieldIndexConfigs> configsByCol,
Schema schema, TableConfig tableConfig);
+ /**
+ * Indicates whether this index requires a dictionary to function for the
provided column.
+ */
+ default boolean requiresDictionary(FieldSpec fieldSpec, FieldIndexConfigs
fieldIndexConfigs) {
Review Comment:
Should it take the per index type `IndexConfig` instead of
`FieldIndexConfigs`?
##########
pinot-controller/src/test/java/org/apache/pinot/controller/helix/ControllerTest.java:
##########
@@ -798,15 +798,33 @@ public void toggleTableState(String tableName, TableType
type, boolean enable)
}
public TableConfig getOfflineTableConfig(String tableName) {
- TableConfig offlineTableConfig =
_helixResourceManager.getOfflineTableConfig(tableName);
- assertNotNull(offlineTableConfig);
- return offlineTableConfig;
+ if (_helixResourceManager != null) {
Review Comment:
I don't follow this change. This basically means controller is not available
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexType.java:
##########
@@ -166,6 +166,14 @@ public String getPrettyName() {
return INDEX_DISPLAY_NAME;
}
+ @Override
+ protected ColumnConfigDeserializer<ForwardIndexConfig> createDeserializer() {
Review Comment:
What's the difference between this one and the default impl?
##########
pinot-core/src/main/java/org/apache/pinot/core/common/DataFetcher.java:
##########
@@ -309,13 +309,15 @@ private class ColumnValueReader implements AutoCloseable {
final Dictionary _dictionary;
final DataType _storedType;
final boolean _singleValue;
+ final boolean _useDictionary;
boolean _readerContextCreated;
ForwardIndexReaderContext _readerContext;
ColumnValueReader(ForwardIndexReader reader, @Nullable Dictionary
dictionary) {
_reader = reader;
_dictionary = dictionary;
+ _useDictionary = _reader.isDictionaryEncoded() && _dictionary != null;
Review Comment:
(minor) Suggest putting this check on the caller side. We can also consider
splitting it into 2 readers, one for dictionary encoded and one for raw
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexType.java:
##########
@@ -235,19 +246,22 @@ protected IndexReaderFactory<ForwardIndexReader>
createReaderFactory() {
return ForwardIndexReaderFactory.getInstance();
}
- public String getFileExtension(ColumnMetadata columnMetadata) {
+ public List<String> getFileExtension(ColumnMetadata columnMetadata) {
Review Comment:
What is this for?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexType.java:
##########
@@ -203,6 +211,9 @@ private boolean isDisabled(Map<String, String> props) {
private ForwardIndexConfig createConfigFromFieldConfig(FieldConfig
fieldConfig) {
ForwardIndexConfig.Builder builder = new ForwardIndexConfig.Builder();
builder.withCompressionCodec(fieldConfig.getCompressionCodec());
+ if (fieldConfig.getEncodingType() == FieldConfig.EncodingType.RAW) {
Review Comment:
Is this a bug fix?
--
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]
