Add config to skip record ingestion on string column length exceeding configured max schema length #13103

tibrewalpratik17 · 2024-05-07T13:40:35Z

label:
ingestion
feature
release-notes

At present, SanitizationTransformer just trims the string length based on configured schema max length. The process is very silent with no observability if the field got trimmed or an option to skip rather than trimming. This becomes critical if the field even if string is treated for json-indexing as post-trimming it can lead to trimmed json / malformed json eventually stopping ingestion.
Here, we add a new table-schema config in FieldSpec maxLengthExceedStrategy. Default value is to TRIM_LENGTH to ensure backward compatibility. If configured to be ERROR, in case of incoming-string-value exceeding the configured max length, it will skip the record, return error and log accordingly. If set as SUBSTITUTE_DEFAULT_VALUE, it will substitute the configured default value in place of the huge string. There is another strategy as NO_ACTION which justs ingests the string.
We have extended similar functionality to JSON and BYTES column type with default being NO_ACTION to preserve backward incompatibility.

Sample FieldSpec:

{
        "name": "Attr",
        "dataType": "STRING",
        "maxLength": 10000,
        "maxLengthExceedStrategy": "FAIL_INGESTION"
}

codecov-commenter · 2024-05-07T14:18:10Z

Codecov Report

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

Project coverage is 62.14%. Comparing base (59551e4) to head (e2a63f8).
Report is 523 commits behind head on master.

Files	Patch %	Lines
...cal/recordtransformer/SanitizationTransformer.java	67.94%	21 Missing and 4 partials ⚠️
...gment/local/segment/creator/TransformPipeline.java	16.66%	4 Missing and 1 partial ⚠️
.../org/apache/pinot/spi/data/DimensionFieldSpec.java	0.00%	2 Missing ⚠️
...a/manager/realtime/RealtimeSegmentDataManager.java	75.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #13103      +/-   ##
============================================
+ Coverage     61.75%   62.14%   +0.39%     
+ Complexity      207      198       -9     
============================================
  Files          2436     2536     +100     
  Lines        133233   139488    +6255     
  Branches      20636    21566     +930     
============================================
+ Hits          82274    86681    +4407     
- Misses        44911    46311    +1400     
- Partials       6048     6496     +448

Flag	Coverage Δ
custom-integration1	`<0.01% <0.00%> (-0.01%)`	⬇️
integration	`<0.01% <0.00%> (-0.01%)`	⬇️
integration1	`<0.01% <0.00%> (-0.01%)`	⬇️
integration2	`0.00% <0.00%> (ø)`
java-11	`62.11% <67.64%> (+0.40%)`	⬆️
java-21	`62.02% <67.64%> (+0.40%)`	⬆️
skip-bytebuffers-false	`62.13% <67.64%> (+0.38%)`	⬆️
skip-bytebuffers-true	`62.00% <67.64%> (+34.27%)`	⬆️
temurin	`62.14% <67.64%> (+0.39%)`	⬆️
unittests	`62.13% <67.64%> (+0.39%)`	⬆️
unittests1	`46.68% <14.70%> (-0.21%)`	⬇️
unittests2	`27.75% <52.94%> (+0.02%)`	⬆️

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.

Jackie-Jiang

@snleee @swaminathanmanish Can you help also review this?

.../src/main/java/org/apache/pinot/segment/local/recordtransformer/SanitizationTransformer.java

satishd

Thanks @tibrewalpratik17 for the PR, overall LGTM. Left a minor comment.

...al/src/test/java/org/apache/pinot/segment/local/recordtransformer/RecordTransformerTest.java

satishd

Thanks @tibrewalpratik17 for addressing the review comments. LGTM.

snleee

LGTM otherwise

.../src/main/java/org/apache/pinot/segment/local/recordtransformer/SanitizationTransformer.java

Jackie-Jiang

I'd suggest extending this to make it applicable to BYTES and JSON as well

pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpec.java

tibrewalpratik17 · 2024-05-22T05:49:37Z

I'd suggest extending this to make it applicable to BYTES and JSON as well

I see #13181 already covering JSON scenario.

… schema max length

Jackie-Jiang

Overall direction looks good!
If we allow maxLengthExceedStrategy to be null, a lot of test changes can be simplified

pinot-common/src/test/java/org/apache/pinot/common/data/FieldSpecTest.java

pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpec.java

tibrewalpratik17 · 2024-05-24T06:19:23Z

If we allow maxLengthExceedStrategy to be null, a lot of test changes can be simplified

Yes this simplifies the PR a lot! Updated accordingly! Thanks.

Jackie-Jiang

Mostly good. Let's also add support for BYTES and JSON since we have all the required strategy supported

pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpec.java

.../src/main/java/org/apache/pinot/segment/local/recordtransformer/SanitizationTransformer.java

Jackie-Jiang · 2024-05-30T20:31:00Z

.../src/main/java/org/apache/pinot/segment/local/recordtransformer/SanitizationTransformer.java

+          case BYTES:
+            maxLengthExceedStrategy = fieldSpec.getMaxLengthExceedStrategy() == null
+                ? FieldSpec.MaxLengthExceedStrategy.NO_ACTION : fieldSpec.getMaxLengthExceedStrategy();
+            _columnToColumnInfoMap.put(fieldSpec.getName(), new SanitizedColumnInfo(fieldSpec.getName(),


We don't need to include the column when it is NO_ACTION

In case of string do we want to ingest NULL_CHARACTER in case of NO_ACTION ?

.../src/main/java/org/apache/pinot/segment/local/recordtransformer/SanitizationTransformer.java

tibrewalpratik17 · 2024-06-02T18:40:25Z

hey @Jackie-Jiang addressed comments! Can you please help with the review?

… configured max schema length (apache#13103)

tibrewalpratik17 force-pushed the fail_on_sanitization branch from 0375dab to 17f2224 Compare May 7, 2024 16:51

Jackie-Jiang added feature ingestion labels May 9, 2024

Jackie-Jiang reviewed May 9, 2024

View reviewed changes

.../src/main/java/org/apache/pinot/segment/local/recordtransformer/SanitizationTransformer.java Outdated Show resolved Hide resolved

satishd reviewed May 13, 2024

View reviewed changes

...al/src/test/java/org/apache/pinot/segment/local/recordtransformer/RecordTransformerTest.java Show resolved Hide resolved

...al/src/test/java/org/apache/pinot/segment/local/recordtransformer/RecordTransformerTest.java Show resolved Hide resolved

satishd approved these changes May 14, 2024

View reviewed changes

tibrewalpratik17 requested a review from Jackie-Jiang May 14, 2024 05:48

tibrewalpratik17 force-pushed the fail_on_sanitization branch 5 times, most recently from 00b6917 to 016d6ee Compare May 20, 2024 08:29

deepthi912 mentioned this pull request May 21, 2024

#13123 MaxLength limiting included for Json Columns #13181

Closed

snleee approved these changes May 21, 2024

View reviewed changes

.../src/main/java/org/apache/pinot/segment/local/recordtransformer/SanitizationTransformer.java Show resolved Hide resolved

.../src/main/java/org/apache/pinot/segment/local/recordtransformer/SanitizationTransformer.java Outdated Show resolved Hide resolved

Jackie-Jiang mentioned this pull request May 22, 2024

Update metadata.properties to have schema max-length #13187

Merged

Jackie-Jiang reviewed May 22, 2024

View reviewed changes

pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpec.java Outdated Show resolved Hide resolved

pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpec.java Outdated Show resolved Hide resolved

tibrewalpratik17 force-pushed the fail_on_sanitization branch 3 times, most recently from 0bbd7de to 9183264 Compare May 22, 2024 10:07

tibrewalpratik17 added 6 commits May 22, 2024 16:40

Add config to skip record ingestion on string column length exceeding…

6975320

… schema max length

handle null and string exceeding length separately

1570072

address comments

0b96516

add FieldSpec

752f7e6

add javadocs

0c70fd6

Add max-length-strategy NO_ACTION

e990348

tibrewalpratik17 force-pushed the fail_on_sanitization branch from 9183264 to e990348 Compare May 22, 2024 11:18

Jackie-Jiang reviewed May 22, 2024

View reviewed changes

address comments

97c0167

tibrewalpratik17 force-pushed the fail_on_sanitization branch from 50ceb6d to 97c0167 Compare May 24, 2024 06:18

Jackie-Jiang reviewed May 24, 2024

View reviewed changes

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 May 24, 2024

deepthi912 mentioned this pull request May 25, 2024

MaxLength limiting included for Json Columns #13227

Open

tibrewalpratik17 force-pushed the fail_on_sanitization branch 2 times, most recently from 3b24d12 to 2c3c923 Compare May 27, 2024 10:15

add support for bytes and json

01b43e6

tibrewalpratik17 force-pushed the fail_on_sanitization branch from 2c3c923 to 01b43e6 Compare May 27, 2024 12:30

refactor fail_ingestion to error

bfc2ca9

tibrewalpratik17 requested a review from Jackie-Jiang May 29, 2024 20:00

Jackie-Jiang reviewed May 30, 2024

View reviewed changes

tibrewalpratik17 force-pushed the fail_on_sanitization branch from bda8321 to 266eba7 Compare May 31, 2024 08:55

address comments 4

e2a63f8

tibrewalpratik17 force-pushed the fail_on_sanitization branch from 266eba7 to e2a63f8 Compare May 31, 2024 10:39

tibrewalpratik17 requested a review from Jackie-Jiang June 2, 2024 18:40

Jackie-Jiang merged commit 9822100 into apache:master Jun 4, 2024
19 checks passed

Jackie-Jiang added the observability label Jun 4, 2024

gortiz pushed a commit to gortiz/pinot that referenced this pull request Jun 14, 2024

Add config to skip record ingestion on string column length exceeding…

bf77a52

… configured max schema length (apache#13103)

tibrewalpratik17 mentioned this pull request Jun 14, 2024

Update pinot-docs related to recent changes pinot-contrib/pinot-docs#351

Merged

Jackie-Jiang mentioned this pull request Jul 11, 2024

MaxLength is not honored for JSON columns (the way is it for long STRING columns) #13123

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add config to skip record ingestion on string column length exceeding configured max schema length #13103

Add config to skip record ingestion on string column length exceeding configured max schema length #13103

tibrewalpratik17 commented May 7, 2024 •

edited

Loading

codecov-commenter commented May 7, 2024 •

edited

Loading

Jackie-Jiang left a comment

satishd left a comment

satishd left a comment

snleee left a comment

Jackie-Jiang left a comment

tibrewalpratik17 commented May 22, 2024

Jackie-Jiang left a comment

tibrewalpratik17 commented May 24, 2024

Jackie-Jiang left a comment

Jackie-Jiang May 30, 2024

tibrewalpratik17 May 31, 2024

tibrewalpratik17 commented Jun 2, 2024

Add config to skip record ingestion on string column length exceeding configured max schema length #13103

Add config to skip record ingestion on string column length exceeding configured max schema length #13103

Conversation

tibrewalpratik17 commented May 7, 2024 • edited Loading

codecov-commenter commented May 7, 2024 • edited Loading

Codecov Report

Jackie-Jiang left a comment

Choose a reason for hiding this comment

satishd left a comment

Choose a reason for hiding this comment

satishd left a comment

Choose a reason for hiding this comment

snleee left a comment

Choose a reason for hiding this comment

Jackie-Jiang left a comment

Choose a reason for hiding this comment

tibrewalpratik17 commented May 22, 2024

Jackie-Jiang left a comment

Choose a reason for hiding this comment

tibrewalpratik17 commented May 24, 2024

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Jackie-Jiang May 30, 2024

Choose a reason for hiding this comment

tibrewalpratik17 May 31, 2024

Choose a reason for hiding this comment

tibrewalpratik17 commented Jun 2, 2024

tibrewalpratik17 commented May 7, 2024 •

edited

Loading

codecov-commenter commented May 7, 2024 •

edited

Loading