CLP as a compressionCodec #12504

saurabhd336 · 2024-02-27T09:49:51Z

Allows using CLP as a compression type for string columns. This hides all internal details of clp encoding, without having to expose the individual parts of a clp encoded log line externally. Giving a clean forwardIndex.getString() interface for the operators.

Enable using

"fieldConfigList": [
      {
        "name": "logLine",
        "encodingType": "RAW",
        "compressionCodec": "CLP",
      }

* Refactoring the upsert compaction related code 1. Fix the issue with fetching validDocId metadata for table with a large number of segments. (Added POST API with list of segments to be part of the request body) 2. Added POST support for MultiHttpRequest to cover 1. 3. Added GET /tables/<tableName>/validDocIdMetadata API on the controller for improving debuggability. * Addressing comments

codecov-commenter · 2024-02-27T17:32:34Z

Codecov Report

Attention: Patch coverage is 77.63158% with 85 lines in your changes are missing coverage. Please review.

Project coverage is 61.34%. Comparing base (59551e4) to head (78532fe).
Report is 150 commits behind head on master.

Files	Patch %	Lines
...index/readers/forward/CLPForwardIndexReaderV1.java	63.04%	34 Missing ⚠️
...ent/creator/impl/fwd/CLPForwardIndexCreatorV1.java	90.65%	8 Missing and 2 partials ⚠️
.../realtime/impl/forward/CLPMutableForwardIndex.java	84.74%	9 Missing ⚠️
...impl/stats/StringColumnPreIndexStatsCollector.java	83.67%	5 Missing and 3 partials ⚠️
...ot/segment/local/io/util/VarLengthValueReader.java	0.00%	7 Missing ⚠️
...verter/stats/MutableNoDictionaryColStatistics.java	0.00%	4 Missing ⚠️
...he/pinot/segment/local/utils/TableConfigUtils.java	0.00%	2 Missing and 2 partials ⚠️
...gment/index/forward/ForwardIndexReaderFactory.java	40.00%	1 Missing and 2 partials ⚠️
...ment/index/forward/ForwardIndexCreatorFactory.java	0.00%	1 Missing and 1 partial ⚠️
.../local/segment/index/forward/ForwardIndexType.java	0.00%	1 Missing and 1 partial ⚠️
... and 2 more

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #12504      +/-   ##
============================================
- Coverage     61.75%   61.34%   -0.41%     
+ Complexity      207      198       -9     
============================================
  Files          2436     2456      +20     
  Lines        133233   134240    +1007     
  Branches      20636    20762     +126     
============================================
+ Hits          82274    82356      +82     
- Misses        44911    45705     +794     
- Partials       6048     6179     +131

Flag	Coverage Δ
custom-integration1	`<0.01% <0.00%> (-0.01%)`	⬇️
integration	`<0.01% <0.00%> (-0.01%)`	⬇️
integration1	`?`
integration2	`0.00% <0.00%> (ø)`
java-11	`27.88% <73.94%> (-33.83%)`	⬇️
java-21	`61.34% <77.63%> (-0.29%)`	⬇️
skip-bytebuffers-false	`27.87% <73.94%> (-33.88%)`	⬇️
skip-bytebuffers-true	`61.34% <77.63%> (+33.61%)`	⬆️
temurin	`61.34% <77.63%> (-0.41%)`	⬇️
unittests	`61.34% <77.63%> (-0.41%)`	⬇️
unittests1	`45.97% <3.68%> (-0.92%)`	⬇️
unittests2	`27.88% <73.94%> (+0.15%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

saurabhd336 · 2024-03-12T09:43:58Z

cc: @kirkrodrigues @jackluo923 @npawar

kirkrodrigues

This is neat! I'm no Pinot expert, so I can't comment on the fine-grained validity of the code, but it roughly makes sense to me. I added a few minor suggestions. I would also suggest renaming any new classes as ClpXXX rather than CLPXXX, so that the classes play better with serialization/deserialization libraries (if ever necessary).

Perhaps this is a silly question, but how can we support CLP's query logic (like clpMatch) on top of this?

kirkrodrigues · 2024-03-14T04:17:36Z

...st/java/org/apache/pinot/segment/local/segment/index/creator/CLPForwardIndexCreatorTest.java

+    logLines.add(
+        "2023/10/27 16:35:10.470 INFO [ControllerResponseFilter] [grizzly-http-server-2] Handled request from 0.0"
+            + ".0.0 GET https://0.0.0.0:8443/health?checkType=liveness, content-type null status code 200 OK");
+    logLines.add(


Can we include a test for a null row?

kirkrodrigues · 2024-03-14T04:19:30Z

pinot-spi/src/main/java/org/apache/pinot/spi/config/table/FieldConfig.java

+    // CLP is a special type of compression codec that isn't generally applicable to all RAW columns and has a
+    // special handling (see {@link CLPForwardIndexCreatorV1})


"a special handling"

It sounds like this is an incomplete sentence? Did you mean to say "special handling for log events" or something like that?

kirkrodrigues · 2024-03-14T04:21:28Z

...in/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexCreatorFactory.java

@@ -69,6 +71,10 @@ public static ForwardIndexCreator createIndexCreator(IndexCreationContext contex
    } else {
      // Dictionary disabled columns
      DataType storedType = fieldSpec.getDataType().getStoredType();
+      if (indexConfig.getCompressionCodec() == FieldConfig.CompressionCodec.CLP) {
+        // CLP compression codec


This comment seems a little redundant with the if condition.

kirkrodrigues · 2024-03-14T04:22:35Z

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

+    if (_sealed) {
+      return _clpStatsCollector.getCLPStats();
+    }
+    throw new IllegalStateException("you must seal the collector first before asking for clp stats");


Suggested change

throw new IllegalStateException("you must seal the collector first before asking for clp stats");

throw new IllegalStateException("The collector must be sealed before calling getCLPStats");

kirkrodrigues · 2024-03-14T04:23:28Z

...n/java/org/apache/pinot/segment/local/segment/creator/impl/fwd/CLPForwardIndexCreatorV1.java

+  @Override
+  public void close()
+      throws IOException {
+    // Delete all temp file


Suggested change

// Delete all temp file

// Delete all temp files

Jackie-Jiang · 2024-03-21T05:23:20Z

...sts/src/test/java/org/apache/pinot/integration/tests/CLPEncodingRealtimeIntegrationTest.java

+import org.testng.annotations.Test;
+
+
+public class CLPEncodingRealtimeIntegrationTest extends BaseClusterIntegrationTestSet {


I feel an integration test is a little bit overkilling here since all we want to verify is reading from the forward index. Suggest removing this one and using unit test to cover it

Jackie-Jiang · 2024-03-21T05:29:41Z

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

@@ -58,6 +58,7 @@ public ForwardIndexConfig(@Nullable Boolean disabled, @Nullable CompressionCodec
    if (compressionCodec != null) {
      switch (compressionCodec) {
        case PASS_THROUGH:
+        case CLP:


This seems like a hack. Should we count CLP as a raw value compression? We don't use dictionary encoding on the values

Saurabh Dubey and others added 24 commits November 29, 2023 16:04

Enricher interface

0e31639

Support in realtime ingestion

c9fd201

Merge branch 'master' of github.com:apache/pinot into recordEnricher

0c7f840

Fix offline segment builder

d2b2328

Add enricher to mapper

262605a

Add columnsToExtract config

fdcdbb7

Merge branch 'master' of github.com:apache/pinot into recordEnricher

371877e

Complete enricher pipeline usage

b4e2083

Add CLP as a compression type

f57eb3d

Finish the flow to add CLP index

c408cde

Add reader

9797791

Complete the reader

c80f6e0

Lint fix

792c5a5

Avoid full file copy

abb3f64

Add mutable CLP index

acffbb7

Fix NPe

ac69d83

Merge branch 'master' of github.com:apache/pinot into logDataType

41a758d

Fix for null values

9f3a4d6

Fix class name

71457e9

Merge branch 'master' of github.com:apache/pinot into logDataType

345b57c

Add context handlint

86823d3

Enrichment configs

b87a5b1

Merge branch 'master' of github.com:apache/pinot into logDataType

94b404b

saurabhd336 changed the title ~~Log data type~~ CLP as a compressionCodec Feb 27, 2024

saurabhd336 force-pushed the logDataType branch from 0251131 to fea9fdb Compare February 27, 2024 09:50

Merge branch 'master' of github.com:apache/pinot into logDataType

835559e

saurabhd336 force-pushed the logDataType branch from fea9fdb to 835559e Compare February 27, 2024 16:52

Jackie-Jiang added the feature label Feb 27, 2024

Saurabh Dubey and others added 4 commits March 11, 2024 14:31

Merge branch 'master' of github.com:apache/pinot into logDataType

9683def

Add mutable index tests

7c16c92

Lint

8ca1650

Merge branch 'master' of github.com:apache/pinot into logDataType

0ad8a6d

Saurabh Dubey and others added 5 commits March 12, 2024 15:56

Fix tests

965f7c2

Remove chunkCompressionType

e474bb2

Add validations

951df7b

Add ITs

fcfac6b

Lint + misc

c81fa0d

saurabhd336 force-pushed the logDataType branch from 6b0be01 to c81fa0d Compare March 13, 2024 07:40

Lint

013ebc1

kirkrodrigues reviewed Mar 14, 2024

View reviewed changes

Saurabh Dubey added 2 commits March 19, 2024 13:46

Fix context close

342ce5c

Add recordByteRanges support

e2e5180

saurabhd336 requested a review from Jackie-Jiang March 20, 2024 10:26

Jackie-Jiang added documentation release-notes Referenced by PRs that need attention when compiling the next release notes Configuration Config changes (addition/deletion/change in behavior) labels Mar 21, 2024

Jackie-Jiang approved these changes Mar 21, 2024

View reviewed changes

Saurabh Dubey added 2 commits March 21, 2024 14:43

Review comments

78532fe

Merge branch 'master' of github.com:apache/pinot into logDataType

0dd5e72

saurabhd336 merged commit 02d1c12 into apache:master Mar 21, 2024
19 of 20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLP as a compressionCodec #12504

CLP as a compressionCodec #12504

saurabhd336 commented Feb 27, 2024

codecov-commenter commented Feb 27, 2024 •

edited

Loading

saurabhd336 commented Mar 12, 2024

kirkrodrigues left a comment

kirkrodrigues Mar 14, 2024

saurabhd336 Mar 21, 2024

kirkrodrigues Mar 14, 2024

saurabhd336 Mar 21, 2024

kirkrodrigues Mar 14, 2024

saurabhd336 Mar 21, 2024

kirkrodrigues Mar 14, 2024

saurabhd336 Mar 21, 2024

kirkrodrigues Mar 14, 2024

saurabhd336 Mar 21, 2024

Jackie-Jiang Mar 21, 2024

Jackie-Jiang Mar 21, 2024

		// CLP is a special type of compression codec that isn't generally applicable to all RAW columns and has a
		// special handling (see {@link CLPForwardIndexCreatorV1})

	throw new IllegalStateException("you must seal the collector first before asking for clp stats");
	throw new IllegalStateException("The collector must be sealed before calling getCLPStats");

		import org.testng.annotations.Test;


		public class CLPEncodingRealtimeIntegrationTest extends BaseClusterIntegrationTestSet {

CLP as a compressionCodec #12504

CLP as a compressionCodec #12504

Conversation

saurabhd336 commented Feb 27, 2024

codecov-commenter commented Feb 27, 2024 • edited Loading

Codecov Report

saurabhd336 commented Mar 12, 2024

kirkrodrigues 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

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

codecov-commenter commented Feb 27, 2024 •

edited

Loading