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

Merge RecordEnricher into RecordTransformer #13704

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aadilkhalifa
Copy link
Contributor

PR for #13049

@aadilkhalifa aadilkhalifa force-pushed the merge-enricher-transformer branch 2 times, most recently from ce7e9d0 to bffd235 Compare July 29, 2024 16:51
@codecov-commenter
Copy link

codecov-commenter commented Jul 29, 2024

Codecov Report

Attention: Patch coverage is 38.15789% with 47 lines in your changes missing coverage. Please review.

Project coverage is 62.02%. Comparing base (59551e4) to head (ccd307b).
Report is 811 commits behind head on master.

Files Patch % Lines
...l/recordtransformer/RecordTransformerRegistry.java 0.00% 16 Missing ⚠️
...l/recordtransformer/RecordTransformerPipeline.java 53.84% 4 Missing and 2 partials ⚠️
.../recordtransformer/clp/CLPEncodingTransformer.java 0.00% 6 Missing ⚠️
...mer/function/CustomFunctionTransformerFactory.java 0.00% 5 Missing ⚠️
...transformer/clp/CLPEncodingTransformerFactory.java 0.00% 3 Missing ⚠️
...ransformer/function/CustomFunctionTransformer.java 0.00% 3 Missing ⚠️
...he/pinot/segment/local/utils/TableConfigUtils.java 0.00% 2 Missing and 1 partial ⚠️
...ent/local/recordtransformer/RecordTransformer.java 0.00% 1 Missing ⚠️
...transformer/RecordTransformerValidationConfig.java 0.00% 1 Missing ⚠️
...al/recordtransformer/clp/ClpTransformerConfig.java 0.00% 1 Missing ⚠️
... and 2 more
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #13704      +/-   ##
============================================
+ Coverage     61.75%   62.02%   +0.26%     
+ Complexity      207      198       -9     
============================================
  Files          2436     2553     +117     
  Lines        133233   140605    +7372     
  Branches      20636    21874    +1238     
============================================
+ Hits          82274    87205    +4931     
- Misses        44911    46773    +1862     
- Partials       6048     6627     +579     
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 61.98% <38.15%> (+0.27%) ⬆️
java-21 61.90% <38.15%> (+0.27%) ⬆️
skip-bytebuffers-false 62.01% <38.15%> (+0.26%) ⬆️
skip-bytebuffers-true 61.85% <38.15%> (+34.12%) ⬆️
temurin 62.02% <38.15%> (+0.26%) ⬆️
unittests 62.01% <38.15%> (+0.26%) ⬆️
unittests1 46.45% <16.21%> (-0.44%) ⬇️
unittests2 27.79% <22.36%> (+0.06%) ⬆️

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.

@aadilkhalifa aadilkhalifa force-pushed the merge-enricher-transformer branch 2 times, most recently from 007c783 to 2aca00f Compare July 30, 2024 05:02
@aadilkhalifa aadilkhalifa changed the title [WIP]: Merge RecordEnricher into RecordTransformer Merge RecordEnricher into RecordTransformer Aug 4, 2024
Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

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

Thanks for taking up this task!

*/
void enrich(GenericRow record);
}
//public interface RecordEnricher {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please cleanup the commented lines, and files that are no longer needed. Same for other places

@@ -129,7 +129,7 @@ public void build(@Nullable SegmentVersion segmentVersion, ServerMetrics serverM
recordReader.init(_realtimeSegmentImpl, sortedDocIds);
RealtimeSegmentSegmentCreationDataSource dataSource =
new RealtimeSegmentSegmentCreationDataSource(_realtimeSegmentImpl, recordReader);
driver.init(genConfig, dataSource, RecordEnricherPipeline.getPassThroughPipeline(),
driver.init(genConfig, dataSource, RecordTransformerPipeline.getPassThroughPipeline(),
Copy link
Contributor

Choose a reason for hiding this comment

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

We want to integrate the current RecordEnricher as one type of RecordTransformer. With that, we shouldn't need a separate pipeline other than TransformPipeline

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants