Add MV raw forward index and MV `BYTES` data type #7595

richardstartin · 2021-10-19T10:57:02Z

Description

Co-authored with @kishoreg

Introduces BYTES_ARRAY type
Introduces forward index writers/readers for MV fixed and variable length raw bytes columns
Collects metadata for the largest row in bytes so it can be used for forward index chunk sizing (we choose the largest chunk size of 1MB or the largest row in bytes, so we guarantee a single row fits in a chunk, and that we don't risk estimating enormous chunk sizes based on the other statistics available)
Removes the compression buffer, compress directly into the forward index file.

Upgrade Notes

Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)

Yes (Please label as backward-incompat, and complete the section below on Release Notes)

Does this PR fix a zero-downtime upgrade introduced earlier?

Yes (Please label this as backward-incompat, and complete the section below on Release Notes)

Does this PR otherwise need attention when creating release notes? Things to consider:

New configuration options
Deprecation of configurations
Signature changes to public methods/interfaces
New plugins added or old plugins removed

Yes (Please label this PR as release-notes and complete the section on Release Notes)

Release Notes

Documentation

codecov-commenter · 2021-10-19T11:35:21Z

Codecov Report

Merging #7595 (d8bd2ad) into master (6fef210) will decrease coverage by 40.58%.
The diff coverage is 0.85%.

@@              Coverage Diff              @@
##             master    #7595       +/-   ##
=============================================
- Coverage     71.59%   31.01%   -40.59%     
=============================================
  Files          1559     1553        -6     
  Lines         79025    79022        -3     
  Branches      11702    11710        +8     
=============================================
- Hits          56579    24508    -32071     
- Misses        18639    52417    +33778     
+ Partials       3807     2097     -1710

Flag	Coverage Δ
integration1	`29.44% <0.85%> (-0.06%)`	⬇️
integration2	`27.80% <0.57%> (-0.09%)`	⬇️
unittests1	`?`
unittests2	`?`

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

Impacted Files	Coverage Δ
...ot/segment/local/io/compression/LZ4Compressor.java	`0.00% <0.00%> (-100.00%)`	⬇️
...nt/local/io/compression/PassThroughCompressor.java	`0.00% <0.00%> (-100.00%)`	⬇️
...segment/local/io/compression/SnappyCompressor.java	`0.00% <0.00%> (-100.00%)`	⬇️
...ment/local/io/compression/ZstandardCompressor.java	`0.00% <0.00%> (-100.00%)`	⬇️
.../io/writer/impl/BaseChunkSVForwardIndexWriter.java	`0.00% <0.00%> (-85.72%)`	⬇️
...riter/impl/FixedByteChunkSVForwardIndexWriter.java	`0.00% <ø> (-100.00%)`	⬇️
.../writer/impl/VarByteChunkSVForwardIndexWriter.java	`0.00% <0.00%> (-100.00%)`	⬇️
...ment/creator/impl/SegmentColumnarIndexCreator.java	`0.00% <0.00%> (-86.67%)`	⬇️
...r/impl/fwd/MultiValueFixedByteRawIndexCreator.java	`0.00% <0.00%> (ø)`
...tor/impl/fwd/MultiValueVarByteRawIndexCreator.java	`0.00% <0.00%> (ø)`
... and 1070 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6fef210...d8bd2ad. Read the comment docs.

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

atris · 2021-10-20T08:21:40Z

This PR has a conflict with #7604 -- we need to figure out the sequencing of these two (duplicate commits for the FWD index).

richardstartin · 2021-10-20T09:23:31Z

Can the other PR wait for this and then rebase? The work done here is intended to prevent OOM, and the common commits can’t be merged without the rest of this PR.

kishoreg · 2021-10-20T14:43:52Z

This PR has a conflict with #7604 -- we need to figure out the sequencing of these two (duplicate commits for the FWD index).

Hi @atris. Let's get this one in first and then rebase text index support on top of this.

...in/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java

...c/main/java/org/apache/pinot/segment/local/io/writer/impl/BaseChunkSVForwardIndexWriter.java

pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/ColumnStatistics.java

richardstartin · 2021-10-20T16:32:22Z

I had to force derivation of numDocs for variable length data because there's no good solution to the buffer size problem given the following constraints:

There is a fixed number of documents per chunk
We don't want to OOM if there is a very large row in a segment, and applying an arbitrary multiplier amplifies this risk
Compression is applied at a chunk level, not intrachunk
The compression libraries all require a single buffer

When there is a very large row (> 1MB) we end up with 1 doc per chunk in the segment. The only good solution is to evolve the forward index format to allow variable numbers of docs per chunk for variable length data, but we can do that later if this becomes a problem,

...ache/pinot/segment/local/segment/index/readers/forward/VarByteChunkMVForwardIndexReader.java

richardstartin · 2021-10-20T17:19:13Z

.../apache/pinot/segment/local/segment/creator/impl/fwd/MultiValueFixedByteRawIndexCreator.java

+    byte[] bytes = new byte[Integer.BYTES
+        + values.length * Integer.BYTES]; //numValues, bytes required to store the content
+    ByteBuffer byteBuffer = ByteBuffer.wrap(bytes);
+    //write the length
+    byteBuffer.putInt(values.length);
+    //write the content of each element
+    for (final int value : values) {
+      byteBuffer.putInt(value);
+    }
+    _indexWriter.putBytes(bytes);


This should not require allocation of a temporary buffer, this could just be implemented as an MV pattern on _indexWriter, just as was done to eliminate the much larger buffers for byte[][] and String[]

…ate it

mayankshriv

Minor comments, lgtm otherwise.

pinot-core/src/main/java/org/apache/pinot/core/minion/RawIndexConverter.java

...c/main/java/org/apache/pinot/segment/local/io/writer/impl/BaseChunkSVForwardIndexWriter.java

...ain/java/org/apache/pinot/segment/local/io/writer/impl/VarByteChunkSVForwardIndexWriter.java

Jackie-Jiang

The reader for fixed-length MV is not implemented

pinot-core/src/main/java/org/apache/pinot/core/minion/RawIndexConverter.java

...t-local/src/main/java/org/apache/pinot/segment/local/io/compression/ZstandardCompressor.java

...ain/java/org/apache/pinot/segment/local/io/writer/impl/VarByteChunkSVForwardIndexWriter.java

...c/main/java/org/apache/pinot/segment/local/io/writer/impl/BaseChunkSVForwardIndexWriter.java

Jackie-Jiang · 2021-10-24T02:10:11Z

.../apache/pinot/segment/local/segment/creator/impl/fwd/MultiValueFixedByteRawIndexCreator.java

+      throws IOException {
+    File file = new File(baseIndexDir,
+        column + Indexes.RAW_MV_FORWARD_INDEX_FILE_EXTENSION);
+    FileUtils.deleteQuietly(file);


(nit) unnecessary?

@kishoreg can you explain why you included this?

.../apache/pinot/segment/local/segment/creator/impl/fwd/MultiValueFixedByteRawIndexCreator.java

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

Jackie-Jiang · 2021-10-24T02:18:25Z

...pache/pinot/segment/local/segment/creator/impl/stats/BytesColumnPredIndexStatsCollector.java

+        int length = value.length();
+        _minLength = Math.min(_minLength, length);
+        _maxLength = Math.max(_maxLength, length);
+        rowLength += length;


Should we count the actual encoded bytes length? We need to add (1 + length) integers to this. Same for STRING type

That seems wrong to me, this is the length of the data and it's not known whether it would be length prefixed (+4) or null terminated (+1) here and adding either would prevent the other.

Jackie-Jiang · 2021-10-24T02:19:33Z

...-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/reader/ForwardIndexReader.java

+    throw new UnsupportedOperationException();
+  }
+
+  default int getFloatMV(int docId, float[] valueBuffer, T context, int[] parentIndices) {


Remove this?

Yes I hadn't noticed this from the initial commits @kishoreg made.

I will remove it in a follow up

* Initial code for MultiValue forward Index * Wiring in the segment creation driver Impl * cleanup * finish off adding BYTES_ARRAY type * use less memory and fewer passes during encoding * reduce memory requirement for forwardindexwriter * track size in bytes of largest row so chunks can be sized to accommodate it * remove TODOs * force derivation of number of docs for raw MV columns * specify character encoding * leave changes to integration tests to MV TEXT index implementation * fix javadoc * don't use StringUtils * fix formatting after rebase * fix javadoc formatting again * use zstd's compress bound Co-authored-by: kishoreg <g.kishore@gmail.com>

mcvsubbu reviewed Oct 19, 2021

View reviewed changes

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

richardstartin commented Oct 19, 2021

View reviewed changes

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

richardstartin force-pushed the mv-fwd-index branch 5 times, most recently from ea9a92b to 174f00b Compare October 19, 2021 22:07

richardstartin force-pushed the mv-fwd-index branch 2 times, most recently from d8e37f4 to ae5701e Compare October 20, 2021 13:48

richardstartin marked this pull request as ready for review October 20, 2021 13:53

kishoreg reviewed Oct 20, 2021

View reviewed changes

richardstartin force-pushed the mv-fwd-index branch from e788aa0 to b381ad1 Compare October 20, 2021 15:01

richardstartin commented Oct 20, 2021

View reviewed changes

pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/ColumnStatistics.java Show resolved Hide resolved

richardstartin force-pushed the mv-fwd-index branch from a8b1402 to fbf804c Compare October 20, 2021 16:26

richardstartin force-pushed the mv-fwd-index branch from fbf804c to b80de86 Compare October 20, 2021 16:33

richardstartin commented Oct 20, 2021

View reviewed changes

...ache/pinot/segment/local/segment/index/readers/forward/VarByteChunkMVForwardIndexReader.java Outdated Show resolved Hide resolved

richardstartin force-pushed the mv-fwd-index branch from b80de86 to 2acfec1 Compare October 20, 2021 17:01

richardstartin changed the title ~~MV fwd index + MV BYTES~~ Add MV raw forward index and MV BYTES data type Oct 20, 2021

richardstartin commented Oct 20, 2021

View reviewed changes

richardstartin mentioned this pull request Oct 20, 2021

Allow MV Field Support For Raw Columns in Text Indices #7604

Closed

richardstartin force-pushed the mv-fwd-index branch from 8d02f6a to 497c8ab Compare October 21, 2021 15:19

kishoreg and others added 4 commits October 21, 2021 20:26

Initial code for MultiValue forward Index

9b0a263

Wiring in the segment creation driver Impl

cdc6890

cleanup

7f54457

finish off adding BYTES_ARRAY type

662d266

richardstartin added 9 commits October 21, 2021 20:29

use less memory and fewer passes during encoding

cba16b6

reduce memory requirement for forwardindexwriter

b2d136b

track size in bytes of largest row so chunks can be sized to accommod…

9fdf8aa

…ate it

remove TODOs

e4dc1c8

force derivation of number of docs for raw MV columns

4cbb866

specify character encoding

c154701

leave changes to integration tests to MV TEXT index implementation

e283444

fix javadoc

46670d1

don't use StringUtils

056531a

richardstartin force-pushed the mv-fwd-index branch from 7affdf5 to 056531a Compare October 21, 2021 19:29

fix formatting after rebase

e9ccb61

kishoreg approved these changes Oct 21, 2021

View reviewed changes

mayankshriv approved these changes Oct 21, 2021

View reviewed changes

richardstartin added 2 commits October 22, 2021 10:23

fix javadoc formatting again

69f1c6a

use zstd's compress bound

d8bd2ad

richardstartin force-pushed the mv-fwd-index branch from ff74da5 to d8bd2ad Compare October 22, 2021 10:28

kishoreg merged commit aed1307 into apache:master Oct 22, 2021

Jackie-Jiang reviewed Oct 24, 2021

View reviewed changes

richardstartin mentioned this pull request Oct 25, 2021

implement FixedByteChunkMVForwardIndexReader #7629

Merged

3 tasks

sajjad-moradi mentioned this pull request Dec 18, 2021

Performance problem in segment build #7929

Closed

Jackie-Jiang added the feature label Nov 22, 2022

Jackie-Jiang mentioned this pull request Nov 22, 2022

Support raw index for multi-valued columns #8755

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MV raw forward index and MV `BYTES` data type #7595

Add MV raw forward index and MV `BYTES` data type #7595

richardstartin commented Oct 19, 2021 •

edited

Loading

codecov-commenter commented Oct 19, 2021 •

edited

Loading

atris commented Oct 20, 2021

richardstartin commented Oct 20, 2021

kishoreg commented Oct 20, 2021

richardstartin commented Oct 20, 2021

richardstartin Oct 20, 2021

mayankshriv left a comment

Jackie-Jiang left a comment

Jackie-Jiang Oct 24, 2021

richardstartin Oct 24, 2021

Jackie-Jiang Oct 24, 2021

richardstartin Oct 24, 2021

Jackie-Jiang Oct 24, 2021

richardstartin Oct 24, 2021

richardstartin Oct 24, 2021

Add MV raw forward index and MV BYTES data type #7595

Add MV raw forward index and MV BYTES data type #7595

Conversation

richardstartin commented Oct 19, 2021 • edited Loading

Description

Upgrade Notes

Release Notes

Documentation

codecov-commenter commented Oct 19, 2021 • edited Loading

Codecov Report

atris commented Oct 20, 2021

richardstartin commented Oct 20, 2021

kishoreg commented Oct 20, 2021

richardstartin commented Oct 20, 2021

Choose a reason for hiding this comment

mayankshriv left a comment

Choose a reason for hiding this comment

Jackie-Jiang left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add MV raw forward index and MV `BYTES` data type #7595

Add MV raw forward index and MV `BYTES` data type #7595

richardstartin commented Oct 19, 2021 •

edited

Loading

codecov-commenter commented Oct 19, 2021 •

edited

Loading