You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "klsince (via GitHub)" <gi...@apache.org> on 2023/04/05 18:13:20 UTC
[GitHub] [pinot] klsince opened a new pull request, #10553: [WIP] allow to overwrite index configs at tier level
klsince opened a new pull request, #10553:
URL: https://github.com/apache/pinot/pull/10553
This PR allows one to overwrite index configs at tier level, e.g. using many index for hot tier for better query perf but a lot less index for cold tier for more cost saving.
Still WIP, borrowing GH to grill the changes over the test cases
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] klsince commented on a diff in pull request #10553: Allow to overwrite index configs at tier level
Posted by "klsince (via GitHub)" <gi...@apache.org>.
klsince commented on code in PR #10553:
URL: https://github.com/apache/pinot/pull/10553#discussion_r1165998811
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/IndexLoadingConfig.java:
##########
@@ -308,6 +318,22 @@ private <C extends IndexConfig> ColumnConfigDeserializer<C> getDeserializer(Inde
return deserializer;
}
+ private TableConfig getTableConfigWithTierOverwrites() {
+ return (_segmentTier == null || _tableConfig == null) ? _tableConfig
+ : TableConfigUtils.overwriteTableConfigForTier(_tableConfig, _segmentTier);
+ }
+
+ private Schema inferSchema() {
+ if (_schema != null) {
+ return _schema;
+ }
+ Schema schema = new Schema();
+ for (String column : getAllKnownColumns()) {
+ schema.addField(new DimensionFieldSpec(column, FieldSpec.DataType.STRING, true));
Review Comment:
yeah, I was a bit confused while seeing this. This logic was there already and I didn't change how it's used other than moving it to this helper method.
The column names put in this dummy schema are used in some prod code. This dummy STRING type must not. It looks like the sole purpose of this dummy schema is to pass the column names to FieldIndexConfigsUtil.createIndexConfigsByColName(), when _schema is null (which can happen e.g. in MultipleTreesBuilder).
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] npawar commented on pull request #10553: Allow to overwrite index configs at tier level
Posted by "npawar (via GitHub)" <gi...@apache.org>.
npawar commented on PR #10553:
URL: https://github.com/apache/pinot/pull/10553#issuecomment-1504425318
@klsince would you please add some more description to the PR, giving examples of the final config you are going with (based on @gortiz feedback)?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on a diff in pull request #10553: Allow to overwrite index configs at tier level
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10553:
URL: https://github.com/apache/pinot/pull/10553#discussion_r1162384815
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/AbstractIndexType.java:
##########
@@ -48,8 +50,18 @@ public String getId() {
@Override
public Map<String, C> getConfig(TableConfig tableConfig, Schema schema) {
+ return getConfig(tableConfig, schema, null);
+ }
+
+ @Override
+ public Map<String, C> getConfig(TableConfig tableConfig, Schema schema, @Nullable String tier) {
+ if (tier != null) {
+ ColumnConfigDeserializer<C> deserializer =
+ _deserializersByTier.computeIfAbsent(tier, t -> createDeserializer(tier));
+ return deserializer.deserialize(tableConfig, schema);
+ }
if (_deserializer == null) {
- _deserializer = createDeserializer();
+ _deserializer = createDeserializer(null);
Review Comment:
So we are only caching the deserializer used when there is no tier? I think it would be better to never cache the deserializer or do it for all tiers.
For example, I would suggest to actually change ColumnCOnfigDeserializer in order to add the tier parameter there, so there would always be a single deserializer that is able to read any tier.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] saurabhd336 commented on pull request #10553: Allow to overwrite index configs at tier level
Posted by "saurabhd336 (via GitHub)" <gi...@apache.org>.
saurabhd336 commented on PR #10553:
URL: https://github.com/apache/pinot/pull/10553#issuecomment-1510775641
@klsince Is there a reason we're not setting `_tableConfig` to the TableConfigUtils.overwriteTableConfigForTier(_tableConfig, _segmentTier) inside IndexLoadingConfig?
Is there a reason we'd want to preserve original tableConfig?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] klsince commented on a diff in pull request #10553: Allow to overwrite index configs at tier level
Posted by "klsince (via GitHub)" <gi...@apache.org>.
klsince commented on code in PR #10553:
URL: https://github.com/apache/pinot/pull/10553#discussion_r1169266722
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/config/TableConfigUtils.java:
##########
@@ -331,6 +339,96 @@ public static void convertFromLegacyTableConfig(TableConfig tableConfig) {
validationConfig.setSegmentPushType(null);
}
+ /**
+ * Helper method to create a new TableConfig by overwriting the original TableConfig with tier specific configs, so
+ * that the consumers of TableConfig don't have to handle tier overwrites themselves. To begin with, we only
+ * consider to overwrite the index configs in `tableIndexConfig` and `fieldConfigList`, e.g.
+ *
+ * {
+ * "tableIndexConfig": {
+ * ... // configs allowed in IndexingConfig, for default tier
+ * "tierOverwrites": {
+ * "hotTier": {...}, // configs allowed in IndexingConfig, for hot tier
+ * "coldTier": {...} // configs allowed in IndexingConfig, for cold tier
+ * }
+ * }
+ * "fieldConfigList": [
+ * {
+ * ... // configs allowed in FieldConfig, for default tier
+ * "tierOverwrites": {
+ * "hotTier": {...}, // configs allowed in FieldConfig, for hot tier
+ * "coldTier": {...} // configs allowed in FieldConfig, for cold tier
+ * }
+ * },
+ * ...
+ * ]
+ * }
Review Comment:
The precedence between using fieldConfigList or tableIndexConfig is defined by IndexType.getConfig() from index-spi, which basically favors fieldConfigList. But this is the detail of IndexType.getConfig() and may get changed as needed in future. So changes in this PR tried to decouple IndexType.getConfig() from the logic of applying tier specific configs.
But for IndexType.getConfig() to access tier specific configs, we make a temp TableConfig by copying tier specific configs from `tierOverwrites` out and overwrite those defined for default tier, and then pass the TableConfig object to IndexType.getConfig().
> ... is the instruction to just repeat whatever json fields...
mostly right, but only for `top-level` config keys defined in IndexingConfig or FieldConfig, e.g. StarTreeIndexConfig is overwritten as a whole, instead of overwriting its inner specific fields.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on pull request #10553: Allow to overwrite index configs at tier level
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #10553:
URL: https://github.com/apache/pinot/pull/10553#issuecomment-1502856849
In order to add the ability to override configs by adding a tiered field, here you follow a pattern that is quite common in Pinot but I tried to avoid in `index-spi`. This pattern is the following:
1. We add a new concept that modify several different parts of the code.
2. This new concept change how different parts of the code behave.
3. We drag this concept until the specific parts of the code that are actually modified by the new concept.
A clear example of this (used in the index spi PEP) is the bloom filter config. The first Pinot versions that supported bloom filter expected to configure them in `bloomFilterColumns` attribute. Then we wanted to be able to configure the bloom filter on each column, so we added `bloomFilterConfigs`. Then each time we wanted to know the bloom filter config for a column we had to first look for the column in `bloomFIlterConfig` and in case we don't find it, look for it in `bloomFilterColumns` (and if we found it there we generate a default config). Not being centralized, we needed to do that several times in different parts of the code. This is a problem because each time we add a new concept like that we have to be sure we touch all the places where this logic is affected. In index-spi PEP I said that this is a problem I associate with not having different models for user specific syntax (in this case, the different ways a user may define a bloom filter) and internal and developer
oriented syntax (where we don't care about the different ways the user may configure a bloom filter, we just want them to be unified in a single model). `index-spi` solved this problem by extracting all the different ways an index can be configured in the TableConfig (customer specific syntax/model) and returning a single config object (developer specific model).
In this PR I think we are falling in the same pattern. The new concept is the ability to override configs by tier. The customer specific syntax is still the TableConfig and instead of centralize the transformation between how the user writes something and how the code reacts to it, we are again spreading the logic in several parts of the code. In this case, index types and startree indexes need to know about tiered (when in fact they don't actually care). IICU the code, encodingType is not actually being override, so we should also add this logic in at least another place.
Instead of doing this, I suggest to try to follow another pattern. For example:
1. We still add `tiered` concept in different places in TableConfig
2. We add a preprocessor layer where the original TableConfig with N tiered configs is transformed into N+1 TableConfig objects without tiered.
3. For each N+1 TableConfig, we apply the logic we have right now.
I think it is easier to see this with an example. In this PR you modifies the airlineStats_offline_table_config.json in order to add the following in FieldConfigList (I'm going to ignore the changes in Startree indexes, but it would also apply there):
```js
{
"name": "ArrTimeBlk",
"encodingType": "DICTIONARY",
"indexes": {
"inverted": {
"enabled": "true"
}
},
"tierOverwrites": {
"hotTier": {
"encodingType": "DICTIONARY",
"indexes": {
"bloom": {
"enabled": "true"
}
}
},
"coldTier": {
"encodingType": "RAW",
"indexes": {
"text": {
"enabled": "true"
}
}
}
}
```
This means that we have 2 tiers plus the default one, so the preprocessor would generate 3 tables that will be equal except for the FieldConfig named `ArrTimeBlk`, where we will have 3 different versions:
The default would be:
```js
"name": "ArrTimeBlk",
"encodingType": "DICTIONARY",
"indexes": {
"inverted": {
"enabled": "true"
}
}
```
The hotTier would be:
```js
"name": "ArrTimeBlk",
"encodingType": "DICTIONARY",
"indexes": {
"inverted": {
"enabled": "true"
},
"bloom": {
"enabled": "true"
}
}
```
The coldTier would be:
```js
"name": "ArrTimeBlk",
"encodingType": "RAW", // This was also modified!
"indexes": {
"inverted": {
"enabled": "true"
},
"indexes": {
"text": {
"enabled": "true"
}
}
}
```
In these 3 new TableConfigs there is no tierOverride, so we can read indexes (and any other config!) without actually needed to know about them.
The preprocessor should not be very complex. It needs to know how to merge jsons (which is not that difficult) and needs to verify that the thing that has been override is correct (for example in this case we could forbid to override the name).
In the abstract void, this approach seems more elegant than the other. The separation of responsibilities is clear: The preprocessor takes care of the tieredOverride, the IndexType.getConfig() take a TableConfig without overrides and generates an index config. The main issue here is that I don't know how much it may affect the rest of the code. In fact I would like to use different classes for TableConfigWithTieredOverride and TieredConfigWithoutOverride, but that would imply so many changes in the code that I don't think it would be feasible right now.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] klsince commented on a diff in pull request #10553: Allow to overwrite index configs at tier level
Posted by "klsince (via GitHub)" <gi...@apache.org>.
klsince commented on code in PR #10553:
URL: https://github.com/apache/pinot/pull/10553#discussion_r1163315275
##########
pinot-tools/src/main/resources/examples/batch/airlineStats/airlineStats_offline_table_config.json:
##########
@@ -23,6 +23,33 @@
"MONTH"
]
}
+ },
+ {
+ "name": "ArrTimeBlk",
+ "encodingType": "DICTIONARY",
+ "indexes": {
+ "inverted": {
+ "enabled": "true"
+ }
+ },
+ "tierOverwrites": {
+ "hotTier": {
+ "encodingType": "DICTIONARY",
+ "indexes": {
+ "bloom": {
+ "enabled": "true"
+ }
+ }
+ },
+ "coldTier": {
+ "encodingType": "RAW",
Review Comment:
It's allowed to overwrite encoding type, as done by `FieldConfig.withTierOverwrites(tier)` method, where the tier specific FieldConfig object is constructed and may use tier specific encoding type, indexes or properties.
DictionaryIndexType has this`fieldConfig = fieldConfig.withTierOverwrites(tier);` (which actually made tests of encodingType passed), but I missed it for ForwardIndexType.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] klsince commented on a diff in pull request #10553: Allow to overwrite index configs at tier level
Posted by "klsince (via GitHub)" <gi...@apache.org>.
klsince commented on code in PR #10553:
URL: https://github.com/apache/pinot/pull/10553#discussion_r1163315275
##########
pinot-tools/src/main/resources/examples/batch/airlineStats/airlineStats_offline_table_config.json:
##########
@@ -23,6 +23,33 @@
"MONTH"
]
}
+ },
+ {
+ "name": "ArrTimeBlk",
+ "encodingType": "DICTIONARY",
+ "indexes": {
+ "inverted": {
+ "enabled": "true"
+ }
+ },
+ "tierOverwrites": {
+ "hotTier": {
+ "encodingType": "DICTIONARY",
+ "indexes": {
+ "bloom": {
+ "enabled": "true"
+ }
+ }
+ },
+ "coldTier": {
+ "encodingType": "RAW",
Review Comment:
It's allowed to overwrite encoding type, as done by `FieldConfig.withTierOverwrites(tier)` method, where the tier specific FieldConfig object is constructed and may use tier specific encoding type, indexes or properties.
DictionaryIndexType has this`fieldConfig = fieldConfig.withTierOverwrites(tier);`, but I missed it for ForwardIndexType.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] klsince commented on pull request #10553: Allow to overwrite index configs at tier level
Posted by "klsince (via GitHub)" <gi...@apache.org>.
klsince commented on PR #10553:
URL: https://github.com/apache/pinot/pull/10553#issuecomment-1511828859
Thanks for reviewing @saurabhd336, and as to your question: the overwriting logic is more like merging, as it is done for those top level config keys. So need to keep the original _tableConfig to derive tier specific index configs correctly, otherwise we may accumulate index configs from different tiers.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on a diff in pull request #10553: Allow to overwrite index configs at tier level
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10553:
URL: https://github.com/apache/pinot/pull/10553#discussion_r1162393011
##########
pinot-tools/src/main/resources/examples/batch/airlineStats/airlineStats_offline_table_config.json:
##########
@@ -23,6 +23,33 @@
"MONTH"
]
}
+ },
+ {
+ "name": "ArrTimeBlk",
+ "encodingType": "DICTIONARY",
+ "indexes": {
+ "inverted": {
+ "enabled": "true"
+ }
+ },
+ "tierOverwrites": {
+ "hotTier": {
+ "encodingType": "DICTIONARY",
+ "indexes": {
+ "bloom": {
+ "enabled": "true"
+ }
+ }
+ },
+ "coldTier": {
+ "encodingType": "RAW",
Review Comment:
Reading the code I thought we weren't going to support overriding encoding type. Which part of the code actually overrides the effective encodingType? For example ForwardIndexType#137 is still reading the default encoding type, ignoring the override. Same with Dictionary.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] klsince commented on pull request #10553: Allow to overwrite index configs at tier level
Posted by "klsince (via GitHub)" <gi...@apache.org>.
klsince commented on PR #10553:
URL: https://github.com/apache/pinot/pull/10553#issuecomment-1505773236
@gortiz @npawar I updated PR, trying to implement the new idea proposed by @gortiz pls take another look.
btw, PR description is updated for the new way of implementing the feature.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] klsince commented on a diff in pull request #10553: Allow to overwrite index configs at tier level
Posted by "klsince (via GitHub)" <gi...@apache.org>.
klsince commented on code in PR #10553:
URL: https://github.com/apache/pinot/pull/10553#discussion_r1169271966
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/TableConfigBuilder.java:
##########
@@ -460,6 +467,7 @@ public TableConfig build() {
indexingConfig.setAggregateMetrics(_aggregateMetrics);
indexingConfig.setOptimizeDictionaryForMetrics(_optimizeDictionaryForMetrics);
indexingConfig.setNoDictionarySizeRatioThreshold(_noDictionarySizeRatioThreshold);
+ indexingConfig.setTierOverwrites(_tierOverwrites);
Review Comment:
Both tableIndexConfig (i.e. the indexingConfig object here) and fieldConfigList have their own `tierOverwrites`. The fieldConfigList (with tierOverwrites on individual fieldConfig as needed by the caller) is provided to this TableConfigBuilder separately.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] npawar merged pull request #10553: Allow to overwrite index configs at tier level
Posted by "npawar (via GitHub)" <gi...@apache.org>.
npawar merged PR #10553:
URL: https://github.com/apache/pinot/pull/10553
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] npawar commented on a diff in pull request #10553: Allow to overwrite index configs at tier level
Posted by "npawar (via GitHub)" <gi...@apache.org>.
npawar commented on code in PR #10553:
URL: https://github.com/apache/pinot/pull/10553#discussion_r1169304072
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/TableConfigBuilder.java:
##########
@@ -460,6 +467,7 @@ public TableConfig build() {
indexingConfig.setAggregateMetrics(_aggregateMetrics);
indexingConfig.setOptimizeDictionaryForMetrics(_optimizeDictionaryForMetrics);
indexingConfig.setNoDictionarySizeRatioThreshold(_noDictionarySizeRatioThreshold);
+ indexingConfig.setTierOverwrites(_tierOverwrites);
Review Comment:
clarified from offline discussion (method name is a bit confusing as it's on top level TableConfigBuilder, but can only be used to set indexingConfig). This can be addressed separately as it only affects tests.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] klsince commented on a diff in pull request #10553: Allow to overwrite index configs at tier level
Posted by "klsince (via GitHub)" <gi...@apache.org>.
klsince commented on code in PR #10553:
URL: https://github.com/apache/pinot/pull/10553#discussion_r1169266722
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/config/TableConfigUtils.java:
##########
@@ -331,6 +339,96 @@ public static void convertFromLegacyTableConfig(TableConfig tableConfig) {
validationConfig.setSegmentPushType(null);
}
+ /**
+ * Helper method to create a new TableConfig by overwriting the original TableConfig with tier specific configs, so
+ * that the consumers of TableConfig don't have to handle tier overwrites themselves. To begin with, we only
+ * consider to overwrite the index configs in `tableIndexConfig` and `fieldConfigList`, e.g.
+ *
+ * {
+ * "tableIndexConfig": {
+ * ... // configs allowed in IndexingConfig, for default tier
+ * "tierOverwrites": {
+ * "hotTier": {...}, // configs allowed in IndexingConfig, for hot tier
+ * "coldTier": {...} // configs allowed in IndexingConfig, for cold tier
+ * }
+ * }
+ * "fieldConfigList": [
+ * {
+ * ... // configs allowed in FieldConfig, for default tier
+ * "tierOverwrites": {
+ * "hotTier": {...}, // configs allowed in FieldConfig, for hot tier
+ * "coldTier": {...} // configs allowed in FieldConfig, for cold tier
+ * }
+ * },
+ * ...
+ * ]
+ * }
Review Comment:
The precedence between using fieldConfigList or tableIndexConfig is defined by IndexType.getConfig() from index-spi, which basically favors fieldConfigList. But this is the detail of IndexType.getConfig() and may get changed as needed in future. So changes in this PR tried to decouple IndexType.getConfig() from the logic of applying tier specific configs.
But for IndexType.getConfig() to access tier specific configs, we make a temp TableConfig by copying tier specific configs from `tierOverwrites` out and overwrite those defined for default tier, and then pass the TableConfig object to IndexType.getConfig().
> ... is the instruction to just repeat whatever json fields...
mostly right, but only for `top-level` config keys defined in IndexingConfig or FieldConfig, e.g. StarTreeIndexConfig is overwritten as a whole, instead of overwriting the inner fields of StarTreeIndexConfig.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] npawar commented on a diff in pull request #10553: Allow to overwrite index configs at tier level
Posted by "npawar (via GitHub)" <gi...@apache.org>.
npawar commented on code in PR #10553:
URL: https://github.com/apache/pinot/pull/10553#discussion_r1169235226
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/config/TableConfigUtils.java:
##########
@@ -331,6 +339,96 @@ public static void convertFromLegacyTableConfig(TableConfig tableConfig) {
validationConfig.setSegmentPushType(null);
}
+ /**
+ * Helper method to create a new TableConfig by overwriting the original TableConfig with tier specific configs, so
+ * that the consumers of TableConfig don't have to handle tier overwrites themselves. To begin with, we only
+ * consider to overwrite the index configs in `tableIndexConfig` and `fieldConfigList`, e.g.
+ *
+ * {
+ * "tableIndexConfig": {
+ * ... // configs allowed in IndexingConfig, for default tier
+ * "tierOverwrites": {
+ * "hotTier": {...}, // configs allowed in IndexingConfig, for hot tier
+ * "coldTier": {...} // configs allowed in IndexingConfig, for cold tier
+ * }
+ * }
+ * "fieldConfigList": [
+ * {
+ * ... // configs allowed in FieldConfig, for default tier
+ * "tierOverwrites": {
+ * "hotTier": {...}, // configs allowed in FieldConfig, for hot tier
+ * "coldTier": {...} // configs allowed in FieldConfig, for cold tier
+ * }
+ * },
+ * ...
+ * ]
+ * }
Review Comment:
how does this work given tableIndexConfig is at table level, vs fieldConfigList is at column level? What exactly is one overriding when adding in tableIndexing? Or is the instruction to just repeat whatever json fields one is interested in ?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/TableConfigBuilder.java:
##########
@@ -460,6 +467,7 @@ public TableConfig build() {
indexingConfig.setAggregateMetrics(_aggregateMetrics);
indexingConfig.setOptimizeDictionaryForMetrics(_optimizeDictionaryForMetrics);
indexingConfig.setNoDictionarySizeRatioThreshold(_noDictionarySizeRatioThreshold);
+ indexingConfig.setTierOverwrites(_tierOverwrites);
Review Comment:
why is this only being set in indexingConfig and not in fieldConfigList? or rather - how does one know which overwrites on is providing via the setter?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on a diff in pull request #10553: Allow to overwrite index configs at tier level
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10553:
URL: https://github.com/apache/pinot/pull/10553#discussion_r1162395550
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/IndexLoadingConfigTest.java:
##########
@@ -0,0 +1,95 @@
+/**
+ * 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.index.loader;
+
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.Map;
+import org.apache.pinot.segment.spi.index.FieldIndexConfigs;
+import org.apache.pinot.segment.spi.index.StandardIndexes;
+import org.apache.pinot.spi.config.instance.InstanceDataManagerConfig;
+import org.apache.pinot.spi.config.table.FieldConfig;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.config.table.TableType;
+import org.apache.pinot.spi.data.FieldSpec;
+import org.apache.pinot.spi.data.Schema;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.utils.JsonUtils;
+import org.apache.pinot.spi.utils.builder.TableConfigBuilder;
+import org.testng.annotations.Test;
+
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+import static org.testng.Assert.assertFalse;
+import static org.testng.Assert.assertTrue;
+
+
+public class IndexLoadingConfigTest {
+ private static final String TABLE_NAME = "table01";
+
+ @Test
+ public void testCalculateIndexConfigsWithTierOverwrites()
Review Comment:
I think it would be more readable if the test directly uses a TableConfig and then set some attributes using plain JSON strings as done in ForwardIndexTypeTest and other similar ones. Also, I would prefer to have two tests, one that reads the normal case and another that reads the tiered case, so in case there is a bug, it would be more localized.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on a diff in pull request #10553: Allow to overwrite index configs at tier level
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10553:
URL: https://github.com/apache/pinot/pull/10553#discussion_r1162384815
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/AbstractIndexType.java:
##########
@@ -48,8 +50,18 @@ public String getId() {
@Override
public Map<String, C> getConfig(TableConfig tableConfig, Schema schema) {
+ return getConfig(tableConfig, schema, null);
+ }
+
+ @Override
+ public Map<String, C> getConfig(TableConfig tableConfig, Schema schema, @Nullable String tier) {
+ if (tier != null) {
+ ColumnConfigDeserializer<C> deserializer =
+ _deserializersByTier.computeIfAbsent(tier, t -> createDeserializer(tier));
+ return deserializer.deserialize(tableConfig, schema);
+ }
if (_deserializer == null) {
- _deserializer = createDeserializer();
+ _deserializer = createDeserializer(null);
Review Comment:
So we creating one deserializer per tier?. I would suggest to actually change ColumnCOnfigDeserializer in order to add the tier parameter there, so there would always be a single deserializer that is able to read any tier.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] klsince commented on pull request #10553: Allow to overwrite index configs at tier level
Posted by "klsince (via GitHub)" <gi...@apache.org>.
klsince commented on PR #10553:
URL: https://github.com/apache/pinot/pull/10553#issuecomment-1504119891
@gortiz thanks for the comments. I like your idea of generating different TableConfig instances for tiers for better separation. The Index.getConfig() encapsulates a lot of details of assembling index configs from everywhere in TableConfig, so I'd +1 to reuse it than make it more complex for being aware of tier (which is the idea of current change). Below are a few points about current change, but I'll think more about your idea anyway.
> ... encodingType is not actually being override, so we should also add this logic in at least another place...
The current change allows to overwrite the encodingType via `FieldConfig.withTierOverwrites(tier)`
> ... In these 3 new TableConfigs there is no tierOverride, so we can read indexes (and any other config!) without actually needed to know about them...
The current change can use any other config as well. If tier is provided, the tier specific ones take precedence.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on a diff in pull request #10553: Allow to overwrite index configs at tier level
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10553:
URL: https://github.com/apache/pinot/pull/10553#discussion_r1165872883
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/IndexLoadingConfig.java:
##########
@@ -308,6 +318,22 @@ private <C extends IndexConfig> ColumnConfigDeserializer<C> getDeserializer(Inde
return deserializer;
}
+ private TableConfig getTableConfigWithTierOverwrites() {
+ return (_segmentTier == null || _tableConfig == null) ? _tableConfig
+ : TableConfigUtils.overwriteTableConfigForTier(_tableConfig, _segmentTier);
+ }
+
+ private Schema inferSchema() {
+ if (_schema != null) {
+ return _schema;
+ }
+ Schema schema = new Schema();
+ for (String column : getAllKnownColumns()) {
+ schema.addField(new DimensionFieldSpec(column, FieldSpec.DataType.STRING, true));
Review Comment:
I guess this is only going to be executed in tests, but using a random type here looks... problematic.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on pull request #10553: Allow to overwrite index configs at tier level
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #10553:
URL: https://github.com/apache/pinot/pull/10553#issuecomment-1507410627
I would like to take a look to the tests tomorrow, but it looks good to me. Let's see what actual committers think about it :)
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #10553: [WIP] allow to overwrite index configs at tier level
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10553:
URL: https://github.com/apache/pinot/pull/10553#issuecomment-1497965103
## [Codecov](https://codecov.io/gh/apache/pinot/pull/10553?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#10553](https://codecov.io/gh/apache/pinot/pull/10553?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6fda5b9) into [master](https://codecov.io/gh/apache/pinot/commit/513fa3147bbe1679eb247e5f169d458c35cf794f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (513fa31) will **decrease** coverage by `53.99%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10553 +/- ##
=============================================
- Coverage 67.84% 13.85% -53.99%
+ Complexity 6001 439 -5562
=============================================
Files 1575 2049 +474
Lines 81930 110329 +28399
Branches 12870 16687 +3817
=============================================
- Hits 55586 15291 -40295
- Misses 22450 93789 +71339
+ Partials 3894 1249 -2645
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests1 | `?` | |
| unittests2 | `13.85% <0.00%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/10553?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ment/local/segment/index/bloom/BloomIndexType.java](https://codecov.io/gh/apache/pinot/pull/10553?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2Jsb29tL0Jsb29tSW5kZXhUeXBlLmphdmE=) | `0.00% <0.00%> (-84.00%)` | :arrow_down: |
| [.../segment/index/dictionary/DictionaryIndexType.java](https://codecov.io/gh/apache/pinot/pull/10553?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2RpY3Rpb25hcnkvRGljdGlvbmFyeUluZGV4VHlwZS5qYXZh) | `0.00% <0.00%> (-93.62%)` | :arrow_down: |
| [.../local/segment/index/forward/ForwardIndexType.java](https://codecov.io/gh/apache/pinot/pull/10553?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2ZvcndhcmQvRm9yd2FyZEluZGV4VHlwZS5qYXZh) | `0.00% <0.00%> (-97.47%)` | :arrow_down: |
| [.../segment/local/segment/index/fst/FstIndexType.java](https://codecov.io/gh/apache/pinot/pull/10553?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2ZzdC9Gc3RJbmRleFR5cGUuamF2YQ==) | `0.00% <0.00%> (-78.95%)` | :arrow_down: |
| [...ot/segment/local/segment/index/h3/H3IndexType.java](https://codecov.io/gh/apache/pinot/pull/10553?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2gzL0gzSW5kZXhUeXBlLmphdmE=) | `0.00% <0.00%> (-85.00%)` | :arrow_down: |
| [...ocal/segment/index/inverted/InvertedIndexType.java](https://codecov.io/gh/apache/pinot/pull/10553?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2ludmVydGVkL0ludmVydGVkSW5kZXhUeXBlLmphdmE=) | `0.00% <0.00%> (-67.50%)` | :arrow_down: |
| [...egment/local/segment/index/json/JsonIndexType.java](https://codecov.io/gh/apache/pinot/pull/10553?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2pzb24vSnNvbkluZGV4VHlwZS5qYXZh) | `0.00% <0.00%> (-74.20%)` | :arrow_down: |
| [...ocal/segment/index/loader/ForwardIndexHandler.java](https://codecov.io/gh/apache/pinot/pull/10553?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2xvYWRlci9Gb3J3YXJkSW5kZXhIYW5kbGVyLmphdmE=) | `0.00% <0.00%> (-82.68%)` | :arrow_down: |
| [...local/segment/index/loader/IndexLoadingConfig.java](https://codecov.io/gh/apache/pinot/pull/10553?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2xvYWRlci9JbmRleExvYWRpbmdDb25maWcuamF2YQ==) | `0.00% <0.00%> (-84.93%)` | :arrow_down: |
| [...al/segment/index/nullvalue/NullValueIndexType.java](https://codecov.io/gh/apache/pinot/pull/10553?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L251bGx2YWx1ZS9OdWxsVmFsdWVJbmRleFR5cGUuamF2YQ==) | `0.00% <0.00%> (-80.96%)` | :arrow_down: |
| ... and [8 more](https://codecov.io/gh/apache/pinot/pull/10553?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
... and [1857 files with indirect coverage changes](https://codecov.io/gh/apache/pinot/pull/10553/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org