You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/08/12 19:40:16 UTC
[GitHub] [pinot] navina opened a new pull request, #9205: Max number of multi values should be set in DefaultNullValueProvider
navina opened a new pull request, #9205:
URL: https://github.com/apache/pinot/pull/9205
Related to #8819
This issue shows up when we try to add a new Multivalue column and reload the segments of the table. Once the consuming segment commits, it will throw an `ArrayIndexOutOfBoundsException` in the `ConstantMVForwardIndexReader` and stops ingestion.
--
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] walterddr commented on a diff in pull request #9205: Max number of multi values should be set in DefaultNullValueProvider
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9205:
URL: https://github.com/apache/pinot/pull/9205#discussion_r947279578
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/column/DefaultNullValueVirtualColumnProvider.java:
##########
@@ -85,7 +85,13 @@ public InvertedIndexReader<?> buildInvertedIndex(VirtualColumnContext context) {
@Override
public ColumnMetadataImpl buildMetadata(VirtualColumnContext context) {
- return getColumnMetadataBuilder(context).setCardinality(1).setSorted(context.getFieldSpec().isSingleValueField())
- .setHasDictionary(true).build();
+
+ ColumnMetadataImpl.Builder builder = getColumnMetadataBuilder(context).setCardinality(1).setHasDictionary(true);
+ if (context.getFieldSpec().isSingleValueField()) {
+ builder.setSorted(true);
+ } else {
+ builder.setMaxNumberOfMultiValues(1);
Review Comment:
why setting this to 1 ?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java:
##########
@@ -966,16 +966,21 @@ public Set<String> getPhysicalColumnNames() {
public DataSource getDataSource(String column) {
FieldSpec fieldSpec = _schema.getFieldSpecFor(column);
if (fieldSpec == null || fieldSpec.isVirtualColumn()) {
+ VirtualColumnContext virtualColumnContext = null;
// Column is either added during ingestion, or was initiated with a virtual column provider
if (fieldSpec == null) {
// If the column was added during ingestion, we will construct the column provider based on its fieldSpec to
// provide values
fieldSpec = _newlyAddedColumnsFieldMap.get(column);
Preconditions.checkNotNull(fieldSpec,
"FieldSpec for " + column + " should not be null. " + "Potentially invalid column name specified.");
+ // newly added column shouldn't have any doc count?
+ virtualColumnContext = new VirtualColumnContext(fieldSpec, 0);
Review Comment:
newly added column should have same doc count right? just cardinality will always be 1 due to only default null value will be select out?
--
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 #9205: Max number of multi values should be set in DefaultNullValueProvider
Posted by GitBox <gi...@apache.org>.
navina commented on code in PR #9205:
URL: https://github.com/apache/pinot/pull/9205#discussion_r947287042
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java:
##########
@@ -966,16 +966,21 @@ public Set<String> getPhysicalColumnNames() {
public DataSource getDataSource(String column) {
FieldSpec fieldSpec = _schema.getFieldSpecFor(column);
if (fieldSpec == null || fieldSpec.isVirtualColumn()) {
+ VirtualColumnContext virtualColumnContext = null;
// Column is either added during ingestion, or was initiated with a virtual column provider
if (fieldSpec == null) {
// If the column was added during ingestion, we will construct the column provider based on its fieldSpec to
// provide values
fieldSpec = _newlyAddedColumnsFieldMap.get(column);
Preconditions.checkNotNull(fieldSpec,
"FieldSpec for " + column + " should not be null. " + "Potentially invalid column name specified.");
+ // newly added column shouldn't have any doc count?
+ virtualColumnContext = new VirtualColumnContext(fieldSpec, 0);
Review Comment:
yep. old code works fine 👍
--
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 a diff in pull request #9205: Max number of multi values should be set in DefaultNullValueProvider
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #9205:
URL: https://github.com/apache/pinot/pull/9205#discussion_r946228992
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java:
##########
@@ -966,16 +966,21 @@ public Set<String> getPhysicalColumnNames() {
public DataSource getDataSource(String column) {
FieldSpec fieldSpec = _schema.getFieldSpecFor(column);
if (fieldSpec == null || fieldSpec.isVirtualColumn()) {
+ VirtualColumnContext virtualColumnContext = null;
// Column is either added during ingestion, or was initiated with a virtual column provider
if (fieldSpec == null) {
// If the column was added during ingestion, we will construct the column provider based on its fieldSpec to
// provide values
fieldSpec = _newlyAddedColumnsFieldMap.get(column);
Preconditions.checkNotNull(fieldSpec,
"FieldSpec for " + column + " should not be null. " + "Potentially invalid column name specified.");
+ // newly added column shouldn't have any doc count?
+ virtualColumnContext = new VirtualColumnContext(fieldSpec, 0);
Review Comment:
Why do we change this? I think the old code is correct
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/constant/ConstantMVForwardIndexReader.java:
##########
@@ -45,6 +45,9 @@ public DataType getStoredType() {
@Override
public int getDictIdMV(int docId, int[] dictIdBuffer, ForwardIndexReaderContext context) {
+ if (dictIdBuffer.length == 0) {
Review Comment:
This is not needed when `maxNumberOfMultiValues` is properly set
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/column/DefaultNullValueVirtualColumnProvider.java:
##########
@@ -85,7 +85,13 @@ public InvertedIndexReader<?> buildInvertedIndex(VirtualColumnContext context) {
@Override
public ColumnMetadataImpl buildMetadata(VirtualColumnContext context) {
- return getColumnMetadataBuilder(context).setCardinality(1).setSorted(context.getFieldSpec().isSingleValueField())
- .setHasDictionary(true).build();
+ if (context.getFieldSpec().isSingleValueField()) {
+ return getColumnMetadataBuilder(context).setCardinality(1).setSorted(true)
+ .setTotalDocs(context.getTotalDocCount())
+ .setHasDictionary(true).build();
+ }
+ return getColumnMetadataBuilder(context).setSorted(false).setTotalDocs(context.getTotalDocCount())
+ .setHasDictionary(true).setMaxNumberOfMultiValues(0)
+ .build();
Review Comment:
It should be set to 1
```suggestion
ColumnMetadataImpl.Builder builder = getColumnMetadataBuilder(context).setCardinality(1).setHasDictionary(true);
if (context.getFieldSpec().isSingleValueField()) {
builder.setSorted(true);
} else {
builder.setMaxNumberOfMultiValues(1);
}
return builder.build();
```
--
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 #9205: Max number of multi values should be set in DefaultNullValueProvider
Posted by GitBox <gi...@apache.org>.
navina commented on code in PR #9205:
URL: https://github.com/apache/pinot/pull/9205#discussion_r947282991
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java:
##########
@@ -966,16 +966,21 @@ public Set<String> getPhysicalColumnNames() {
public DataSource getDataSource(String column) {
FieldSpec fieldSpec = _schema.getFieldSpecFor(column);
if (fieldSpec == null || fieldSpec.isVirtualColumn()) {
+ VirtualColumnContext virtualColumnContext = null;
// Column is either added during ingestion, or was initiated with a virtual column provider
if (fieldSpec == null) {
// If the column was added during ingestion, we will construct the column provider based on its fieldSpec to
// provide values
fieldSpec = _newlyAddedColumnsFieldMap.get(column);
Preconditions.checkNotNull(fieldSpec,
"FieldSpec for " + column + " should not be null. " + "Potentially invalid column name specified.");
+ // newly added column shouldn't have any doc count?
+ virtualColumnContext = new VirtualColumnContext(fieldSpec, 0);
Review Comment:
ah I see. I thought the doc count here represents number of docs with non-null values for this column.
I wasn't sure about this change. Thanks for clarifying. Let me revert to old code and verify that the fix still works.
--
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 #9205: Max number of multi values should be set in DefaultNullValueProvider
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #9205:
URL: https://github.com/apache/pinot/pull/9205#issuecomment-1213556550
# [Codecov](https://codecov.io/gh/apache/pinot/pull/9205?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 [#9205](https://codecov.io/gh/apache/pinot/pull/9205?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1cb5565) into [master](https://codecov.io/gh/apache/pinot/commit/6bf696ec25f2571c252c07099b81f9876f839b91?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6bf696e) will **increase** coverage by `35.23%`.
> The diff coverage is `69.23%`.
> :exclamation: Current head 1cb5565 differs from pull request most recent head b4ed22a. Consider uploading reports for the commit b4ed22a to get more accurate results
```diff
@@ Coverage Diff @@
## master #9205 +/- ##
=============================================
+ Coverage 28.36% 63.59% +35.23%
- Complexity 53 4985 +4932
=============================================
Files 1842 1804 -38
Lines 98517 96704 -1813
Branches 15005 14794 -211
=============================================
+ Hits 27944 61499 +33555
+ Misses 67879 30721 -37158
- Partials 2694 4484 +1790
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `67.10% <69.23%> (?)` | |
| unittests2 | `15.29% <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/9205?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...readers/constant/ConstantMVForwardIndexReader.java](https://codecov.io/gh/apache/pinot/pull/9205/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L3JlYWRlcnMvY29uc3RhbnQvQ29uc3RhbnRNVkZvcndhcmRJbmRleFJlYWRlci5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...al/segment/virtualcolumn/VirtualColumnContext.java](https://codecov.io/gh/apache/pinot/pull/9205/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L3ZpcnR1YWxjb2x1bW4vVmlydHVhbENvbHVtbkNvbnRleHQuamF2YQ==) | `100.00% <ø> (+100.00%)` | :arrow_up: |
| [...local/indexsegment/mutable/MutableSegmentImpl.java](https://codecov.io/gh/apache/pinot/pull/9205/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9pbmRleHNlZ21lbnQvbXV0YWJsZS9NdXRhYmxlU2VnbWVudEltcGwuamF2YQ==) | `54.16% <50.00%> (+54.16%)` | :arrow_up: |
| [.../column/DefaultNullValueVirtualColumnProvider.java](https://codecov.io/gh/apache/pinot/pull/9205/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2NvbHVtbi9EZWZhdWx0TnVsbFZhbHVlVmlydHVhbENvbHVtblByb3ZpZGVyLmphdmE=) | `70.83% <100.00%> (+70.83%)` | :arrow_up: |
| [...va/org/apache/pinot/core/routing/RoutingTable.java](https://codecov.io/gh/apache/pinot/pull/9205/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yb3V0aW5nL1JvdXRpbmdUYWJsZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...va/org/apache/pinot/common/config/NettyConfig.java](https://codecov.io/gh/apache/pinot/pull/9205/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL05ldHR5Q29uZmlnLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...a/org/apache/pinot/common/metrics/MinionMeter.java](https://codecov.io/gh/apache/pinot/pull/9205/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...g/apache/pinot/common/metrics/ControllerMeter.java](https://codecov.io/gh/apache/pinot/pull/9205/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Db250cm9sbGVyTWV0ZXIuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/pinot/pull/9205/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJRdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/common/metrics/MinionQueryPhase.java](https://codecov.io/gh/apache/pinot/pull/9205/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9NaW5pb25RdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1516 more](https://codecov.io/gh/apache/pinot/pull/9205/diff?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] navina commented on pull request #9205: Max number of multi values should be set in DefaultNullValueProvider
Posted by GitBox <gi...@apache.org>.
navina commented on PR #9205:
URL: https://github.com/apache/pinot/pull/9205#issuecomment-1213455624
@Jackie-Jiang / @KKcorps : please review. ty!
--
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 #9205: Max number of multi values should be set in DefaultNullValueProvider
Posted by GitBox <gi...@apache.org>.
navina commented on code in PR #9205:
URL: https://github.com/apache/pinot/pull/9205#discussion_r948321018
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/constant/ConstantMVForwardIndexReader.java:
##########
@@ -45,6 +45,9 @@ public DataType getStoredType() {
@Override
public int getDictIdMV(int docId, int[] dictIdBuffer, ForwardIndexReaderContext context) {
+ if (dictIdBuffer.length == 0) {
Review Comment:
Discussed with @Jackie-Jiang offline. Looks like `dictIdBuffer.length > 0` is the expected invariant for this method due to how MV column values are stored in the bitmap. I have added some javadocs so that future contributors are aware of this. Please do take a look. 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] navina commented on a diff in pull request #9205: Max number of multi values should be set in DefaultNullValueProvider
Posted by GitBox <gi...@apache.org>.
navina commented on code in PR #9205:
URL: https://github.com/apache/pinot/pull/9205#discussion_r947295408
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/column/DefaultNullValueVirtualColumnProvider.java:
##########
@@ -85,7 +85,13 @@ public InvertedIndexReader<?> buildInvertedIndex(VirtualColumnContext context) {
@Override
public ColumnMetadataImpl buildMetadata(VirtualColumnContext context) {
- return getColumnMetadataBuilder(context).setCardinality(1).setSorted(context.getFieldSpec().isSingleValueField())
- .setHasDictionary(true).build();
+
+ ColumnMetadataImpl.Builder builder = getColumnMetadataBuilder(context).setCardinality(1).setHasDictionary(true);
+ if (context.getFieldSpec().isSingleValueField()) {
+ builder.setSorted(true);
+ } else {
+ builder.setMaxNumberOfMultiValues(1);
Review Comment:
good catch. This should be 0.
--
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 #9205: Max number of multi values should be set in DefaultNullValueProvider
Posted by GitBox <gi...@apache.org>.
navina commented on code in PR #9205:
URL: https://github.com/apache/pinot/pull/9205#discussion_r947296365
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/column/DefaultNullValueVirtualColumnProvider.java:
##########
@@ -85,7 +85,13 @@ public InvertedIndexReader<?> buildInvertedIndex(VirtualColumnContext context) {
@Override
public ColumnMetadataImpl buildMetadata(VirtualColumnContext context) {
- return getColumnMetadataBuilder(context).setCardinality(1).setSorted(context.getFieldSpec().isSingleValueField())
- .setHasDictionary(true).build();
+
+ ColumnMetadataImpl.Builder builder = getColumnMetadataBuilder(context).setCardinality(1).setHasDictionary(true);
+ if (context.getFieldSpec().isSingleValueField()) {
+ builder.setSorted(true);
+ } else {
+ builder.setMaxNumberOfMultiValues(1);
Review Comment:
fixed
--
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 #9205: Max number of multi values should be set in DefaultNullValueProvider
Posted by GitBox <gi...@apache.org>.
navina commented on code in PR #9205:
URL: https://github.com/apache/pinot/pull/9205#discussion_r947298294
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/constant/ConstantMVForwardIndexReader.java:
##########
@@ -45,6 +45,9 @@ public DataType getStoredType() {
@Override
public int getDictIdMV(int docId, int[] dictIdBuffer, ForwardIndexReaderContext context) {
+ if (dictIdBuffer.length == 0) {
Review Comment:
This looks like a code smell and is better to be more defensive. There is no guarantee that a caller to this method knows how `maxNumberOfMultiValues` influences the behavior of this method.
--
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 #9205: Max number of multi values should be set in DefaultNullValueProvider
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged PR #9205:
URL: https://github.com/apache/pinot/pull/9205
--
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