Support rewriting forward index upon changing compression type for existing raw MV column #9510

vvivekiyer · 2022-10-02T07:18:33Z

OSS issue: #9348
Label: Feature

In PR #9454, we added support to change Compression Type for raw SV columns during segmentReload. This PR extends the support to raw MV columns.

codecov-commenter · 2022-10-02T08:06:41Z

Codecov Report

Merging #9510 (26777bd) into master (c581fd9) will decrease coverage by 12.53%.
The diff coverage is 0.00%.

@@              Coverage Diff              @@
##             master    #9510       +/-   ##
=============================================
- Coverage     28.10%   15.56%   -12.54%     
- Complexity       53      174      +121     
=============================================
  Files          1922     1883       -39     
  Lines        102937   101236     -1701     
  Branches      15657    15459      -198     
=============================================
- Hits          28926    15755    -13171     
- Misses        71178    84294    +13116     
+ Partials       2833     1187     -1646

Flag	Coverage Δ
integration1	`?`
integration2	`?`
unittests2	`15.56% <0.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...tor/impl/fwd/MultiValueVarByteRawIndexCreator.java	`0.00% <0.00%> (ø)`
...ocal/segment/index/loader/ForwardIndexHandler.java	`0.00% <0.00%> (ø)`
...ocal/segment/readers/PinotSegmentColumnReader.java	`0.00% <0.00%> (ø)`
...inot/segment/spi/creator/IndexCreationContext.java	`0.00% <0.00%> (ø)`
...src/main/java/org/apache/pinot/sql/FilterKind.java	`0.00% <0.00%> (-100.00%)`	⬇️
...ain/java/org/apache/pinot/core/data/table/Key.java	`0.00% <0.00%> (-100.00%)`	⬇️
...n/java/org/apache/pinot/core/data/table/Table.java	`0.00% <0.00%> (-100.00%)`	⬇️
.../java/org/apache/pinot/core/data/table/Record.java	`0.00% <0.00%> (-100.00%)`	⬇️
.../java/org/apache/pinot/core/util/GroupByUtils.java	`0.00% <0.00%> (-100.00%)`	⬇️
.../java/org/apache/pinot/common/auth/AuthConfig.java	`0.00% <0.00%> (-100.00%)`	⬇️
... and 987 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

siddharthteotia · 2022-10-04T02:28:19Z

@somandal to help review this.

somandal · 2022-10-04T02:28:26Z

I’ll review this

somandal · 2022-10-12T04:37:56Z

pom.xml

-        <version>${netty.version}</version>
-        <type>pom</type>
-        <scope>import</scope>
+        <artifactId>netty-buffer</artifactId>


Any specific need for this pom.xml change?

Ah, was a mistake! Removed.

...l/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java

siddharthteotia · 2022-10-12T20:42:18Z

@vvivekiyer - can you rebase please ?

...l/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java

somandal · 2022-10-13T17:00:16Z

...l/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java

+      Preconditions.checkState(lengthOfLongestEntry >= 0,
+          "lengthOfLongestEntry cannot be negative. segment=" + segmentName + " column={}" + column);
+      int maxNumberOfMVEntries = existingColMetadata.getMaxNumberOfMultiValues();
+      int maxRowLengthInBytes = lengthOfLongestEntry - (Integer.BYTES * maxNumberOfMVEntries) - Integer.BYTES;


This code is not at all obvious. Can you explain this part here? e.g. why are you subtracting (Integer.BYTES * maxNumberOfMVEntries)

for MultiValueVarByteRawIndexCreator I see why this will work and that this will actually get passed to the forward index creator and used

int maxLengthPrefixes = Integer.BYTES * maxNumberOfElements; int totalMaxLength = Integer.BYTES + maxRowLengthInBytes + maxLengthPrefixes;

For MultiValueFixedByteRawIndexCreator they calculate it as:

int totalMaxLength = Integer.BYTES + (maxNumberOfMultiValueElements * valueType.getStoredType().size());

The maxRowLengthInBytes isn't actually passed to this forward index creator though and is thus never actually used.

Can you perhaps add a comment about why it's okay to set this up in this way (that it's only used by the variable length types)?

I feel this calculation should not live here because this is dependent on the file format layout / version of the originally created raw forward index.

If the latter changes, this will fail. So I suggest moving this into a static function inside the raw chunk forward index writer that controls the format.

Whoever changes the format should keep the math inside the function backward compatible

Good point. I've added comments explaining the logic. I've also moved the math inside static functions in MultiValueVarByteRawIndexCreator.

IMO, it's still a little confusing because of our existing naming - e.g lengthOfLongestEntry maxRowLengthInBytes, etc. Cleaning these names everywhere is a larger undertaking and better left as a future cleanup. Thoughts?

somandal · 2022-10-13T17:11:24Z

...l/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java

+        PinotSegmentColumnReader columnReader = new PinotSegmentColumnReader(reader, null, null, maxNumberOfMVEntries);
+
+        for (int i = 0; i < numDocs; i++) {
+          Object[] values = (Object[]) columnReader.getValue(i);


wondering if it's a better idea to expose the getIntMV, etc functions in the columnReader or directly use the ForwardIndexReader instead of doing it this way. In your code, you're doing double copies, first to construct the Object[] values and then to construct the actual value type array.

The ForwardIndexReader directly provides APIs to get the array in the desired type:

default int getIntMV(int docId, int[] valueBuffer, T context) { throw new UnsupportedOperationException(); } default int[] getIntMV(int docId, T context) { throw new UnsupportedOperationException(); }

Good point. Did this and also refactored the code quite a bit.

...l/src/main/java/org/apache/pinot/segment/local/segment/readers/PinotSegmentColumnReader.java

somandal · 2022-10-13T17:15:52Z

...l/src/main/java/org/apache/pinot/segment/local/segment/readers/PinotSegmentColumnReader.java

+      } else {
+        Preconditions.checkState(_maxNumValuesPerMVEntry >= 0, "maxNumValuesPerMVEntry is negative for an MV column.");
+
+        switch (_forwardIndexReader.getStoredType()) {


Thanks for adding this. I do see value in keeping this code even if you decide to either add more efficient functions to directly fetch the type array or if you decide to directly use the ForwardIndexReader.

...c/test/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandlerTest.java

...l/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java

...rg/apache/pinot/segment/local/segment/creator/impl/fwd/MultiValueVarByteRawIndexCreator.java

...l/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java

siddharthteotia · 2022-10-14T23:37:17Z

Tagging @Jackie-Jiang in case he wants to take a look. This is a follow-up to #9454

Jackie-Jiang · 2022-10-17T23:19:54Z

I don't have concern merging it given the added tests

vvivekiyer force-pushed the fwdIndexHandlerMV branch from 9512ae4 to b577d85 Compare October 2, 2022 07:29

siddharthteotia added the feature label Oct 4, 2022

Make StreamMessage generic

107ef80

vvivekiyer force-pushed the fwdIndexHandlerMV branch from b577d85 to 629dde1 Compare October 11, 2022 08:34

vvivekiyer marked this pull request as ready for review October 11, 2022 08:35

vvivekiyer force-pushed the fwdIndexHandlerMV branch from 629dde1 to b01b7db Compare October 11, 2022 08:41

somandal reviewed Oct 12, 2022

View reviewed changes

vvivekiyer force-pushed the fwdIndexHandlerMV branch from b01b7db to fb09698 Compare October 12, 2022 04:41

vvivekiyer commented Oct 12, 2022

View reviewed changes

...l/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java Outdated Show resolved Hide resolved

Merge branch 'apache:master' into master

f8c03df

vvivekiyer force-pushed the fwdIndexHandlerMV branch from fb09698 to 1b374e8 Compare October 12, 2022 23:45

Allow changing compressionType for MV columns during segmentReload

a0e7a34

vvivekiyer force-pushed the fwdIndexHandlerMV branch from 1b374e8 to a0e7a34 Compare October 12, 2022 23:52

somandal reviewed Oct 13, 2022

View reviewed changes

siddharthteotia reviewed Oct 14, 2022

View reviewed changes

...l/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java Outdated Show resolved Hide resolved

Address review comments

e763bb6

vvivekiyer requested review from siddharthteotia and somandal and removed request for siddharthteotia October 14, 2022 09:46

somandal reviewed Oct 14, 2022

View reviewed changes

...l/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java Outdated Show resolved Hide resolved

somandal reviewed Oct 14, 2022

View reviewed changes

...rg/apache/pinot/segment/local/segment/creator/impl/fwd/MultiValueVarByteRawIndexCreator.java Outdated Show resolved Hide resolved

...rg/apache/pinot/segment/local/segment/creator/impl/fwd/MultiValueVarByteRawIndexCreator.java Outdated Show resolved Hide resolved

Refactor and cleanup code

91a6dcf

vvivekiyer force-pushed the fwdIndexHandlerMV branch from 45a89db to 91a6dcf Compare October 14, 2022 20:21

vvivekiyer requested a review from somandal October 14, 2022 20:22

somandal approved these changes Oct 14, 2022

View reviewed changes

siddharthteotia reviewed Oct 14, 2022

View reviewed changes

...l/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java Outdated Show resolved Hide resolved

Optimize for loop in ForwardIndexHandler

26777bd

siddharthteotia changed the title ~~Allow changing compressionType for raw MV columns during segmentReload~~ Support rewriting forward index upon changing compression type for existing raw MV column Oct 17, 2022

siddharthteotia approved these changes Oct 17, 2022

View reviewed changes

siddharthteotia merged commit ae93ce7 into apache:master Oct 17, 2022

vvivekiyer deleted the fwdIndexHandlerMV branch October 17, 2022 23:26

siddharthteotia mentioned this pull request Oct 17, 2022

[Feature] Change forward index and dictionary properties during segment reload #9348

Closed

vvivekiyer mentioned this pull request Nov 9, 2022

SegmentReload: Add query tests for changeCompression and enableDictionary #9768

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support rewriting forward index upon changing compression type for existing raw MV column #9510

Support rewriting forward index upon changing compression type for existing raw MV column #9510

vvivekiyer commented Oct 2, 2022

codecov-commenter commented Oct 2, 2022 •

edited

Loading

siddharthteotia commented Oct 4, 2022

somandal commented Oct 4, 2022

somandal Oct 12, 2022

vvivekiyer Oct 12, 2022

siddharthteotia commented Oct 12, 2022

somandal Oct 13, 2022

siddharthteotia Oct 14, 2022

vvivekiyer Oct 14, 2022

somandal Oct 13, 2022

vvivekiyer Oct 14, 2022

somandal Oct 13, 2022

siddharthteotia commented Oct 14, 2022

Jackie-Jiang commented Oct 17, 2022

Support rewriting forward index upon changing compression type for existing raw MV column #9510

Support rewriting forward index upon changing compression type for existing raw MV column #9510

Conversation

vvivekiyer commented Oct 2, 2022

codecov-commenter commented Oct 2, 2022 • edited Loading

Codecov Report

siddharthteotia commented Oct 4, 2022

somandal commented Oct 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddharthteotia commented Oct 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddharthteotia commented Oct 14, 2022

Jackie-Jiang commented Oct 17, 2022

codecov-commenter commented Oct 2, 2022 •

edited

Loading