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/06/22 14:39:19 UTC
[GitHub] [pinot] somandal opened a new pull request, #8953: Add multi-value raw APIs and their implementation for querying immutable and mutable fixed-width segment types
somandal opened a new pull request, #8953:
URL: https://github.com/apache/pinot/pull/8953
This PR is the first of a series of PRs that adds support for querying multi-value columns in RAW format (without dictionary). Support to create segments with multi-value columns with dictionary disabled already existed. On trying to create a multi-value RAW column and querying it, we ran into this issue: https://github.com/apache/pinot/issues/8875
This PR adds MV RAW APIs and their implementation for the following types of columns:
Offline segments with MV columns of types INT, LONG, FLOAT, DOUBLE, and STRING
Mutable (realtime) segments with MV columns of INT, LONG, FLOAT, and DOUBLE
TODO - Support for variable length MV RAW columns for Mutable segments will be added in the future as it requires a mutable variable length writer.
TODO - Make group by, filter, etc work correctly with MV raw columns (this will be addressed in a future PR)
Test cases for various types of queries that are currently supported have been added.
cc @siddharthteotia @Jackie-Jiang
--
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] somandal commented on a diff in pull request #8953: Add multi-value noDict APIs and their implementation for querying immutable and mutable fixed-width segment types
Posted by GitBox <gi...@apache.org>.
somandal commented on code in PR #8953:
URL: https://github.com/apache/pinot/pull/8953#discussion_r905600098
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/reader/ForwardIndexReader.java:
##########
@@ -410,9 +421,303 @@ default byte[] getBytes(int docId, T context) {
/**
* MULTI-VALUE COLUMN RAW INDEX APIs
- * TODO: Not supported yet
*/
+ /**
+ * Fills the values
+ * @param docIds Array containing the document ids to read
+ * @param length Number of values to read
+ * @param values Values to fill
+ * @param context Reader context
+ */
+ default void readValuesMV(int[] docIds, int length, int[][] values, T context) {
+ switch (getValueType()) {
+ case INT:
+ for (int i = 0; i < length; i++) {
+ values[i] = getIntMV(docIds[i], context);
+ }
+ break;
+ case LONG:
+ for (int i = 0; i < length; i++) {
+ long[] longValueBuffer = getLongMV(docIds[i], context);
Review Comment:
I am not sure I fully understand what you mean by reuse the buffer here. Can you elaborate?
This is my understanding. In this function, we need to create a buffer based on the `valueType` available. i.e. if the valueType is double, we need to create a double[] buffer and if it is string, we need a String[] buffer. We cannot know beforehand which buffer type to create as it depends on `getValueType()`.
Even if I pass `maxNumValuesPerMVEntry`, I'll have to allocate a typed buffer here. How is this different from me creating the buffer as part of `getLongMV()` or one of the other APIs? I will just create it here, pass it to the `getLongMV()` function, typecast the values in the filled in buffer to the desired values[][] type and store it in values[][].
Or are you suggesting that we create a buffer depending on the `valueType` of the caller (e.g. `DataFetcher` knows the `valueType`) within the caller and pass that pre-allocated buffer of `maxNumValuesPerMVEntry` size down to the `ForwardIndexReader`? I have 2 questions regarding this, a) will the `valueType` in the callers like `DataFetcher` match the `valueType` of the ForwardIndexReader for all purposes? and b) the number of APIs we need to add to the `ForwardIndexReader` will explode as we'll need one for each buffer type + values[][] type combination. Is this acceptable?
I may be misunderstanding your comment, so do let me know what you mean.
--
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 #8953: Add multi-value noDict APIs and their implementation for querying immutable and mutable fixed-width segment types
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #8953:
URL: https://github.com/apache/pinot/pull/8953#discussion_r906222092
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/reader/ForwardIndexReader.java:
##########
@@ -410,9 +421,303 @@ default byte[] getBytes(int docId, T context) {
/**
* MULTI-VALUE COLUMN RAW INDEX APIs
- * TODO: Not supported yet
*/
+ /**
+ * Fills the values
+ * @param docIds Array containing the document ids to read
+ * @param length Number of values to read
+ * @param values Values to fill
+ * @param context Reader context
+ */
+ default void readValuesMV(int[] docIds, int length, int[][] values, T context) {
+ switch (getValueType()) {
+ case INT:
+ for (int i = 0; i < length; i++) {
+ values[i] = getIntMV(docIds[i], context);
+ }
+ break;
+ case LONG:
+ for (int i = 0; i < length; i++) {
+ long[] longValueBuffer = getLongMV(docIds[i], context);
Review Comment:
Essentially we can avoid creating a separate array for each iteration of the for look (similar to the dict ids buffer). Let me explain with code:
```suggestion
long[] longValuesBuffer = new long[maxNumValuesPerMVEntry];
for (int i = 0; i < length; i++) {
int numValues = getLongMV(docIds[i], longValuesBuffer, context);
int[] intValues = new int[numValues];
for (int j = 0; j < numValues; j++) {
intValues[j] = (int) longValuesBuffer[j];
}
values[i] = intValues;
}
```
--
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] siddharthteotia commented on a diff in pull request #8953: Add multi-value noDict APIs and their implementation for querying immutable and mutable fixed-width segment types
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #8953:
URL: https://github.com/apache/pinot/pull/8953#discussion_r909088699
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/fwd/MultiValueFixedByteRawIndexCreator.java:
##########
@@ -77,7 +77,8 @@ public MultiValueFixedByteRawIndexCreator(File baseIndexDir, ChunkCompressionTyp
boolean deriveNumDocsPerChunk, int writerVersion)
throws IOException {
File file = new File(baseIndexDir, column + Indexes.RAW_MV_FORWARD_INDEX_FILE_EXTENSION);
- int totalMaxLength = maxNumberOfMultiValueElements * valueType.getStoredType().size();
+ // Store the length followed by the values
+ int totalMaxLength = Integer.BYTES + (maxNumberOfMultiValueElements * valueType.getStoredType().size());
Review Comment:
Just noticed this. I wonder how the reader was working before because without this change, looks like we can't correctly deserialize / read the bytes written by MV raw index writer ?
--
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] somandal commented on a diff in pull request #8953: Add multi-value noDict APIs and their implementation for querying immutable and mutable fixed-width segment types
Posted by GitBox <gi...@apache.org>.
somandal commented on code in PR #8953:
URL: https://github.com/apache/pinot/pull/8953#discussion_r909062567
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/mutable/MutableForwardIndex.java:
##########
@@ -92,11 +92,26 @@ default int getDictIdMV(int docId, int[] dictIdBuffer) {
throw new UnsupportedOperationException();
}
+ /**
+ * Reads the dictionary ids for a multi-value column at the given document id into a buffer and returns the buffer.
+ *
+ * @param docId Document id
+ * @return A buffer containing the multi-value entries
+ */
+ default int[] getDictIdMV(int docId) {
Review Comment:
So I didn't see a need to change the setDictIdMV() API as it already takes an array of dictIDs and doesn't need to return anything:
```
default void setDictIdMV(int docId, int[] dictIds) {
throw new UnsupportedOperationException();
}
```
The above is implemented by only `FixedByteMVMutableForwardIndex` today.
--
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] somandal commented on pull request #8953: Add multi-value noDict APIs and their implementation for querying immutable and mutable fixed-width segment types
Posted by GitBox <gi...@apache.org>.
somandal commented on PR #8953:
URL: https://github.com/apache/pinot/pull/8953#issuecomment-1169366106
> looks good to me overall. I think some of the code paths are not covered in unit tests. I watched some but it would be nice to cover a bit more
>
> if the other 2 PRs that are following this one is going to cover those then nvm.
Yes I have much more exhaustive tests in the other PRs. I'll send out the next one as soon as this one is merged. Hope that's okay
--
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] somandal commented on a diff in pull request #8953: Add multi-value noDict APIs and their implementation for querying immutable and mutable fixed-width segment types
Posted by GitBox <gi...@apache.org>.
somandal commented on code in PR #8953:
URL: https://github.com/apache/pinot/pull/8953#discussion_r906253811
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/reader/ForwardIndexReader.java:
##########
@@ -410,9 +421,303 @@ default byte[] getBytes(int docId, T context) {
/**
* MULTI-VALUE COLUMN RAW INDEX APIs
- * TODO: Not supported yet
*/
+ /**
+ * Fills the values
+ * @param docIds Array containing the document ids to read
+ * @param length Number of values to read
+ * @param values Values to fill
+ * @param context Reader context
+ */
+ default void readValuesMV(int[] docIds, int length, int[][] values, T context) {
+ switch (getValueType()) {
+ case INT:
+ for (int i = 0; i < length; i++) {
+ values[i] = getIntMV(docIds[i], context);
+ }
+ break;
+ case LONG:
+ for (int i = 0; i < length; i++) {
+ long[] longValueBuffer = getLongMV(docIds[i], context);
Review Comment:
oh of course! completely missed that aspect of reusability. thanks. I will make this change.
--
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 #8953: Add multi-value noDict APIs and their implementation for querying immutable and mutable fixed-width segment types
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #8953:
URL: https://github.com/apache/pinot/pull/8953#discussion_r909027521
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/reader/ForwardIndexReader.java:
##########
@@ -496,4 +861,26 @@ default int getStringMV(int docId, String[] valueBuffer, T context) {
default int getBytesMV(int docId, byte[][] valueBuffer, T context) {
throw new UnsupportedOperationException();
}
+
+ /**
+ * Reads the bytes type multi-value at the given document id.
+ *
+ * @param docId Document id
+ * @param context Reader context
+ * @return BYTE values at the given document id
+ */
+ default byte[][] getBytesMV(int docId, T context) {
Review Comment:
i dont think this is tested. should we add a readBytesValues() method in DataFetcher ?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java:
##########
@@ -436,9 +436,10 @@ private boolean isNoDictionaryColumn(Set<String> noDictionaryColumns, Set<String
column, dataType.toString());
return false;
}
- // So don't create dictionary if the column is member of noDictionary, is single-value
- // and doesn't have an inverted index
- return fieldSpec.isSingleValueField() && !invertedIndexColumns.contains(column);
+ // So don't create dictionary if the column is member of noDictionary, is single-value or multi-value with a
+ // fixed-width field and doesn't have an inverted index
Review Comment:
```suggestion
// So don't create dictionary if the column (1) is member of noDictionary, and (2) is single-value or multi-value with a
// fixed-width field, and (3) doesn't have an inverted index
```
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/mutable/MutableForwardIndex.java:
##########
@@ -92,11 +92,26 @@ default int getDictIdMV(int docId, int[] dictIdBuffer) {
throw new UnsupportedOperationException();
}
+ /**
+ * Reads the dictionary ids for a multi-value column at the given document id into a buffer and returns the buffer.
+ *
+ * @param docId Document id
+ * @return A buffer containing the multi-value entries
+ */
+ default int[] getDictIdMV(int docId) {
Review Comment:
this and others -->there's also a setDictIdMV APIs. do we plan to add those?
--
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 #8953: Add multi-value noDict APIs and their implementation for querying immutable and mutable fixed-width segment types
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #8953:
URL: https://github.com/apache/pinot/pull/8953#discussion_r906222092
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/reader/ForwardIndexReader.java:
##########
@@ -410,9 +421,303 @@ default byte[] getBytes(int docId, T context) {
/**
* MULTI-VALUE COLUMN RAW INDEX APIs
- * TODO: Not supported yet
*/
+ /**
+ * Fills the values
+ * @param docIds Array containing the document ids to read
+ * @param length Number of values to read
+ * @param values Values to fill
+ * @param context Reader context
+ */
+ default void readValuesMV(int[] docIds, int length, int[][] values, T context) {
+ switch (getValueType()) {
+ case INT:
+ for (int i = 0; i < length; i++) {
+ values[i] = getIntMV(docIds[i], context);
+ }
+ break;
+ case LONG:
+ for (int i = 0; i < length; i++) {
+ long[] longValueBuffer = getLongMV(docIds[i], context);
Review Comment:
Essentially we can avoid creating a separate array for each iteration of the for look (similar to the dict ids buffer). Let me explain with code:
```suggestion
long[] longValuesBuffer = new long[maxNumValuesPerMVEntry];
for (int i = 0; i < length; i++) {
int numValues = getLongMV(docIds[i], longValuesBuffer, context);
int[] intValues = new int[numValues];
for (int j = 0; j < numValues; j++) {
intValues[j] = (int) longValuesBuffer[j];
}
...
}
```
--
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 #8953: Add multi-value noDict APIs and their implementation for querying immutable and mutable fixed-width segment types
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #8953:
URL: https://github.com/apache/pinot/pull/8953#issuecomment-1165819411
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8953?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 [#8953](https://codecov.io/gh/apache/pinot/pull/8953?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c3b469a) into [master](https://codecov.io/gh/apache/pinot/commit/3a8079f4a140f48c24cfa6b195584f0cb67771fc?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3a8079f) will **decrease** coverage by `6.74%`.
> The diff coverage is `37.39%`.
```diff
@@ Coverage Diff @@
## master #8953 +/- ##
============================================
- Coverage 69.78% 63.04% -6.75%
+ Complexity 4883 4715 -168
============================================
Files 1817 1770 -47
Lines 94758 93139 -1619
Branches 14170 14022 -148
============================================
- Hits 66131 58716 -7415
- Misses 23999 30186 +6187
+ Partials 4628 4237 -391
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `66.30% <37.39%> (+0.03%)` | :arrow_up: |
| unittests2 | `14.95% <0.00%> (-0.05%)` | :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/8953?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ocal/indexsegment/mutable/IntermediateSegment.java](https://codecov.io/gh/apache/pinot/pull/8953/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9pbmRleHNlZ21lbnQvbXV0YWJsZS9JbnRlcm1lZGlhdGVTZWdtZW50LmphdmE=) | `76.51% <ø> (ø)` | |
| [...readers/constant/ConstantMVForwardIndexReader.java](https://codecov.io/gh/apache/pinot/pull/8953/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%> (ø)` | |
| [...t/local/startree/v2/store/StarTreeLoaderUtils.java](https://codecov.io/gh/apache/pinot/pull/8953/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zdGFydHJlZS92Mi9zdG9yZS9TdGFyVHJlZUxvYWRlclV0aWxzLmphdmE=) | `91.11% <ø> (ø)` | |
| [...segment/spi/index/mutable/MutableForwardIndex.java](https://codecov.io/gh/apache/pinot/pull/8953/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-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2luZGV4L211dGFibGUvTXV0YWJsZUZvcndhcmRJbmRleC5qYXZh) | `8.47% <0.00%> (+4.03%)` | :arrow_up: |
| [...e/impl/forward/FixedByteMVMutableForwardIndex.java](https://codecov.io/gh/apache/pinot/pull/8953/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9pbXBsL2ZvcndhcmQvRml4ZWRCeXRlTVZNdXRhYmxlRm9yd2FyZEluZGV4LmphdmE=) | `70.78% <6.12%> (-23.16%)` | :arrow_down: |
| [...local/indexsegment/mutable/MutableSegmentImpl.java](https://codecov.io/gh/apache/pinot/pull/8953/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.89% <20.98%> (-3.74%)` | :arrow_down: |
| [...t/segment/spi/index/reader/ForwardIndexReader.java](https://codecov.io/gh/apache/pinot/pull/8953/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-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2luZGV4L3JlYWRlci9Gb3J3YXJkSW5kZXhSZWFkZXIuamF2YQ==) | `44.55% <26.96%> (-25.05%)` | :arrow_down: |
| [.../readers/forward/FixedBitMVForwardIndexReader.java](https://codecov.io/gh/apache/pinot/pull/8953/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L3JlYWRlcnMvZm9yd2FyZC9GaXhlZEJpdE1WRm9yd2FyZEluZGV4UmVhZGVyLmphdmE=) | `72.28% <40.54%> (-25.54%)` | :arrow_down: |
| [...x/readers/forward/BaseChunkForwardIndexReader.java](https://codecov.io/gh/apache/pinot/pull/8953/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L3JlYWRlcnMvZm9yd2FyZC9CYXNlQ2h1bmtGb3J3YXJkSW5kZXhSZWFkZXIuamF2YQ==) | `46.49% <46.15%> (ø)` | |
| [...ders/forward/VarByteChunkMVForwardIndexReader.java](https://codecov.io/gh/apache/pinot/pull/8953/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L3JlYWRlcnMvZm9yd2FyZC9WYXJCeXRlQ2h1bmtNVkZvcndhcmRJbmRleFJlYWRlci5qYXZh) | `79.43% <47.22%> (-16.40%)` | :arrow_down: |
| ... and [445 more](https://codecov.io/gh/apache/pinot/pull/8953/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) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8953?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8953?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [3a8079f...c3b469a](https://codecov.io/gh/apache/pinot/pull/8953?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] siddharthteotia commented on pull request #8953: Add multi-value noDict APIs and their implementation for querying immutable and mutable fixed-width segment types
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on PR #8953:
URL: https://github.com/apache/pinot/pull/8953#issuecomment-1169368374
> > looks good to me overall. I think some of the code paths are not covered in unit tests. I watched some but it would be nice to cover a bit more
> > if the other 2 PRs that are following this one is going to cover those then nvm.
>
> Yes I have much more exhaustive tests in the other PRs. I'll send out the next one as soon as this one is merged. Hope that's okay
yes @walterddr - the original PR https://github.com/apache/pinot/pull/8917 by @somandal where this piece was split from has all comprehensive tests. When the split was done, splitting tests was not super straightforward. When the final split is merged, all remaining tests will be 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] Jackie-Jiang commented on a diff in pull request #8953: Add multi-value noDict APIs and their implementation for querying immutable and mutable fixed-width segment types
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #8953:
URL: https://github.com/apache/pinot/pull/8953#discussion_r905557641
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/reader/ForwardIndexReader.java:
##########
@@ -410,9 +421,303 @@ default byte[] getBytes(int docId, T context) {
/**
* MULTI-VALUE COLUMN RAW INDEX APIs
- * TODO: Not supported yet
*/
+ /**
+ * Fills the values
+ * @param docIds Array containing the document ids to read
+ * @param length Number of values to read
+ * @param values Values to fill
+ * @param context Reader context
+ */
+ default void readValuesMV(int[] docIds, int length, int[][] values, T context) {
+ switch (getValueType()) {
+ case INT:
+ for (int i = 0; i < length; i++) {
+ values[i] = getIntMV(docIds[i], context);
+ }
+ break;
+ case LONG:
+ for (int i = 0; i < length; i++) {
+ long[] longValueBuffer = getLongMV(docIds[i], context);
Review Comment:
We should be able to reuse the buffer when we need to cast the values. We can pass in the `maxNumValuesPerMVEntry` and use that to create the reusable buffer
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/reader/ForwardIndexReader.java:
##########
@@ -100,6 +100,17 @@ default int getDictIdMV(int docId, int[] dictIdBuffer, T context) {
throw new UnsupportedOperationException();
}
+ /**
+ * Reads the dictionary ids for a multi-value column at the given document id into a buffer and returns the buffer.
+ *
+ * @param docId Document id
+ * @param context Reader context
+ * @return A buffer containing the multi-value entries
+ */
Review Comment:
(minor) Suggest modifying the javadoc a little bit as the buffer is a little bit confusing. Same for other methods
```suggestion
/**
* Reads the dictionary ids for a multi-value column at the given document id.
*
* @param docId Document id
* @param context Reader context
* @return Dictionary ids at the given document id
*/
```
--
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] siddharthteotia commented on pull request #8953: Add multi-value noDict APIs and their implementation for querying immutable and mutable fixed-width segment types
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on PR #8953:
URL: https://github.com/apache/pinot/pull/8953#issuecomment-1169573706
@somandal will follow up with part 2 and 3 as discussed on original PR https://github.com/apache/pinot/pull/8917
--
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] somandal commented on a diff in pull request #8953: Add multi-value noDict APIs and their implementation for querying immutable and mutable fixed-width segment types
Posted by GitBox <gi...@apache.org>.
somandal commented on code in PR #8953:
URL: https://github.com/apache/pinot/pull/8953#discussion_r909089594
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/fwd/MultiValueFixedByteRawIndexCreator.java:
##########
@@ -77,7 +77,8 @@ public MultiValueFixedByteRawIndexCreator(File baseIndexDir, ChunkCompressionTyp
boolean deriveNumDocsPerChunk, int writerVersion)
throws IOException {
File file = new File(baseIndexDir, column + Indexes.RAW_MV_FORWARD_INDEX_FILE_EXTENSION);
- int totalMaxLength = maxNumberOfMultiValueElements * valueType.getStoredType().size();
+ // Store the length followed by the values
+ int totalMaxLength = Integer.BYTES + (maxNumberOfMultiValueElements * valueType.getStoredType().size());
Review Comment:
I don't think this code path was tested correctly (especially on the read path for non-integer types). I only ran into issues (I believe it was a buffer overflow) when I tried LONG and DOUBLE type columns. Then spent time understanding what the code does and realized that the size is missing.
--
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] somandal commented on a diff in pull request #8953: Add multi-value noDict APIs and their implementation for querying immutable and mutable fixed-width segment types
Posted by GitBox <gi...@apache.org>.
somandal commented on code in PR #8953:
URL: https://github.com/apache/pinot/pull/8953#discussion_r910139202
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/fwd/MultiValueFixedByteRawIndexCreator.java:
##########
@@ -77,7 +77,8 @@ public MultiValueFixedByteRawIndexCreator(File baseIndexDir, ChunkCompressionTyp
boolean deriveNumDocsPerChunk, int writerVersion)
throws IOException {
File file = new File(baseIndexDir, column + Indexes.RAW_MV_FORWARD_INDEX_FILE_EXTENSION);
- int totalMaxLength = maxNumberOfMultiValueElements * valueType.getStoredType().size();
+ // Store the length followed by the values
+ int totalMaxLength = Integer.BYTES + (maxNumberOfMultiValueElements * valueType.getStoredType().size());
Review Comment:
So the main finding:
This change is needed because today when we setup the `totalMaxLength` without this change, we don't account for the additional 'length' of the MV row that we need to add. The writer accounts for the additional bytes used in the header as the offset but not this 'length' of the MV row since this writer is used by both SV and MV types. With this change, the reader side will also get the current max length and work as expected (otherwise we would have run into issues on the reader side too).
Next question to answer was why the test `MultiValueFixedByteRawIndexCreatorTest` wasn't failing. My findings on that:
We calculate the worst case chunk size large enough to fit all of the multi-value rows in the below code:
```
int totalMaxLength = maxNumberOfMultiValueElements * valueType.getStoredType().size();
chunkSize = numDocsPerChunk * (CHUNK_HEADER_ENTRY_ROW_OFFSET_SIZE + (long) totalMaxLength)
```
Which means that if you have different number of multi-value entries per MV row, they collectively won't take up the full size of the chunk, as some rows will be smaller than `totalMaxLength` and some will the at `totalMaxLength`.
You need sufficient multi-value rows to have 'maxNumberOfMultiValueElements' or enough elements to fully fill up the chunk (due to the header taking up totalDocs * 4 bytes, we will hit the buffer overflow before we write out all docs in case all MVs are at the max length). In the test, the number of entries per MV row varies greatly, and the total collective length was less than the space for data in the chunk buffer. This means we just don't exhaust the full chunk size available. thus never hitting a buffer overflow error.
To validate this theory, I modified one of the functions to always choose 49 elements per MV int array like this (instead of choosing the array length at random):
```
private static List<int[]> ints() {
return IntStream.range(0, 1000)
.mapToObj(i -> new int[49])
.peek(array -> {
for (int i = 0; i < array.length; i++) {
array[i] = RANDOM.nextInt();
}
})
.collect(Collectors.toList());
}
```
I was able to reproduce the buffer overflow (and this test passed when I added the Integer.BYTES to the 'totalMaxLength' above):
```
java.nio.BufferOverflowException
at java.base/java.nio.DirectByteBuffer.put(DirectByteBuffer.java:409)
at java.base/java.nio.ByteBuffer.put(ByteBuffer.java:906)
at org.apache.pinot.segment.local.io.writer.impl.VarByteChunkSVForwardIndexWriter.putBytes(VarByteChunkSVForwardIndexWriter.java:108)
at org.apache.pinot.segment.local.segment.creator.impl.fwd.MultiValueFixedByteRawIndexCreator.putIntMV(MultiValueFixedByteRawIndexCreator.java:119)
at org.apache.pinot.segment.local.segment.index.creator.MultiValueFixedByteRawIndexCreatorTest.lambda$testMV$14(MultiValueFixedByteRawIndexCreatorTest.java:124)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1541)
at org.apache.pinot.segment.local.segment.index.creator.MultiValueFixedByteRawIndexCreatorTest.testMV(MultiValueFixedByteRawIndexCreatorTest.java:124)
at org.apache.pinot.segment.local.segment.index.creator.MultiValueFixedByteRawIndexCreatorTest.testMVInt(MultiValueFixedByteRawIndexCreatorTest.java:75)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:566)
at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:108)
at org.testng.internal.Invoker.invokeMethod(Invoker.java:661)
at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:869)
at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1193)
at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:126)
at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:109)
at org.testng.TestRunner.privateRun(TestRunner.java:744)
at org.testng.TestRunner.run(TestRunner.java:602)
at org.testng.SuiteRunner.runTest(SuiteRunner.java:380)
at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:375)
at org.testng.SuiteRunner.privateRun(SuiteRunner.java:340)
at org.testng.SuiteRunner.run(SuiteRunner.java:289)
at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:52)
at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:86)
at org.testng.TestNG.runSuitesSequentially(TestNG.java:1301)
at org.testng.TestNG.runSuitesLocally(TestNG.java:1226)
at org.testng.TestNG.runSuites(TestNG.java:1144)
at org.testng.TestNG.run(TestNG.java:1115)
at com.intellij.rt.testng.IDEARemoteTestNG.run(IDEARemoteTestNG.java:66)
at com.intellij.rt.testng.RemoteTestNGStarter.main(RemoteTestNGStarter.java:109)
```
--
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] somandal commented on a diff in pull request #8953: Add multi-value noDict APIs and their implementation for querying immutable and mutable fixed-width segment types
Posted by GitBox <gi...@apache.org>.
somandal commented on code in PR #8953:
URL: https://github.com/apache/pinot/pull/8953#discussion_r909089594
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/fwd/MultiValueFixedByteRawIndexCreator.java:
##########
@@ -77,7 +77,8 @@ public MultiValueFixedByteRawIndexCreator(File baseIndexDir, ChunkCompressionTyp
boolean deriveNumDocsPerChunk, int writerVersion)
throws IOException {
File file = new File(baseIndexDir, column + Indexes.RAW_MV_FORWARD_INDEX_FILE_EXTENSION);
- int totalMaxLength = maxNumberOfMultiValueElements * valueType.getStoredType().size();
+ // Store the length followed by the values
+ int totalMaxLength = Integer.BYTES + (maxNumberOfMultiValueElements * valueType.getStoredType().size());
Review Comment:
I don't think this code path was tested correctly (especially on the read path for non-integer types). I only ran into issues (I believe it was a buffer overflow) when I tried LONG and DOUBLE type columns. Then spent time understanding what the code does and realized that the size is missing.
And just to note, this class correctly sets the length. It just didn't account for it in the size.
--
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] siddharthteotia commented on pull request #8953: Add multi-value noDict APIs and their implementation for querying immutable and mutable fixed-width segment types
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on PR #8953:
URL: https://github.com/apache/pinot/pull/8953#issuecomment-1168101133
Hi @walterddr - do you want to take a look at this PR since Jackie is out ?
Context for you:
Original issue for adding query support for MV raw column - https://github.com/apache/pinot/issues/8875
Original PR with a lot of tests that has complete code for the above issue - https://github.com/apache/pinot/pull/8917
This is part 1 split from original PR to help make review easier per discussion with Jackie. 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] somandal commented on a diff in pull request #8953: Add multi-value noDict APIs and their implementation for querying immutable and mutable fixed-width segment types
Posted by GitBox <gi...@apache.org>.
somandal commented on code in PR #8953:
URL: https://github.com/apache/pinot/pull/8953#discussion_r905600098
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/reader/ForwardIndexReader.java:
##########
@@ -410,9 +421,303 @@ default byte[] getBytes(int docId, T context) {
/**
* MULTI-VALUE COLUMN RAW INDEX APIs
- * TODO: Not supported yet
*/
+ /**
+ * Fills the values
+ * @param docIds Array containing the document ids to read
+ * @param length Number of values to read
+ * @param values Values to fill
+ * @param context Reader context
+ */
+ default void readValuesMV(int[] docIds, int length, int[][] values, T context) {
+ switch (getValueType()) {
+ case INT:
+ for (int i = 0; i < length; i++) {
+ values[i] = getIntMV(docIds[i], context);
+ }
+ break;
+ case LONG:
+ for (int i = 0; i < length; i++) {
+ long[] longValueBuffer = getLongMV(docIds[i], context);
Review Comment:
I am not sure I fully understand what you mean by reuse the buffer here. Can you elaborate?
This is my understanding. In this function, we need to create a buffer based on the `valueType` available? i.e. if the valueType is double, we need to create a double[] buffer and if it is string, we need a String[] buffer. We cannot know beforehand which buffer type to create as it depends on `getValueType()`.
Even if I pass `maxNumValuesPerMVEntry`, I'll have to allocate a typed buffer here? How is this different from me creating the buffer as part of `getLongMV()` or one of the other APIs? I will just create it here, pass it to the `getLongMV()` function, typecast the values in the filled in buffer to the desired values[][] type and store it in values[][].
Callers of this API may know up front about the type they have (e.g. `DataFetcher`), but we'd still have to pre-allocate all buffer types up front as reusable buffers within the `DataFetcher` and then the number of APIs we need to add to the `ForwardIndexReader` will explode as we'll need one for each buffer type + values[][] type combination.
I may be misunderstanding your comment, so do let me know what you mean.
--
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] siddharthteotia commented on a diff in pull request #8953: Add multi-value noDict APIs and their implementation for querying immutable and mutable fixed-width segment types
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #8953:
URL: https://github.com/apache/pinot/pull/8953#discussion_r909233038
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/fwd/MultiValueFixedByteRawIndexCreator.java:
##########
@@ -77,7 +77,8 @@ public MultiValueFixedByteRawIndexCreator(File baseIndexDir, ChunkCompressionTyp
boolean deriveNumDocsPerChunk, int writerVersion)
throws IOException {
File file = new File(baseIndexDir, column + Indexes.RAW_MV_FORWARD_INDEX_FILE_EXTENSION);
- int totalMaxLength = maxNumberOfMultiValueElements * valueType.getStoredType().size();
+ // Store the length followed by the values
+ int totalMaxLength = Integer.BYTES + (maxNumberOfMultiValueElements * valueType.getStoredType().size());
Review Comment:
Discussed this offline with @somandal . The MV writer caller wasn't doing the right thing and this change is needed. @somandal , it will be great if you can share the findings here for future reference.
--
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] siddharthteotia merged pull request #8953: Add multi-value noDict APIs and their implementation for querying immutable and mutable fixed-width segment types
Posted by GitBox <gi...@apache.org>.
siddharthteotia merged PR #8953:
URL: https://github.com/apache/pinot/pull/8953
--
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] somandal commented on a diff in pull request #8953: Add multi-value noDict APIs and their implementation for querying immutable and mutable fixed-width segment types
Posted by GitBox <gi...@apache.org>.
somandal commented on code in PR #8953:
URL: https://github.com/apache/pinot/pull/8953#discussion_r909067857
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/reader/ForwardIndexReader.java:
##########
@@ -496,4 +861,26 @@ default int getStringMV(int docId, String[] valueBuffer, T context) {
default int getBytesMV(int docId, byte[][] valueBuffer, T context) {
throw new UnsupportedOperationException();
}
+
+ /**
+ * Reads the bytes type multi-value at the given document id.
+ *
+ * @param docId Document id
+ * @param context Reader context
+ * @return BYTE values at the given document id
+ */
+ default byte[][] getBytesMV(int docId, T context) {
Review Comment:
From my testing it looks like MV Bytes column type isn't supported today and will need more work. I think support for `readBytesValues()` in `DataFetcher` can be added when MV Bytes support is added. What do you think?
--
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 #8953: Add multi-value noDict APIs and their implementation for querying immutable and mutable fixed-width segment types
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #8953:
URL: https://github.com/apache/pinot/pull/8953#discussion_r906222092
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/reader/ForwardIndexReader.java:
##########
@@ -410,9 +421,303 @@ default byte[] getBytes(int docId, T context) {
/**
* MULTI-VALUE COLUMN RAW INDEX APIs
- * TODO: Not supported yet
*/
+ /**
+ * Fills the values
+ * @param docIds Array containing the document ids to read
+ * @param length Number of values to read
+ * @param values Values to fill
+ * @param context Reader context
+ */
+ default void readValuesMV(int[] docIds, int length, int[][] values, T context) {
+ switch (getValueType()) {
+ case INT:
+ for (int i = 0; i < length; i++) {
+ values[i] = getIntMV(docIds[i], context);
+ }
+ break;
+ case LONG:
+ for (int i = 0; i < length; i++) {
+ long[] longValueBuffer = getLongMV(docIds[i], context);
Review Comment:
Essentially we can avoid creating a separate array for each iteration of the for look (similar to the dict ids buffer). Let me explain with code:
```suggestion
long[] longValuesBuffer = new long[maxNumValuesPerMVEntry];
for (int i = 0; i < length; i++) {
int numValues = getLongMV(docIds[i], longValuesBuffer, context);
int[] intValues = new int[numValues];
for (int j = 0; j < numValues; j++) {
intValues[j] = (int) longValuesBuffer[j];
}
}
```
--
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] somandal commented on a diff in pull request #8953: Add multi-value noDict APIs and their implementation for querying immutable and mutable fixed-width segment types
Posted by GitBox <gi...@apache.org>.
somandal commented on code in PR #8953:
URL: https://github.com/apache/pinot/pull/8953#discussion_r906271033
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/reader/ForwardIndexReader.java:
##########
@@ -100,6 +100,17 @@ default int getDictIdMV(int docId, int[] dictIdBuffer, T context) {
throw new UnsupportedOperationException();
}
+ /**
+ * Reads the dictionary ids for a multi-value column at the given document id into a buffer and returns the buffer.
+ *
+ * @param docId Document id
+ * @param context Reader context
+ * @return A buffer containing the multi-value entries
+ */
Review Comment:
done
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/reader/ForwardIndexReader.java:
##########
@@ -410,9 +421,303 @@ default byte[] getBytes(int docId, T context) {
/**
* MULTI-VALUE COLUMN RAW INDEX APIs
- * TODO: Not supported yet
*/
+ /**
+ * Fills the values
+ * @param docIds Array containing the document ids to read
+ * @param length Number of values to read
+ * @param values Values to fill
+ * @param context Reader context
+ */
+ default void readValuesMV(int[] docIds, int length, int[][] values, T context) {
+ switch (getValueType()) {
+ case INT:
+ for (int i = 0; i < length; i++) {
+ values[i] = getIntMV(docIds[i], context);
+ }
+ break;
+ case LONG:
+ for (int i = 0; i < length; i++) {
+ long[] longValueBuffer = getLongMV(docIds[i], context);
Review Comment:
done
--
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