You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "gortiz (via GitHub)" <gi...@apache.org> on 2023/04/10 10:30:46 UTC
[GitHub] [pinot] gortiz opened a new pull request, #10584: Enable index config by default.
gortiz opened a new pull request, #10584:
URL: https://github.com/apache/pinot/pull/10584
This is a bugfix.
As introduced in #10183, each `IndexConfig` subclass had to manually check for `enabled` nullability in its own way. It is correct that some indexes may request to explicitly ask for `enabled: true` in order to be enabled, but all default indexes should be enabled if their config object is present.
For example:
```js
{
bloom: {
fpp: 0.02,
}
}
```
Should be considered as a enabled config. Even the empty object should be considered enabled
```js
{
bloom: {
}
}
```
In order to explicitly disable an index one must write
```js
{
bloom: {
enabled: "false"
}
}
```
If the index key is not present or set to null, then the default value will be used. In that case, whether the index is enable or not depends on the index type. For example forward index is enabled by default.
Previous code was erroneously initializing some index configs as disabled, as detected in #10554 for bloom filter. Affected indexes were:
- Bloom
- Dictionary
- H3
- FST
- Range
- Text
- Inverted
This PR adds a constructor in `IndexConfig` that contains the default logic, which is the one all default indexes should use. The PR also add unit tests that verify the correct deserialization of indexes for all index types.
--
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 #10584: Enable index config by default.
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #10584:
URL: https://github.com/apache/pinot/pull/10584#issuecomment-1502024725
I still think from a user perspective is cleaner to say
```js
{
bloom: {
enabled: false
}
}
```
Than
```js
{
bloom: {
disabled: true
}
}
```
But I'm open to change that if other people think like @Jackie-Jiang. What I think that is important is to decide that ASAP. Once people start using `enabled` it would be more difficult to change 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] Jackie-Jiang merged pull request #10584: Enable index config by default.
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #10584:
URL: https://github.com/apache/pinot/pull/10584
--
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 #10584: Enable index config by default.
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10584:
URL: https://github.com/apache/pinot/pull/10584#discussion_r1161801712
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/IndexConfig.java:
##########
@@ -35,7 +35,15 @@ public class IndexConfig extends BaseJsonConfig {
public static final IndexConfig DISABLED = new IndexConfig(false);
private final boolean _enabled;
+ /**
+ * This is used as the default constructor for subclasses. The attribute {@link #_enabled} will be initialized to
+ * {@code enabled == null || enabled}, so it will be false if and only if {@code Boolean.FALSE.equals(enabled)}.
+ */
@JsonCreator
+ public IndexConfig(@JsonProperty("enabled") Boolean enabled) {
+ this(enabled == null || enabled);
+ }
+
public IndexConfig(@JsonProperty("enabled") boolean enabled) {
Review Comment:
I usually try to keep older constructors to do not break binary backward compatibility, but I don't think anyone had time to add code that relay on this constructor. Another reason to keep it here may be to skip boxing/unboxing, but this shouldn't be in the hotpath.
So I'm going to remove 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] KKcorps commented on a diff in pull request #10584: Enable index config by default.
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on code in PR #10584:
URL: https://github.com/apache/pinot/pull/10584#discussion_r1161798288
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/IndexConfig.java:
##########
@@ -35,7 +35,15 @@ public class IndexConfig extends BaseJsonConfig {
public static final IndexConfig DISABLED = new IndexConfig(false);
private final boolean _enabled;
+ /**
+ * This is used as the default constructor for subclasses. The attribute {@link #_enabled} will be initialized to
+ * {@code enabled == null || enabled}, so it will be false if and only if {@code Boolean.FALSE.equals(enabled)}.
+ */
@JsonCreator
+ public IndexConfig(@JsonProperty("enabled") Boolean enabled) {
+ this(enabled == null || enabled);
+ }
+
public IndexConfig(@JsonProperty("enabled") boolean enabled) {
Review Comment:
Do we need this constructor? We can directly set the value it in the previous one right?
--
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 #10584: Enable index config by default.
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #10584:
URL: https://github.com/apache/pinot/pull/10584#issuecomment-1502099192
> What happens if we deploy this, then it switches? I guess enabled: false is the only case where the behavior will switch?
That is my concern. The easiest change would be to just ignore the `enabled` attribute, so:
| when enabled was | prev this PR | after this PR | if we use disabled |
|---|---|---|---|
| not defined | sometimes disabled the index | enable the index | enable the index |
| null | sometimes disable the index | enable the index | enable the index |
| true | enable the index | enable the index | enable the index |
| false | disable the index | disable the index | **enable the index** |
--
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] jadami10 commented on pull request #10584: Enable index config by default.
Posted by "jadami10 (via GitHub)" <gi...@apache.org>.
jadami10 commented on PR #10584:
URL: https://github.com/apache/pinot/pull/10584#issuecomment-1502042214
> One less common example that IMO can lead to surprising behavior is:
>
> ```
> {
> enabled: null
> }
> ```
>
> The above example will actually enable the index
It's surprising, but I think it's still better. If you're configuring an index in any way, you probably already want it on. And if you don't want it on, you're either going to delete the entire block, or explicitly set `"enabled": false`.
As for enabled vs disabled, was this a new a field, or did it already exist? Because if it existed, wouldn't this be a big breaking change 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] Jackie-Jiang commented on pull request #10584: Enable index config by default.
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #10584:
URL: https://github.com/apache/pinot/pull/10584#issuecomment-1502057435
> As for enabled vs disabled, was this a new a field, or did it already exist? Because if it existed, wouldn't this be a big breaking change now?
It is newly introduced, and we are debating whether we should explicitly put `enabled: false` or `disabled: true` to disable an index. I'm suggesting using `disabled: true` to disable the index, where `disabled: null` will be treated as index enabled
--
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] jadami10 commented on pull request #10584: Enable index config by default.
Posted by "jadami10 (via GitHub)" <gi...@apache.org>.
jadami10 commented on PR #10584:
URL: https://github.com/apache/pinot/pull/10584#issuecomment-1502094779
What happens if we deploy this, then it switches? I guess `enabled: false` is the only case where the behavior will switch?
--
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 #10584: Enable index config by default.
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #10584:
URL: https://github.com/apache/pinot/pull/10584#issuecomment-1502109953
Let's move the discussion to https://github.com/apache/pinot/pull/10585
--
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 #10584: Enable index config by default.
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10584:
URL: https://github.com/apache/pinot/pull/10584#issuecomment-1501849682
## [Codecov](https://codecov.io/gh/apache/pinot/pull/10584?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 [#10584](https://codecov.io/gh/apache/pinot/pull/10584?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2b1d28e) into [master](https://codecov.io/gh/apache/pinot/commit/ea6361701872a753df6faa60e949a40cb2a86580?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ea63617) will **decrease** coverage by `56.49%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10584 +/- ##
=============================================
- Coverage 70.39% 13.90% -56.49%
+ Complexity 6483 439 -6044
=============================================
Files 2106 2052 -54
Lines 112882 110405 -2477
Branches 17001 16703 -298
=============================================
- Hits 79466 15355 -64111
- Misses 27838 93792 +65954
+ Partials 5578 1258 -4320
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `13.90% <0.00%> (+<0.01%)` | :arrow_up: |
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/10584?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...pinot/segment/spi/index/DictionaryIndexConfig.java](https://codecov.io/gh/apache/pinot/pull/10584?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2luZGV4L0RpY3Rpb25hcnlJbmRleENvbmZpZy5qYXZh) | `0.00% <0.00%> (-68.19%)` | :arrow_down: |
| [...he/pinot/segment/spi/index/ForwardIndexConfig.java](https://codecov.io/gh/apache/pinot/pull/10584?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2luZGV4L0ZvcndhcmRJbmRleENvbmZpZy5qYXZh) | `0.00% <0.00%> (-62.00%)` | :arrow_down: |
| [...apache/pinot/segment/spi/index/FstIndexConfig.java](https://codecov.io/gh/apache/pinot/pull/10584?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2luZGV4L0ZzdEluZGV4Q29uZmlnLmphdmE=) | `0.00% <0.00%> (-66.67%)` | :arrow_down: |
| [...ache/pinot/segment/spi/index/RangeIndexConfig.java](https://codecov.io/gh/apache/pinot/pull/10584?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2luZGV4L1JhbmdlSW5kZXhDb25maWcuamF2YQ==) | `0.00% <0.00%> (-87.50%)` | :arrow_down: |
| [...pache/pinot/segment/spi/index/TextIndexConfig.java](https://codecov.io/gh/apache/pinot/pull/10584?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2luZGV4L1RleHRJbmRleENvbmZpZy5qYXZh) | `0.00% <0.00%> (-81.09%)` | :arrow_down: |
| [...pinot/segment/spi/index/creator/H3IndexConfig.java](https://codecov.io/gh/apache/pinot/pull/10584?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2luZGV4L2NyZWF0b3IvSDNJbmRleENvbmZpZy5qYXZh) | `0.00% <0.00%> (-64.71%)` | :arrow_down: |
| [...ache/pinot/spi/config/table/BloomFilterConfig.java](https://codecov.io/gh/apache/pinot/pull/10584?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL0Jsb29tRmlsdGVyQ29uZmlnLmphdmE=) | `0.00% <0.00%> (-68.00%)` | :arrow_down: |
| [...org/apache/pinot/spi/config/table/IndexConfig.java](https://codecov.io/gh/apache/pinot/pull/10584?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL0luZGV4Q29uZmlnLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...apache/pinot/spi/config/table/JsonIndexConfig.java](https://codecov.io/gh/apache/pinot/pull/10584?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL0pzb25JbmRleENvbmZpZy5qYXZh) | `0.00% <0.00%> (-86.21%)` | :arrow_down: |
... and [1653 files with indirect coverage changes](https://codecov.io/gh/apache/pinot/pull/10584/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
[GitHub] [pinot] gortiz commented on pull request #10584: Enable index config by default.
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #10584:
URL: https://github.com/apache/pinot/pull/10584#issuecomment-1502062529
I think we can discuss about that, but it shouldn't block this PR. We can merge this to solve the problem and then decide whether we want to change the attribute from `enabled` to `disabled`
--
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] Jackie-Jiang commented on pull request #10584: Enable index config by default.
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #10584:
URL: https://github.com/apache/pinot/pull/10584#issuecomment-1502031312
> I still think from a user perspective is cleaner to say
>
> ```js
> {
> bloom: {
> enabled: false
> }
> }
> ```
>
> Than
>
> ```js
> {
> bloom: {
> disabled: true
> }
> }
> ```
>
> But I'm open to change that if other people think like @Jackie-Jiang. What I think that is important is to decide that ASAP. Once people start using `enabled` it would be more difficult to change it.
Agree. Once the key is determined, it will be hard to change.
One less common example that IMO can lead to surprising behavior is:
```
{
enabled: null
}
```
The above example will actually enable the index
--
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] jadami10 commented on pull request #10584: Enable index config by default.
Posted by "jadami10 (via GitHub)" <gi...@apache.org>.
jadami10 commented on PR #10584:
URL: https://github.com/apache/pinot/pull/10584#issuecomment-1501903726
Thank you for the awesome test coverage. I'll let others review as well. In case there's some concern with always enabling an index unless the config is exactly `"enabled": false`, I definitely think the way you did it here is the least surprising for users.
--
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