#4317 Feature/variable length bytes offline dictionary for indexing bytes and string dicts. #4321

buchireddy · 2019-06-14T22:12:45Z

Implementation of the feature mentioned in #4317

…dex.

variable length byte dictionary. In the next iteration, we'll be deciding dynamically whether to use fixed or variable length impl.

…rfTest.java

Also fixed other review comments from Kishore.

for a column configurable through TableConfig.

- IndexLoadingConfig - RealtimeSegmentConfig

causing a race condition and unit test failure.

kishoreg

LGTM overall. Minor comments

pinot-core/src/main/java/org/apache/pinot/core/io/util/VarLengthBytesValueReaderWriter.java

...-core/src/main/java/org/apache/pinot/core/segment/creator/impl/SegmentDictionaryCreator.java

pinot-common/src/main/java/org/apache/pinot/common/config/IndexingConfig.java

.../src/main/java/org/apache/pinot/core/data/manager/realtime/HLRealtimeSegmentDataManager.java

.../src/main/java/org/apache/pinot/core/data/manager/realtime/LLRealtimeSegmentDataManager.java

pinot-core/src/main/java/org/apache/pinot/core/io/util/VarLengthBytesValueReaderWriter.java

...-core/src/main/java/org/apache/pinot/core/segment/creator/impl/SegmentDictionaryCreator.java

.../java/org/apache/pinot/core/segment/index/loader/defaultcolumn/BaseDefaultColumnHandler.java

...ore/src/main/java/org/apache/pinot/core/segment/index/readers/ImmutableDictionaryReader.java

mcvsubbu

What is wrong with a template implementation like this #4348
instead of duplicate logic?
IDE will re-factor out Buffer, rename, etc. to our heart's content.

mcvsubbu · 2019-06-21T16:01:18Z

@buchireddy , @kishoreg please provide your comments on #4348. I need a ship-it from one of you

I have not addressed most of the comments by @mayankshriv which will be in the implementation of VarLengthBytesValueReaderWriter. Basically, my proposal is to build mutable and immutable versions of OffHeapByteArrayStore. Either we can derive two classes out of this and use them, or we can play around with the ctor of OffHeapByteArraystore to make it independent of memory manager etc. (caller allocates) and provide only a pnot-databuffer that is mutable (vs the immutable, which could then becoime an empty constructor with an init call). We can work around these things. All I want is one way to encode strings in a compact fashion. thanks.

Jackie-Jiang

Can you please re-format the files with Pinot format introduced in #3705? Also, for member variables, we prefix the value with '_' and do not use 'this.' when accessing them.

pinot-core/src/main/java/org/apache/pinot/core/io/util/VarLengthBytesValueReaderWriter.java

buchireddy · 2019-07-01T06:39:00Z

FTR, I was waiting to address review comments on this PR until now because @mcvsubbu was working on pulling out Buffer as a common class (PR #4348) which can be reused in this PR as well. I personally think that's a nice-to-have change and will help to reuse the byte array store code for both mutable and immutable dictionaries.
However, @Jackie-Jiang is working on refactoring/cleaning the mutable byte array store related code so #4348 isn't going to be merged soon. Also, @Jackie-Jiang and I discussed that the byte array store used for VariableLength dictionary is different enough from the regular mutable store that it's okay for VarLength dictionary to have it's own immutable bytes store. We can definitely find better optimizations for immutable byte store that may not be applicable for mutable store.
Hence, I am now going to work on addressing the review comments on this PR and will get it ready to be merged. @mcvsubbu @Jackie-Jiang Please reply if you guys think otherwise.
cc @kishoreg

kishoreg · 2019-07-02T21:26:00Z

@buchireddy Have you addressed all the comments? @Jackie-Jiang, can you take another look and approve.

buchireddy · 2019-07-02T22:50:46Z

@kishoreg @Jackie-Jiang @mayankshriv I've addressed the review comments and pushed the latest code. Please take a look and LMK if you have any more comments.

since we don't expect it to be ever called.

we can't really reuse it so removing the ThreadLocal byte[]. Thanks to the unit test which caught this issue 👍

codecov-io · 2019-07-03T07:05:02Z

Codecov Report

Merging #4321 into master will increase coverage by 9.12%.
The diff coverage is 71.42%.

@@             Coverage Diff              @@
##             master    #4321      +/-   ##
============================================
+ Coverage     56.23%   65.35%   +9.12%     
  Complexity       20       20              
============================================
  Files          1061     1067       +6     
  Lines         54980    55526     +546     
  Branches       7824     7906      +82     
============================================
+ Hits          30916    36289    +5373     
+ Misses        21677    16665    -5012     
- Partials       2387     2572     +185

Impacted Files	Coverage Δ	Complexity Δ
...ot/core/segment/store/SegmentLocalFSDirectory.java	`65.73% <0%> (+1.68%)`	`0 <0> (ø)`	⬇️
.../core/segment/creator/ColumnIndexCreationInfo.java	`93.54% <100%> (+7.34%)`	`0 <0> (ø)`	⬇️
...manager/realtime/HLRealtimeSegmentDataManager.java	`81.21% <100%> (+81.21%)`	`0 <0> (ø)`	⬇️
...ment/creator/impl/SegmentColumnarIndexCreator.java	`87.5% <100%> (+0.8%)`	`0 <0> (ø)`	⬇️
...t/creator/impl/SegmentIndexCreationDriverImpl.java	`89.71% <100%> (+1.74%)`	`0 <0> (ø)`	⬇️
...loader/defaultcolumn/BaseDefaultColumnHandler.java	`91.89% <100%> (+6.75%)`	`0 <0> (ø)`	⬇️
...manager/realtime/LLRealtimeSegmentDataManager.java	`71.65% <100%> (+28.13%)`	`0 <0> (ø)`	⬇️
...gment/index/readers/ImmutableDictionaryReader.java	`90.47% <33.33%> (-3.79%)`	`0 <0> (ø)`
...org/apache/pinot/common/config/IndexingConfig.java	`53.57% <50%> (+0.3%)`	`0 <0> (ø)`	⬇️
...indexsegment/generator/SegmentGeneratorConfig.java	`61.96% <57.14%> (+9.95%)`	`0 <0> (ø)`	⬇️
... and 394 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 2e543e9...ea66a11. Read the comment docs.

kishoreg

LGTM. @mayankshriv, please review.

pinot-core/src/main/java/org/apache/pinot/core/realtime/converter/RealtimeSegmentConverter.java

so that we don't need to make any assumptions about where the data starts.

mayankshriv

LGTM

buchireddy and others added 13 commits June 14, 2019 15:04

Adding a variable length bytes value reader for offline dictionary in…

f406cfd

…dex.

Make variables private.

c88eb0b

Cleaned up the changes a bit and removed the hacks to use the

1d14aba

variable length byte dictionary. In the next iteration, we'll be deciding dynamically whether to use fixed or variable length impl.

Minor cleanup.

58c1a4a

Adding stats about the string values length in the StringDictionaryPe…

d7994d3

…rfTest.java

Improved the magic header for VariableLength dictionary.

384de88

Also fixed other review comments from Kishore.

Making the feature of whether to use variable length dictionary

d26c6c9

for a column configurable through TableConfig.

Use the variable length dictionary config in realtime segments path too.

9bf6411

Fixed a bug that was caught by the unit tests.

bdeb491

Adding variable length dictionary config in more places like:

c44a09c

- IndexLoadingConfig - RealtimeSegmentConfig

Use separate file names for different tests in a single class. This was

4ef1f66

causing a race condition and unit test failure.

Dont set null value for varLengthDictionaryColumns

929f377

Fixed review comments.

c617439

This was referenced Jun 14, 2019

#4317 Feature/variable length bytes offline dictionary #4318

Closed

Support variable length Offline Dictionary Indexes for bytes, strings and maps to save on storage #4317

Closed

kishoreg requested changes Jun 19, 2019

View reviewed changes

mayankshriv reviewed Jun 20, 2019

View reviewed changes

mcvsubbu reviewed Jun 20, 2019

View reviewed changes

Jackie-Jiang reviewed Jun 28, 2019

View reviewed changes

pinot-core/src/main/java/org/apache/pinot/core/io/util/VarLengthBytesValueReaderWriter.java Outdated Show resolved Hide resolved

Fixed review comments and formatted the code as per Pinot code style.

d8d71c6

Variable length dictionary shouldn't support getPaddedString()

d61dab3

since we don't expect it to be ever called.

buchireddy mentioned this pull request Jul 3, 2019

Cleanup FixedByteValueReaderWriter and ValueReader buffer usage #4393

Open

The way byte[] returned from ImmutableDictionaryReader is reused,

16784dc

we can't really reuse it so removing the ThreadLocal byte[]. Thanks to the unit test which caught this issue 👍

kishoreg approved these changes Jul 3, 2019

View reviewed changes

pinot-core/src/main/java/org/apache/pinot/core/realtime/converter/RealtimeSegmentConverter.java Show resolved Hide resolved

buchireddy mentioned this pull request Jul 5, 2019

Avoid creating garbage in variable length bytes store for offline dictionary #4401

Closed

Explicitly persist the data section offset in the header of the store

ea66a11

so that we don't need to make any assumptions about where the data starts.

mayankshriv approved these changes Jul 6, 2019

View reviewed changes

mayankshriv merged commit 49d8fa7 into apache:master Jul 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#4317 Feature/variable length bytes offline dictionary for indexing bytes and string dicts. #4321

#4317 Feature/variable length bytes offline dictionary for indexing bytes and string dicts. #4321

buchireddy commented Jun 14, 2019 •

edited

kishoreg left a comment

mcvsubbu left a comment •

edited

mcvsubbu commented Jun 21, 2019

Jackie-Jiang left a comment

buchireddy commented Jul 1, 2019

kishoreg commented Jul 2, 2019

buchireddy commented Jul 2, 2019

codecov-io commented Jul 3, 2019 •

edited

kishoreg left a comment

mayankshriv left a comment

#4317 Feature/variable length bytes offline dictionary for indexing bytes and string dicts. #4321

#4317 Feature/variable length bytes offline dictionary for indexing bytes and string dicts. #4321

Conversation

buchireddy commented Jun 14, 2019 • edited

kishoreg left a comment

Choose a reason for hiding this comment

mcvsubbu left a comment • edited

Choose a reason for hiding this comment

mcvsubbu commented Jun 21, 2019

Jackie-Jiang left a comment

Choose a reason for hiding this comment

buchireddy commented Jul 1, 2019

kishoreg commented Jul 2, 2019

buchireddy commented Jul 2, 2019

codecov-io commented Jul 3, 2019 • edited

Codecov Report

kishoreg left a comment

Choose a reason for hiding this comment

mayankshriv left a comment

Choose a reason for hiding this comment

buchireddy commented Jun 14, 2019 •

edited

mcvsubbu left a comment •

edited

codecov-io commented Jul 3, 2019 •

edited