You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "swaminathanmanish (via GitHub)" <gi...@apache.org> on 2023/03/27 18:26:32 UTC
[GitHub] [pinot] swaminathanmanish opened a new pull request, #10484: Adding fields to enable/disable dictionary optimization for metric columns
swaminathanmanish opened a new pull request, #10484:
URL: https://github.com/apache/pinot/pull/10484
This [PR](https://github.com/apache/pinot/pull/8398) introduced index config options to enable/disable dictionary optimization for metrics based on a custom threshold (to enable/disable dictionary for a column) and we want to be able to make use of those options via TableConfigBuilder.
--
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] swaminathanmanish commented on a diff in pull request #10484: Adding fields in TableConfigBuilder to enable/disable dictionary optimization for metric columns
Posted by "swaminathanmanish (via GitHub)" <gi...@apache.org>.
swaminathanmanish commented on code in PR #10484:
URL: https://github.com/apache/pinot/pull/10484#discussion_r1149744978
##########
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:
Yes testing for that path is part of this [PR](https://github.com/apache/pinot/pull/8398), where it reads fields from the IndexConfig.
cc @KKcorps
--
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] swaminathanmanish commented on a diff in pull request #10484: Adding fields in TableConfigBuilder to enable/disable dictionary optimization for metric columns
Posted by "swaminathanmanish (via GitHub)" <gi...@apache.org>.
swaminathanmanish commented on code in PR #10484:
URL: https://github.com/apache/pinot/pull/10484#discussion_r1149704765
##########
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 index table config :)
https://docs.pinot.apache.org/configuration-reference/table#table-index-config
> does this test include data? can we add a test with some data and verify that the threshold is honored or not?
>
> Otherwise, lgtm!
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.
--
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] swaminathanmanish commented on a diff in pull request #10484: Adding fields in TableConfigBuilder to enable/disable dictionary optimization for metric columns
Posted by "swaminathanmanish (via GitHub)" <gi...@apache.org>.
swaminathanmanish commented on code in PR #10484:
URL: https://github.com/apache/pinot/pull/10484#discussion_r1149744978
##########
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:
Yes thats part of this [PR](https://github.com/apache/pinot/pull/8398), to read from the IndexConfig.
--
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] swaminathanmanish commented on a diff in pull request #10484: Adding fields in TableConfigBuilder to enable/disable dictionary optimization for metric columns
Posted by "swaminathanmanish (via GitHub)" <gi...@apache.org>.
swaminathanmanish commented on code in PR #10484:
URL: https://github.com/apache/pinot/pull/10484#discussion_r1149684522
##########
pinot-common/src/test/java/org/apache/pinot/common/utils/config/TableConfigSerDeTest.java:
##########
@@ -64,7 +64,13 @@
public class TableConfigSerDeTest {
-
+ @Test
+ public void testTableConfigBuilderMetricsColumnOptimizationOptions() {
+ TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName("testTable").
+ setOptimizeDictionaryForMetrics(true).setNoDictionarySizeRatioThreshold(0.72).build();
+ assertEquals(tableConfig.getIndexingConfig().getNoDictionarySizeRatioThreshold(), 0.72);
+ assertEquals(tableConfig.getIndexingConfig().isOptimizeDictionaryForMetrics(), true);
+ }
Review Comment:
I've removed this test now since ser/deser should cover these functions anyway.
--
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] snleee commented on a diff in pull request #10484: Adding fields in TableConfigBuilder to enable/disable dictionary optimization for metric columns
Posted by "snleee (via GitHub)" <gi...@apache.org>.
snleee commented on code in PR #10484:
URL: https://github.com/apache/pinot/pull/10484#discussion_r1149670583
##########
pinot-common/src/test/java/org/apache/pinot/common/utils/config/TableConfigSerDeTest.java:
##########
@@ -64,7 +64,13 @@
public class TableConfigSerDeTest {
-
+ @Test
+ public void testTableConfigBuilderMetricsColumnOptimizationOptions() {
+ TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName("testTable").
+ setOptimizeDictionaryForMetrics(true).setNoDictionarySizeRatioThreshold(0.72).build();
+ assertEquals(tableConfig.getIndexingConfig().getNoDictionarySizeRatioThreshold(), 0.72);
+ assertEquals(tableConfig.getIndexingConfig().isOptimizeDictionaryForMetrics(), true);
Review Comment:
can we add the ser/de test as well?
From other test:
```
// Serialize then de-serialize
TableConfig tableConfigToCompare = JsonUtils.stringToObject(tableConfig.toJsonString(), TableConfig.class);
assertEquals(tableConfigToCompare, tableConfig);
tableConfigToCompare = TableConfigUtils.fromZNRecord(TableConfigUtils.toZNRecord(tableConfig));
assertEquals(tableConfigToCompare, 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] swaminathanmanish commented on a diff in pull request #10484: Adding fields in TableConfigBuilder to enable/disable dictionary optimization for metric columns
Posted by "swaminathanmanish (via GitHub)" <gi...@apache.org>.
swaminathanmanish commented on code in PR #10484:
URL: https://github.com/apache/pinot/pull/10484#discussion_r1149744978
##########
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:
Yes thats part of this [PR](https://github.com/apache/pinot/pull/8398), to read from the IndexConfig.
cc @KKcorps
--
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] swaminathanmanish commented on a diff in pull request #10484: Adding fields in TableConfigBuilder to enable/disable dictionary optimization for metric columns
Posted by "swaminathanmanish (via GitHub)" <gi...@apache.org>.
swaminathanmanish commented on code in PR #10484:
URL: https://github.com/apache/pinot/pull/10484#discussion_r1149684704
##########
pinot-common/src/test/java/org/apache/pinot/common/utils/config/TableConfigSerDeTest.java:
##########
@@ -64,7 +64,13 @@
public class TableConfigSerDeTest {
-
+ @Test
+ public void testTableConfigBuilderMetricsColumnOptimizationOptions() {
+ TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName("testTable").
+ setOptimizeDictionaryForMetrics(true).setNoDictionarySizeRatioThreshold(0.72).build();
+ assertEquals(tableConfig.getIndexingConfig().getNoDictionarySizeRatioThreshold(), 0.72);
+ assertEquals(tableConfig.getIndexingConfig().isOptimizeDictionaryForMetrics(), true);
Review Comment:
Added.
--
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] swaminathanmanish commented on pull request #10484: Adding fields to enable/disable dictionary optimization for metric columns
Posted by "swaminathanmanish (via GitHub)" <gi...@apache.org>.
swaminathanmanish commented on PR #10484:
URL: https://github.com/apache/pinot/pull/10484#issuecomment-1485667064
@snleee - Please take a look.
--
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] swaminathanmanish commented on a diff in pull request #10484: Adding fields in TableConfigBuilder to enable/disable dictionary optimization for metric columns
Posted by "swaminathanmanish (via GitHub)" <gi...@apache.org>.
swaminathanmanish commented on code in PR #10484:
URL: https://github.com/apache/pinot/pull/10484#discussion_r1149700724
##########
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 :).
From this doc - https://docs.pinot.apache.org/configuration-reference/table#table-index-config
"If optimizeDictionaryForMetrics enabled, dictionary is not created for the metric columns for which noDictionaryIndexSize/ indexWithDictionarySize is less than the noDictionarySizeRatioThreshold
Default: 0.85"
--
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] swaminathanmanish commented on a diff in pull request #10484: Adding fields in TableConfigBuilder to enable/disable dictionary optimization for metric columns
Posted by "swaminathanmanish (via GitHub)" <gi...@apache.org>.
swaminathanmanish commented on code in PR #10484:
URL: https://github.com/apache/pinot/pull/10484#discussion_r1149705462
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/TableConfigBuilder.java:
##########
@@ -103,6 +103,8 @@ public class TableConfigBuilder {
private List<StarTreeIndexConfig> _starTreeIndexConfigs;
private List<String> _jsonIndexColumns;
private boolean _aggregateMetrics;
+ private boolean _optimizeDictionaryForMetrics;
Review Comment:
Yes, _noDictionarySizeRatioThreshold is relevant only when _optimizeDictionaryForMetrics is true. Will add java docs.
--
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] snleee commented on a diff in pull request #10484: Adding fields in TableConfigBuilder to enable/disable dictionary optimization for metric columns
Posted by "snleee (via GitHub)" <gi...@apache.org>.
snleee commented on code in PR #10484:
URL: https://github.com/apache/pinot/pull/10484#discussion_r1149670583
##########
pinot-common/src/test/java/org/apache/pinot/common/utils/config/TableConfigSerDeTest.java:
##########
@@ -64,7 +64,13 @@
public class TableConfigSerDeTest {
-
+ @Test
+ public void testTableConfigBuilderMetricsColumnOptimizationOptions() {
+ TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName("testTable").
+ setOptimizeDictionaryForMetrics(true).setNoDictionarySizeRatioThreshold(0.72).build();
+ assertEquals(tableConfig.getIndexingConfig().getNoDictionarySizeRatioThreshold(), 0.72);
+ assertEquals(tableConfig.getIndexingConfig().isOptimizeDictionaryForMetrics(), true);
Review Comment:
can we add the ser/de test as well?
From other test:
```
// Serialize then de-serialize
TableConfig tableConfigToCompare = JsonUtils.stringToObject(tableConfig.toJsonString(), TableConfig.class);
assertEquals(tableConfigToCompare, tableConfig);
checkQuotaConfig(tableConfigToCompare);
tableConfigToCompare = TableConfigUtils.fromZNRecord(TableConfigUtils.toZNRecord(tableConfig));
assertEquals(tableConfigToCompare, tableConfig);
checkQuotaConfig(tableConfigToCompare);
```
--
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] navina commented on a diff in pull request #10484: Adding fields in TableConfigBuilder to enable/disable dictionary optimization for metric columns
Posted by "navina (via GitHub)" <gi...@apache.org>.
navina commented on code in PR #10484:
URL: https://github.com/apache/pinot/pull/10484#discussion_r1149684929
##########
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:
This naming is pretty confusing.
"sizeThresholdForNoDictionary" or "ratioForNoDictionary" ?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/TableConfigBuilder.java:
##########
@@ -103,6 +103,8 @@ public class TableConfigBuilder {
private List<StarTreeIndexConfig> _starTreeIndexConfigs;
private List<String> _jsonIndexColumns;
private boolean _aggregateMetrics;
+ private boolean _optimizeDictionaryForMetrics;
Review Comment:
`_noDictionarySizeRatioThreshold` is accepted only when `_optimizeDictionaryForMetrics` is set as `true`?
Can you clarify the dependency on these configs, if any? Perhaps add javadocs here so the next contributors understand the relationship.
We don't have a good way to document configs atm. javadocs can help in the interim
--
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] navina commented on a diff in pull request #10484: Adding fields in TableConfigBuilder to enable/disable dictionary optimization for metric columns
Posted by "navina (via GitHub)" <gi...@apache.org>.
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: 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] mayankshriv merged pull request #10484: Adding fields in TableConfigBuilder to enable/disable dictionary optimization for metric columns
Posted by "mayankshriv (via GitHub)" <gi...@apache.org>.
mayankshriv merged PR #10484:
URL: https://github.com/apache/pinot/pull/10484
--
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] swaminathanmanish commented on pull request #10484: Adding fields in TableConfigBuilder to enable/disable dictionary optimization for metric columns
Posted by "swaminathanmanish (via GitHub)" <gi...@apache.org>.
swaminathanmanish commented on PR #10484:
URL: https://github.com/apache/pinot/pull/10484#issuecomment-1485696092
> TableConfigSerDeTest
Added a test. Thanks !
--
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] snleee commented on a diff in pull request #10484: Adding fields in TableConfigBuilder to enable/disable dictionary optimization for metric columns
Posted by "snleee (via GitHub)" <gi...@apache.org>.
snleee commented on code in PR #10484:
URL: https://github.com/apache/pinot/pull/10484#discussion_r1149670583
##########
pinot-common/src/test/java/org/apache/pinot/common/utils/config/TableConfigSerDeTest.java:
##########
@@ -64,7 +64,13 @@
public class TableConfigSerDeTest {
-
+ @Test
+ public void testTableConfigBuilderMetricsColumnOptimizationOptions() {
+ TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName("testTable").
+ setOptimizeDictionaryForMetrics(true).setNoDictionarySizeRatioThreshold(0.72).build();
+ assertEquals(tableConfig.getIndexingConfig().getNoDictionarySizeRatioThreshold(), 0.72);
+ assertEquals(tableConfig.getIndexingConfig().isOptimizeDictionaryForMetrics(), true);
Review Comment:
can we add the ser/de test as well?
From other test:
```
// Serialize then de-serialize
TableConfig tableConfigToCompare = JsonUtils.stringToObject(tableConfig.toJsonString(), TableConfig.class);
assertEquals(tableConfigToCompare, tableConfig);
checkQuotaConfig(tableConfigToCompare);
tableConfigToCompare = TableConfigUtils.fromZNRecord(TableConfigUtils.toZNRecord(tableConfig));
assertEquals(tableConfigToCompare, tableConfig);
checkQuotaConfig(tableConfigToCompare);
```
```
--
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] snleee commented on a diff in pull request #10484: Adding fields in TableConfigBuilder to enable/disable dictionary optimization for metric columns
Posted by "snleee (via GitHub)" <gi...@apache.org>.
snleee commented on code in PR #10484:
URL: https://github.com/apache/pinot/pull/10484#discussion_r1149668917
##########
pinot-common/src/test/java/org/apache/pinot/common/utils/config/TableConfigSerDeTest.java:
##########
@@ -64,7 +64,13 @@
public class TableConfigSerDeTest {
-
+ @Test
+ public void testTableConfigBuilderMetricsColumnOptimizationOptions() {
+ TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName("testTable").
+ setOptimizeDictionaryForMetrics(true).setNoDictionarySizeRatioThreshold(0.72).build();
+ assertEquals(tableConfig.getIndexingConfig().getNoDictionarySizeRatioThreshold(), 0.72);
+ assertEquals(tableConfig.getIndexingConfig().isOptimizeDictionaryForMetrics(), true);
+ }
Review Comment:
add an extra line here :P
--
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] snleee commented on a diff in pull request #10484: Adding fields in TableConfigBuilder to enable/disable dictionary optimization for metric columns
Posted by "snleee (via GitHub)" <gi...@apache.org>.
snleee commented on code in PR #10484:
URL: https://github.com/apache/pinot/pull/10484#discussion_r1149664272
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/TableConfigBuilder.java:
##########
@@ -256,6 +258,15 @@ public TableConfigBuilder setInvertedIndexColumns(List<String> invertedIndexColu
return this;
}
+ public TableConfigBuilder setOptimizeDictionaryForMetrics(boolean optimizeDictionaryForMetrics) {
+ _optimizeDictionaryForMetrics = optimizeDictionaryForMetrics;
+ return this;
+ }
+
+ public TableConfigBuilder setNoDictionarySizeRatioThreshold(double noDictionarySizeRatioThreshold) {
+ _noDictionarySizeRatioThreshold = noDictionarySizeRatioThreshold;
+ return this;
+ }
Review Comment:
add an extra line after L269
--
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] snleee commented on a diff in pull request #10484: Adding fields in TableConfigBuilder to enable/disable dictionary optimization for metric columns
Posted by "snleee (via GitHub)" <gi...@apache.org>.
snleee commented on code in PR #10484:
URL: https://github.com/apache/pinot/pull/10484#discussion_r1149664272
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/TableConfigBuilder.java:
##########
@@ -256,6 +258,15 @@ public TableConfigBuilder setInvertedIndexColumns(List<String> invertedIndexColu
return this;
}
+ public TableConfigBuilder setOptimizeDictionaryForMetrics(boolean optimizeDictionaryForMetrics) {
+ _optimizeDictionaryForMetrics = optimizeDictionaryForMetrics;
+ return this;
+ }
+
+ public TableConfigBuilder setNoDictionarySizeRatioThreshold(double noDictionarySizeRatioThreshold) {
+ _noDictionarySizeRatioThreshold = noDictionarySizeRatioThreshold;
+ return this;
+ }
Review Comment:
add an extra line
--
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