#13049 Refactored RecordTransformer & merged RecordEnricher by deepthi912 · Pull Request #13086 · apache/pinot

deepthi912 · 2024-05-06T04:53:49Z

Removed RecordEnricher, RecordEnricherPipeline, RecordEnricherValidationConfig(directly using the boolean instead of having a class for this), RecordEnricherFactory and moved the methods into RecordTransformer.
Moved some of the methods from RecordEnricherRegistry into RecordTransformer except static block ---> This will help in recording the details of type of ericher into a map while loading.
Removed CLPEncodingEnricherFactory and CustomFunctionEnricherFactory classes and merged into respective EncodingEricher classes

…moved RecordEnricher as it serves similar purpose as RecordTransformer.

codecov-commenter · 2024-05-06T06:00:35Z

Codecov Report

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

Project coverage is 62.16%. Comparing base (59551e4) to head (437c840).
Report is 521 commits behind head on master.

Files	Patch %	Lines
...ent/local/recordtransformer/RecordTransformer.java	24.39%	29 Missing and 2 partials ⚠️
...cord/enricher/function/CustomFunctionEnricher.java	0.00%	13 Missing ⚠️
...ache/pinot/segment/local/utils/IngestionUtils.java	33.33%	11 Missing and 1 partial ⚠️
...lugin/record/enricher/clp/CLPEncodingEnricher.java	0.00%	8 Missing ⚠️
...he/pinot/common/utils/config/TableConfigUtils.java	45.45%	4 Missing and 2 partials ⚠️
...he/pinot/segment/local/utils/TableConfigUtils.java	69.23%	1 Missing and 3 partials ⚠️
...l/realtime/converter/RealtimeSegmentConverter.java	77.77%	2 Missing ⚠️
...plugin/record/enricher/RecordEnricherRegistry.java	66.66%	0 Missing and 1 partial ⚠️
...creator/RecordReaderSegmentCreationDataSource.java	50.00%	0 Missing and 1 partial ⚠️
...t/creator/impl/SegmentIndexCreationDriverImpl.java	88.88%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #13086      +/-   ##
============================================
+ Coverage     61.75%   62.16%   +0.40%     
+ Complexity      207      198       -9     
============================================
  Files          2436     2530      +94     
  Lines        133233   139028    +5795     
  Branches      20636    21544     +908     
============================================
+ Hits          82274    86420    +4146     
- Misses        44911    46136    +1225     
- Partials       6048     6472     +424

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.13% <40.15%> (+0.42%)`	⬆️
java-21	`62.04% <40.15%> (+0.41%)`	⬆️
skip-bytebuffers-false	`62.15% <40.15%> (+0.40%)`	⬆️
skip-bytebuffers-true	`62.03% <40.15%> (+34.30%)`	⬆️
temurin	`62.16% <40.15%> (+0.40%)`	⬆️
unittests	`62.15% <40.15%> (+0.40%)`	⬆️
unittests1	`46.67% <6.87%> (-0.23%)`	⬇️
unittests2	`27.82% <33.33%> (+0.09%)`	⬆️

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

We should be able to delete the record enricher related interfaces and general handling classes (e.g. factory, pipeline, registry) after merging it into record transformer package. Can you take a look and see if it is possible to further clean up the code?

deepthi912 · 2024-05-10T23:52:59Z

Sure, I wanted to do that too. Will look and see where I can improve further

deepthi912 · 2024-05-11T04:53:55Z

@Jackie-Jiang I noticed that registery, pipeline have static methods which are used by some of the classes. These static methods are calling RecordEnricherFactory which is again implemented by function and clp Enricher classes. I tried to check if I could squeeze in anywhere but no right spot. Not sure if by just removing RecordEnricher, we can make any big difference in refactoring though.

Jackie-Jiang · 2024-05-13T22:38:41Z

Basically the idea is to re-do #12243 and integrate CLP transform as a RecordTransformer

…rom RecordEnricher package

deepthi-912 · 2024-06-02T01:32:02Z

@Jackie-Jiang PR is ready for review.

Jackie-Jiang · 2024-06-02T05:22:35Z

...-flink-connector/src/main/java/org/apache/pinot/connector/flink/sink/FlinkSegmentWriter.java

  private Schema _schema;
  private Set<String> _fieldsToRead;
-  private RecordEnricherPipeline _recordEnricherPipeline;
+  private RecordTransformer _recordEnricherPipeline;


After changing RecordEnricher into RecordTransformer, we should be able to put the RecordEnricher in front of the existing RecordTransformers as part of the default CompositeTransformer

Jackie-Jiang · 2024-06-02T05:28:21Z

...ment-local/src/main/java/org/apache/pinot/plugin/record/enricher/RecordEnricherRegistry.java

 import org.slf4j.LoggerFactory;


 public class RecordEnricherRegistry {


I think we should be able to remove this registry and follow the existing way of creating RecordTransformer. Same for other places where 2 pipelines exist

Jackie-Jiang · 2024-06-02T05:29:12Z

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

  private final PartitionGroupConsumptionStatus _partitionGroupConsumptionStatus;
  final String _clientId;
-  private final RecordEnricherPipeline _recordEnricherPipeline;
+  private final RecordTransformer _recordEnricherPipeline;


Same here, we should be able to integrate it into the TransformPipeline

Got it. I will take a look into TransformPipeline. Didn't know this class existed. Will modify according to it.

Jackie-Jiang · 2024-06-02T05:44:15Z

...-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/RecordTransformer.java

 * The record transformer which takes a {@link GenericRow} and transform it based on some custom rules.
 */
 public interface RecordTransformer extends Serializable {
+  final boolean GROOVY_DISABLED = false;


Suggest keeping the interface simple. We need to add getInputColumns() from RecordEnricher, other method seems unnecessary. Take a look at CompositeTransformer for record transformer handling

Jackie-Jiang · 2024-06-02T05:47:10Z

...java/org/apache/pinot/segment/local/segment/creator/impl/SegmentIndexCreationDriverImpl.java

  private long _totalStatsCollectorTime = 0;
  private boolean _continueOnError;

+  public static void persistCreationMeta(File indexDir, long crc, long creationTime)


These changes are not related to this PR. I guess you might enabled Rearrange code option in IDE during auto reformat. Can you revert these unrelated changes? There are quite some unrelated auto reformat in several files

Yeah. I will look into this formatting issue.

Jackie-Jiang · 2024-12-06T21:41:39Z

Merge #14601 instead

deepthi912 added 4 commits May 6, 2024 00:41

apache#13049 Refactor RecordTransformer to include RecordEnricher. Re…

d066872

…moved RecordEnricher as it serves similar purpose as RecordTransformer.

tranform RecordTransformer method

ee42776

Formatting issues in build

d4f3cd6

Formatting of code in Pinot

b4eb21b

Jackie-Jiang added cleanup Code cleanup or removal of dead code refactor Code restructuring without changing behavior labels May 10, 2024

Jackie-Jiang reviewed May 10, 2024

View reviewed changes

Remove unused interface RecordEnricherConfig

0e02293

deepthi912 added 9 commits May 30, 2024 00:05

Merge Pipeline, Factory and ValidationConfig into RecordTransformer f…

12e9a87

…rom RecordEnricher package

Checkstyle changes for the PR

f4424da

Checkstyle exceptions in the PR

e261dae

Checkstyle exception

4a18a04

Create a private constructer for not allowing instantiation

b328fec

checkstyle exception

cfdf2c9

checkstyle violation - formatting

4202736

Failing test - RecordTransformer

437c840

Checkstyle change - Pinot test skipped

7c7f3cd

Jackie-Jiang reviewed Jun 2, 2024

View reviewed changes

Jackie-Jiang mentioned this pull request Dec 4, 2024

Make RecordEnricher extend RecordTransformer #14601

Merged

Jackie-Jiang closed this Dec 6, 2024

		import org.slf4j.LoggerFactory;


		public class RecordEnricherRegistry {

Conversation

deepthi912 commented May 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented May 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

deepthi912 commented May 10, 2024

Uh oh!

deepthi912 commented May 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jackie-Jiang commented May 13, 2024

Uh oh!

deepthi-912 commented Jun 2, 2024

Uh oh!

Jackie-Jiang Jun 2, 2024

Choose a reason for hiding this comment

Uh oh!

Jackie-Jiang Jun 2, 2024

Choose a reason for hiding this comment

Uh oh!

Jackie-Jiang Jun 2, 2024

Choose a reason for hiding this comment

Uh oh!

deepthi912 Jun 2, 2024

Choose a reason for hiding this comment

Uh oh!

Jackie-Jiang Jun 2, 2024

Choose a reason for hiding this comment

Uh oh!

Jackie-Jiang Jun 2, 2024

Choose a reason for hiding this comment

Uh oh!

deepthi912 Jun 2, 2024

Choose a reason for hiding this comment

Uh oh!

Jackie-Jiang commented Dec 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

deepthi912 commented May 6, 2024 •

edited

Loading

codecov-commenter commented May 6, 2024 •

edited

Loading

deepthi912 commented May 11, 2024 •

edited

Loading