Add support for gracefully handling the errors while transformations by KKcorps · Pull Request #9377 · apache/pinot

KKcorps · 2022-09-11T11:28:37Z

Add support for honouring continueOnError during

complexTypeTransform
filterTransform
recordReader

Also, adds support for keeping track of such records using incompleteRecordCount

codecov-commenter · 2022-09-11T12:11:05Z

Codecov Report

Merging #9377 (ebe6dad) into master (65e97d2) will decrease coverage by 1.36%.
The diff coverage is 53.03%.

@@             Coverage Diff              @@
##             master    #9377      +/-   ##
============================================
- Coverage     69.76%   68.40%   -1.37%     
+ Complexity     4787     4705      -82     
============================================
  Files          1885     1885              
  Lines        100293   100464     +171     
  Branches      15256    15292      +36     
============================================
- Hits          69966    68718    -1248     
- Misses        25381    26793    +1412     
- Partials       4946     4953       +7

Flag	Coverage Δ
integration1	`26.04% <0.00%> (-0.05%)`	⬇️
integration2	`?`
unittests1	`66.89% <53.03%> (-0.06%)`	⬇️
unittests2	`15.35% <0.00%> (+0.04%)`	⬆️

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

Impacted Files	Coverage Δ
.../org/apache/pinot/spi/data/readers/GenericRow.java	`66.66% <ø> (ø)`
...gment/local/segment/creator/TransformPipeline.java	`83.72% <42.85%> (-7.95%)`	⬇️
...ent/local/recordtransformer/FilterTransformer.java	`59.09% <43.75%> (-40.91%)`	⬇️
...ocal/recordtransformer/ComplexTypeTransformer.java	`58.71% <47.05%> (-1.87%)`	⬇️
...t/creator/impl/SegmentIndexCreationDriverImpl.java	`80.10% <52.63%> (-3.62%)`	⬇️
...t/local/recordtransformer/DataTypeTransformer.java	`91.34% <100.00%> (+0.08%)`	⬆️
...local/recordtransformer/ExpressionTransformer.java	`89.06% <100.00%> (+0.35%)`	⬆️
...t/core/plan/StreamingInstanceResponsePlanNode.java	`0.00% <0.00%> (-100.00%)`	⬇️
...ore/operator/streaming/StreamingResponseUtils.java	`0.00% <0.00%> (-100.00%)`	⬇️
...server/starter/helix/SegmentReloadStatusValue.java	`0.00% <0.00%> (-100.00%)`	⬇️
... and 178 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

xiangfu0 · 2022-09-11T23:10:48Z

...l/src/main/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformer.java

-    flattenMap(record, new ArrayList<>(record.getFieldToValueMap().keySet()));
-    for (String collection : _fieldsToUnnest) {
-      unnestCollection(record, collection);
+    try {


cc: @Jackie-Jiang I remember you mentioned that try-catch will impact the performance? shall do if-else check first?

Codewise I think this is simpler.

if (_continueOnError) { flattenMap(record, new ArrayList<>(record.getFieldToValueMap().keySet())); for (String collection : _fieldsToUnnest) { unnestCollection(record, collection); } } else { try { flattenMap(record, new ArrayList<>(record.getFieldToValueMap().keySet())); for (String collection : _fieldsToUnnest) { unnestCollection(record, collection); } } catch (Exception e) { record.putValue(GenericRow.INCOMPLETE_RECORD_KEY, true); LOGGER.debug("Caught exception while transforming complex types for record: {}", record.toString(), e); } }

I can do that. It just looked like code was being duplicated and hence didn't do it

@Jackie-Jiang AFAIK, compiler doesn't optimize the code inside the try block, but other than that there's no performance overhead for just using a try block. Let me know if you have experienced otherwise.

The current way should be fine. I don't remember seeing extra overhead on try-catch

Thanks @Jackie-Jiang for confirming! @KKcorps can you also update the change in this PR(https://github.com/apache/pinot/pull/9376/files) to make the code consistent?

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

xiangfu0

lgtm, otherwise

sajjad-moradi

This PR only handles segment generation part. We need to take care of the realtime ingestion as well.

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

sajjad-moradi · 2022-09-12T21:44:06Z

...l/src/main/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformer.java

-    flattenMap(record, new ArrayList<>(record.getFieldToValueMap().keySet()));
-    for (String collection : _fieldsToUnnest) {
-      unnestCollection(record, collection);
+    try {


@Jackie-Jiang AFAIK, compiler doesn't optimize the code inside the try block, but other than that there's no performance overhead for just using a try block. Let me know if you have experienced otherwise.

Jackie-Jiang

This is great!

Jackie-Jiang · 2022-09-13T19:00:07Z

...l/src/main/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformer.java

-    flattenMap(record, new ArrayList<>(record.getFieldToValueMap().keySet()));
-    for (String collection : _fieldsToUnnest) {
-      unnestCollection(record, collection);
+    try {


The current way should be fine. I don't remember seeing extra overhead on try-catch

...al/src/main/java/org/apache/pinot/segment/local/recordtransformer/ExpressionTransformer.java

KKcorps · 2022-09-14T15:17:48Z

This PR only handles segment generation part. We need to take care of the realtime ingestion as well.

Agreed. The reason I haven't tackled it in this PR is because it is not clear what user can do to fix missing data in case we ignore it in realtime-ingestion.

sajjad-moradi · 2022-09-14T21:16:57Z

This PR only handles segment generation part. We need to take care of the realtime ingestion as well.

Agreed. The reason I haven't tackled it in this PR is because it is not clear what user can do to fix missing data in case we ignore it in realtime-ingestion.

IMO it should be up to users. If they set continueOnError to true, they have agreed that's the behavior they want, so RT ingestion continues. If they don't set it or if they set it to false, then ingestion stops and they'll take care of the issue right away.

…pache#9377) * Add support for gracefully handling the errors while transformations * Maintain consistent checks across transformers * Add incompleteRowCount metric to realtime servers Co-authored-by: Kartik Khare <kharekartik@Kartiks-MacBook-Pro.local>

KKcorps requested review from Jackie-Jiang and xiangfu0 September 11, 2022 11:28

Add support for gracefully handling the errors while transformations

2574c9f

KKcorps force-pushed the continueOnError_patch branch from e1fe791 to 2574c9f Compare September 11, 2022 11:33

xiangfu0 reviewed Sep 11, 2022

View reviewed changes

xiangfu0 reviewed Sep 12, 2022

View reviewed changes

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

xiangfu0 approved these changes Sep 12, 2022

View reviewed changes

sajjad-moradi reviewed Sep 12, 2022

View reviewed changes

Jackie-Jiang reviewed Sep 13, 2022

View reviewed changes

Maintain consistent checks across transformers

ebe6dad

KKcorps force-pushed the continueOnError_patch branch from f7b3844 to ebe6dad Compare September 14, 2022 14:49

Add incompleteRowCount metric to realtime servers

cb19689

KKcorps merged commit bdf632c into apache:master Sep 22, 2022

rohityadav1993 mentioned this pull request Oct 16, 2025

column major segment build for columnar datasource #16727

Merged

Conversation

KKcorps commented Sep 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Sep 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

xiangfu0 Sep 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KKcorps Sep 12, 2022

Choose a reason for hiding this comment

Uh oh!

sajjad-moradi Sep 12, 2022

Choose a reason for hiding this comment

Uh oh!

Jackie-Jiang Sep 13, 2022

Choose a reason for hiding this comment

Uh oh!

xiangfu0 Sep 13, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xiangfu0 left a comment

Choose a reason for hiding this comment

Uh oh!

sajjad-moradi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sajjad-moradi Sep 12, 2022

Choose a reason for hiding this comment

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

Jackie-Jiang Sep 13, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

KKcorps commented Sep 14, 2022

Uh oh!

sajjad-moradi commented Sep 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

KKcorps commented Sep 11, 2022 •

edited

Loading

codecov-commenter commented Sep 11, 2022 •

edited

Loading

xiangfu0 Sep 11, 2022 •

edited

Loading