navina commented on code in PR #10484:
URL: https://github.com/apache/pinot/pull/10484#discussion_r1149714646
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/TableConfigBuilder.java:
##########
@@ -256,6 +258,16 @@ public TableConfigBuilder
setInvertedIndexColumns(List<String> invertedIndexColu
return this;
}
+ public TableConfigBuilder setOptimizeDictionaryForMetrics(boolean
optimizeDictionaryForMetrics) {
+ _optimizeDictionaryForMetrics = optimizeDictionaryForMetrics;
+ return this;
+ }
+
+ public TableConfigBuilder setNoDictionarySizeRatioThreshold(double
noDictionarySizeRatioThreshold) {
Review Comment:
>Im following what we introduced in the tableIndexConfig, to keep it
consistent :).
Yeah. But it is not clear. At first glance, `optimizeDictionaryForMetrics`
made me think " for what metrics are we optimizing? " :)) If we want to be this
verbose, then it should have been `optimizeDictionaryForMetricColumns` :))
Would really appreciate an improvement over
`noDictionarySizeRatioThreshold`. Welcome others to chime in as well.
> The test creates tableConfig with some values in the 2 new fields,
serializes & de-serializes the tableConfig and verifies if the values match
with what was initialized.
Right. That is only testing the config values. do we not already have tests
that loads a metric column with data and ensures the indexing config actually
enforces the optimization? I would assume such a test would have been added
when this optimization was introduced.
--
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]
