Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make RecordEnricher extend RecordTransformer #14601

Merged
merged 1 commit into from
Dec 6, 2024

Conversation

Jackie-Jiang
Copy link
Contributor

Address #13049
Another attempt after #13086 and #13704

  • Make RecordEnricher extend RecordTransformer
  • Integrate RecordEnricherPipeline into CompositeTransformer
  • Add API RecordTransformer.getInputColumns() to return the required input columns, and simplify IngestionUtils.getFieldsForRecordExtractor()

Incompatible

RecordTransformer is moved to pinot-spi module: org.apache.pinot.spi.recordtransformer
RecordEnricher is moved from org.apache.pinot.spi.recordenricher to org.apache.pinot.spi.recordtransformer.enricher

@Jackie-Jiang Jackie-Jiang added incompatible Indicate PR that introduces backward incompatibility ingestion cleanup refactor labels Dec 4, 2024
@codecov-commenter
Copy link

codecov-commenter commented Dec 4, 2024

Codecov Report

Attention: Patch coverage is 57.14286% with 48 lines in your changes missing coverage. Please review.

Project coverage is 63.96%. Comparing base (59551e4) to head (cc9e2a4).
Report is 1428 commits behind head on master.

Files with missing lines Patch % Lines
...ache/pinot/segment/local/utils/IngestionUtils.java 53.12% 12 Missing and 3 partials ⚠️
...rdtransformer/enricher/RecordEnricherRegistry.java 0.00% 12 Missing ⚠️
.../local/recordtransformer/CompositeTransformer.java 75.86% 6 Missing and 1 partial ⚠️
...a/manager/realtime/RealtimeSegmentDataManager.java 40.00% 3 Missing ⚠️
...local/recordtransformer/ExpressionTransformer.java 75.00% 1 Missing and 1 partial ⚠️
...nricher/function/CustomFunctionEnricherConfig.java 0.00% 2 Missing ⚠️
...spi/recordtransformer/enricher/RecordEnricher.java 0.00% 2 Missing ⚠️
...ent/local/recordtransformer/FilterTransformer.java 0.00% 0 Missing and 1 partial ⚠️
...dtransformer/enricher/clp/CLPEncodingEnricher.java 0.00% 1 Missing ⚠️
...former/enricher/clp/CLPEncodingEnricherConfig.java 0.00% 1 Missing ⚠️
... and 2 more
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #14601      +/-   ##
============================================
+ Coverage     61.75%   63.96%   +2.21%     
- Complexity      207     1572    +1365     
============================================
  Files          2436     2687     +251     
  Lines        133233   147650   +14417     
  Branches      20636    22629    +1993     
============================================
+ Hits          82274    94442   +12168     
- Misses        44911    46270    +1359     
- Partials       6048     6938     +890     
Flag Coverage Δ
custom-integration1 100.00% <ø> (+99.99%) ⬆️
integration 100.00% <ø> (+99.99%) ⬆️
integration1 100.00% <ø> (+99.99%) ⬆️
integration2 0.00% <ø> (ø)
java-11 63.92% <57.14%> (+2.21%) ⬆️
java-21 63.85% <57.14%> (+2.23%) ⬆️
skip-bytebuffers-false 63.93% <57.14%> (+2.19%) ⬆️
skip-bytebuffers-true 63.83% <57.14%> (+36.10%) ⬆️
temurin 63.96% <57.14%> (+2.21%) ⬆️
unittests 63.95% <57.14%> (+2.21%) ⬆️
unittests1 55.64% <36.60%> (+8.75%) ⬆️
unittests2 34.60% <52.67%> (+6.87%) ⬆️

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 Jackie-Jiang force-pushed the make_enricher_transformer branch from 09c5d51 to cc9e2a4 Compare December 4, 2024 23:46
Copy link
Contributor

@swaminathanmanish swaminathanmanish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much needed clean up. Thanks Jackie !

Copy link
Contributor

@swaminathanmanish swaminathanmanish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM !

@@ -141,6 +141,7 @@ private void resetBuffer()
@Override
public void collect(GenericRow row)
throws IOException {
// TODO: Revisit whether we should transform the row
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain what the TODO means?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In flush() we will use the collected row to create a segment, which applies the transforms again

@Jackie-Jiang Jackie-Jiang merged commit 694546b into apache:master Dec 6, 2024
21 checks passed
@Jackie-Jiang Jackie-Jiang deleted the make_enricher_transformer branch December 6, 2024 21:40
davecromberge pushed a commit to davecromberge/pinot that referenced this pull request Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup incompatible Indicate PR that introduces backward incompatibility ingestion refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants