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/20 08:04:56 UTC
[GitHub] [pinot] gortiz opened a new pull request, #10653: Remove actually unsupported config that selectively enable nullable columns
gortiz opened a new pull request, #10653:
URL: https://github.com/apache/pinot/pull/10653
As explained in #10652, nullability is not actually applied per column but to the whole table.
Before this PR, NullValueIndexType followed the same pattern common pattern where first it tries to check if it is configured in `fieldConfigList.indexes` and if it is not, it delegates into the other way to configure them.
In this case, the other way is to set `tableIndexConfg.nullHandlingEnabled` to true.
But even if NullValueIndexType open the possibility of setting nullability per column, the rest of the code is not prepared to do that. I've opened #10652 in order to discuss if we want to add that feature and how to do that, but meanwhile, I think it is better to disable the option of selectively configure nullability per column, given that it is confusing.
The PR also applies a small change in AbstractIndexType in order to improve error messages when indexes are defined using two different syntaxis.
--
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] 61yao commented on a diff in pull request #10653: Remove actually unsupported config that selectively enable nullable columns
Posted by "61yao (via GitHub)" <gi...@apache.org>.
61yao commented on code in PR #10653:
URL: https://github.com/apache/pinot/pull/10653#discussion_r1176810646
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/nullvalue/NullValueIndexType.java:
##########
@@ -67,14 +67,11 @@ public IndexConfig getDefaultConfig() {
@Override
public ColumnConfigDeserializer<IndexConfig> createDeserializer() {
- return IndexConfigDeserializer.fromIndexes("null", getIndexConfigClass())
- .withFallbackAlternative(
Review Comment:
I am not very familiar with the code. What does this withFallbackAlternative do?
--
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] 61yao commented on a diff in pull request #10653: Remove actually unsupported config that selectively enable nullable columns
Posted by "61yao (via GitHub)" <gi...@apache.org>.
61yao commented on code in PR #10653:
URL: https://github.com/apache/pinot/pull/10653#discussion_r1176811227
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/AbstractIndexType.java:
##########
@@ -51,7 +51,11 @@ public Map<String, C> getConfig(TableConfig tableConfig, Schema schema) {
if (_deserializer == null) {
_deserializer = createDeserializer();
}
- return _deserializer.deserialize(tableConfig, schema);
+ try {
+ return _deserializer.deserialize(tableConfig, schema);
Review Comment:
Can you help me understand why the original code doesn't have to deal with exception?
--
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 #10653: Remove actually unsupported config that selectively enable nullable columns
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10653:
URL: https://github.com/apache/pinot/pull/10653#issuecomment-1515983152
## [Codecov](https://codecov.io/gh/apache/pinot/pull/10653?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 [#10653](https://codecov.io/gh/apache/pinot/pull/10653?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (708b787) into [master](https://codecov.io/gh/apache/pinot/commit/2d6a38c0a13aecb055ea1d81efdc4cc8dfba197a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2d6a38c) will **decrease** coverage by `35.41%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10653 +/- ##
=============================================
- Coverage 70.33% 34.93% -35.41%
+ Complexity 6499 462 -6037
=============================================
Files 2108 2108
Lines 113535 113541 +6
Branches 17117 17117
=============================================
- Hits 79859 39669 -40190
- Misses 28083 70413 +42330
+ Partials 5593 3459 -2134
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `24.50% <0.00%> (+<0.01%)` | :arrow_up: |
| integration2 | `23.96% <0.00%> (-0.18%)` | :arrow_down: |
| unittests1 | `?` | |
| unittests2 | `13.87% <0.00%> (-0.02%)` | :arrow_down: |
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/10653?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...creator/impl/nullvalue/NullValueVectorCreator.java](https://codecov.io/gh/apache/pinot/pull/10653?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9udWxsdmFsdWUvTnVsbFZhbHVlVmVjdG9yQ3JlYXRvci5qYXZh) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [...al/segment/index/nullvalue/NullValueIndexType.java](https://codecov.io/gh/apache/pinot/pull/10653?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: |
| [...che/pinot/segment/spi/index/AbstractIndexType.java](https://codecov.io/gh/apache/pinot/pull/10653?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2luZGV4L0Fic3RyYWN0SW5kZXhUeXBlLmphdmE=) | `0.00% <0.00%> (-83.34%)` | :arrow_down: |
| [...ment/spi/index/MergedColumnConfigDeserializer.java](https://codecov.io/gh/apache/pinot/pull/10653?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2luZGV4L01lcmdlZENvbHVtbkNvbmZpZ0Rlc2VyaWFsaXplci5qYXZh) | `0.00% <0.00%> (-75.87%)` | :arrow_down: |
... and [1212 files with indirect coverage changes](https://codecov.io/gh/apache/pinot/pull/10653/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 a diff in pull request #10653: Remove actually unsupported config that selectively enable nullable columns
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10653:
URL: https://github.com/apache/pinot/pull/10653#discussion_r1177411649
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/AbstractIndexType.java:
##########
@@ -51,7 +51,11 @@ public Map<String, C> getConfig(TableConfig tableConfig, Schema schema) {
if (_deserializer == null) {
_deserializer = createDeserializer();
}
- return _deserializer.deserialize(tableConfig, schema);
+ try {
+ return _deserializer.deserialize(tableConfig, schema);
Review Comment:
With which exception? `ConfigDeclaredTwiceException`? Here we are dealing with that exception. But it wraps it in order to also inform about which index type is the one that is failing. The code that throws the original exception doesn't have the context to know which index type is the one that is repeated.
--
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 #10653: Remove actually unsupported config that selectively enable nullable columns
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10653:
URL: https://github.com/apache/pinot/pull/10653#discussion_r1177416617
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/nullvalue/NullValueIndexType.java:
##########
@@ -67,14 +67,11 @@ public IndexConfig getDefaultConfig() {
@Override
public ColumnConfigDeserializer<IndexConfig> createDeserializer() {
- return IndexConfigDeserializer.fromIndexes("null", getIndexConfigClass())
- .withFallbackAlternative(
Review Comment:
It is one of the options on `MergedColumnConfigDeserializer`. Specifically, it uses `MergedColumnConfigDeserializer.OnConflict.PICK_FIRST`, which means that in case a configuration is specified twice, it will pick the first and silently skip the second.
In this case it meant that first it would look for something like:
```js
{
fieldConfigList: [
{
name: "whatever",
indexes: {
null: {} // this is what IndexConfigDeserializer.fromIndexes looks for
}
}
}
}
```
And if it is not found, it would look for the alternative which is to read `tableConfig.getIndexingConfig().isNullHandlingEnabled()`. To be more precise, both alternatives will always be read and then merged silently. That means that in case the config is incorrectly specified in one of the branches, we are going to produce a failure.
The new code removes the first part in order to do not look for nullability in `fieldConfigList`, which is desired because it is actually not supported, so we would be generating an extra index that would never be used.
--
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
Re: [PR] Remove actually unsupported config that selectively enable nullable columns [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #10653:
URL: https://github.com/apache/pinot/pull/10653
--
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