xiangfu0 commented on code in PR #17269:
URL: https://github.com/apache/pinot/pull/17269#discussion_r3057666939
##########
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:
This change was already reverted in a subsequent commit. The current code
uses createOfflineTableConfig() in the integration test instead.
##########
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:
Addressed: the _useDictionary flag is now computed on the caller side (in
putColumnValueReaderFor) and passed to the ColumnValueReader constructor,
rather than being derived inside ColumnValueReader itself.
##########
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:
Fixed: the signature now takes IndexConfig (the per-index-type config)
instead of FieldIndexConfigs.
--
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]
