Add prefixesToRename config for renaming fields upon ingestion #8273

jeffreyliu34 · 2022-03-02T01:45:26Z

Description

This change allows fields to be renamed upon ingestion to avoid duplication of columns/data. More specifically, the proposed change will enabling dropping specified prefixes of fields that are in the source data, so that the resulting field names do not have those prefixes. This addresses issue #8161

More details here in this design doc here - https://docs.google.com/document/d/1U_vQC0BiCCEcx49Tsp499V5F075iJ3fW9IrsD8sNdU0/edit?usp=sharing

Upgrade Notes

Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)

No (Please label as backward-incompat, and complete the section below on Release Notes)

Does this PR fix a zero-downtime upgrade introduced earlier?

No (Please label this as backward-incompat, and complete the section below on Release Notes)

Does this PR otherwise need attention when creating release notes? Things to consider:

New configuration options
Deprecation of configurations
Signature changes to public methods/interfaces
New plugins added or old plugins removed

Yes (Please label this PR as release-notes and complete the section on Release Notes)

Release Notes

Added prefixesToRename config to rename columns upon ingestion based on their prefixes

Documentation

codecov-commenter · 2022-03-02T21:16:42Z

Codecov Report

Merging #8273 (5072901) into master (e8c849e) will decrease coverage by 0.30%.
The diff coverage is 69.04%.

@@             Coverage Diff              @@
##             master    #8273      +/-   ##
============================================
- Coverage     71.09%   70.78%   -0.31%     
+ Complexity     4314     4265      -49     
============================================
  Files          1626     1636      +10     
  Lines         84881    85844     +963     
  Branches      12788    12931     +143     
============================================
+ Hits          60343    60764     +421     
- Misses        20394    20886     +492     
- Partials       4144     4194      +50

Flag	Coverage Δ
integration1	`28.90% <0.00%> (+0.09%)`	⬆️
integration2	`27.66% <0.00%> (+0.13%)`	⬆️
unittests1	`66.92% <69.04%> (-0.49%)`	⬇️
unittests2	`14.18% <0.00%> (+0.06%)`	⬆️

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

Impacted Files	Coverage Δ
...he/pinot/segment/local/utils/TableConfigUtils.java	`65.34% <25.00%> (-0.73%)`	⬇️
...ache/pinot/segment/local/utils/IngestionUtils.java	`25.00% <66.66%> (+5.74%)`	⬆️
...ocal/recordtransformer/ComplexTypeTransformer.java	`58.57% <78.57%> (+2.89%)`	⬆️
.../spi/config/table/ingestion/ComplexTypeConfig.java	`100.00% <100.00%> (ø)`
...data/manager/realtime/DefaultSegmentCommitter.java	`0.00% <0.00%> (-80.00%)`	⬇️
...ls/nativefst/automaton/MinimizationOperations.java	`0.00% <0.00%> (-36.20%)`	⬇️
...org/apache/pinot/spi/config/table/QueryConfig.java	`53.33% <0.00%> (-26.67%)`	⬇️
...a/manager/realtime/RealtimeSegmentDataManager.java	`50.00% <0.00%> (-25.00%)`	⬇️
...tils/nativefst/automaton/TransitionComparator.java	`9.37% <0.00%> (-25.00%)`	⬇️
...er/api/resources/LLCSegmentCompletionHandlers.java	`43.56% <0.00%> (-18.82%)`	⬇️
... and 170 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e8c849e...5072901. Read the comment docs.

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

yupeng9 · 2022-03-04T16:23:44Z

I feel the config of prefixesToDropFromFields is a bit limited. Perhaps we can consider a config of prefixRename or sth similar to allow more manipulation of the prefix? So prefixesToDropFromFields will become a special case of this which is rename to empty.

Also, how do we handle the collision from multiple nested columns that happen to have the same name, after the prefix drop?

jeffreyliu34 · 2022-03-04T20:51:54Z

Yea I like that. How does something like this look? @yupeng9

prefixesToRename: [
    {
         "prefixName": "after."
         "prefixReplace": ""
    },
    ...
]

In terms of avoiding collisions with same column names, perhaps we can add a validation check when the table config is added? I believe there's similar validation checks in place currently for comparing column names in the configs against the schema cc @icefury71

Also since the general idea is to allow for renaming columns from the source data upon ingestion, we could also end up expanding on this via some different approaches. One approach could be to specify the exact full column names we want to rename, and specify what we want to rename it to, like how we have that capability in transform configs. Another approach would be to apply the config in batch, so like batch removing/renaming common prefixes, or batch converting snake_case to camelCase, etc. so that it's not tedious to specify everything like in the first approach. For this PR, I was thinking of just starting with prefixRename but definitely appreciate any thoughts.

yupeng9 · 2022-03-04T22:58:44Z

Yea I like that. How does something like this look? @yupeng9
prefixesToRename: [
    {
         "prefixName": "after."
         "prefixReplace": ""
    },
    ...
]
In terms of avoiding collisions with same column names, perhaps we can add a validation check when the table config is added? I believe there's similar validation checks in place currently for comparing column names in the configs against the schema cc @icefury71

Also since the general idea is to allow for renaming columns from the source data upon ingestion, we could also end up expanding on this via some different approaches. One approach could be to specify the exact full column names we want to rename, and specify what we want to rename it to, like how we have that capability in transform configs. Another approach would be to apply the config in batch, so like batch removing/renaming common prefixes, or batch converting snake_case to camelCase, etc. so that it's not tedious to specify everything like in the first approach. For this PR, I was thinking of just starting with prefixRename but definitely appreciate any thoughts.

I think it could be even simpler:

prefixesToRename:   { "after.": "" },

Yes, validation works, since this is at the schema level.

jeffreyliu34 · 2022-03-11T01:51:00Z

Thanks @yupeng9 , I've updated based on your suggestion and added validation

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

yupeng9

LGTM. Thanks for adding this option

Jackie-Jiang

In the release note session, could you please add the actual config key so that user knows how to config it by reading the release note?

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

Jackie-Jiang · 2022-03-13T16:46:37Z

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

+    if (_prefixesToRename.isEmpty()) {
+      return;
+    }
+    List<String> fields = new ArrayList<>(record.getFieldToValueMap().keySet());


Avoid creating this extra list

Ah actually, we need this list to avoid ConcurrentModificationException

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

pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java

pinot-spi/src/main/java/org/apache/pinot/spi/config/table/ingestion/ComplexTypeConfig.java

pinot-common/src/test/java/org/apache/pinot/common/utils/config/TableConfigSerDeTest.java

…g/TableConfigSerDeTest.java Co-authored-by: Xiaotian (Jackie) Jiang <[email protected]>

jeffreyliu34 · 2022-03-14T20:46:22Z

In the release note session, could you please add the actual config key so that user knows how to config it by reading the release note?

Yep updated. Thanks for the review @Jackie-Jiang !

Jeffrey Liu added 2 commits March 1, 2022 09:56

Allow renaming of unnested fields upon ingestion

59ad3fe

Rename config and add unit tests

2242987

icefury71 requested a review from yupeng9 March 3, 2022 21:33

icefury71 reviewed Mar 3, 2022

View reviewed changes

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

Jeffrey Liu added 2 commits March 10, 2022 13:40

Refactor renaming of prefixes

22b21a1

Add validation for prefixesToRename config

df49b32

jeffreyliu34 changed the title ~~Add prefixesToDropFromFields config for renaming fields upon ingestion~~ Add prefixesToRename config for renaming fields upon ingestion Mar 11, 2022

yupeng9 reviewed Mar 11, 2022

View reviewed changes

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

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

Fix empty name after renaming case

f1c226f

yupeng9 approved these changes Mar 12, 2022

View reviewed changes

Jackie-Jiang added the release-notes Referenced by PRs that need attention when compiling the next release notes label Mar 13, 2022

Jackie-Jiang reviewed Mar 13, 2022

View reviewed changes

Fix code style nits

981843c

Jackie-Jiang approved these changes Mar 14, 2022

View reviewed changes

pinot-common/src/test/java/org/apache/pinot/common/utils/config/TableConfigSerDeTest.java Outdated Show resolved Hide resolved

Update pinot-common/src/test/java/org/apache/pinot/common/utils/confi…

d8895c8

…g/TableConfigSerDeTest.java Co-authored-by: Xiaotian (Jackie) Jiang <[email protected]>

Update previous nits to pass unit tests

5072901

icefury71 merged commit 360a205 into apache:master Mar 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add prefixesToRename config for renaming fields upon ingestion #8273

Add prefixesToRename config for renaming fields upon ingestion #8273

jeffreyliu34 commented Mar 2, 2022 •

edited

Loading

codecov-commenter commented Mar 2, 2022 •

edited

Loading

yupeng9 commented Mar 4, 2022

jeffreyliu34 commented Mar 4, 2022 •

edited

Loading

yupeng9 commented Mar 4, 2022

jeffreyliu34 commented Mar 11, 2022

yupeng9 left a comment

Jackie-Jiang left a comment

Jackie-Jiang Mar 13, 2022

jeffreyliu34 Mar 14, 2022

jeffreyliu34 commented Mar 14, 2022

Add prefixesToRename config for renaming fields upon ingestion #8273

Add prefixesToRename config for renaming fields upon ingestion #8273

Conversation

jeffreyliu34 commented Mar 2, 2022 • edited Loading

Description

Upgrade Notes

Release Notes

Documentation

codecov-commenter commented Mar 2, 2022 • edited Loading

Codecov Report

yupeng9 commented Mar 4, 2022

jeffreyliu34 commented Mar 4, 2022 • edited Loading

yupeng9 commented Mar 4, 2022

jeffreyliu34 commented Mar 11, 2022

yupeng9 left a comment

Choose a reason for hiding this comment

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Jackie-Jiang Mar 13, 2022

Choose a reason for hiding this comment

jeffreyliu34 Mar 14, 2022

Choose a reason for hiding this comment

jeffreyliu34 commented Mar 14, 2022

jeffreyliu34 commented Mar 2, 2022 •

edited

Loading

codecov-commenter commented Mar 2, 2022 •

edited

Loading

jeffreyliu34 commented Mar 4, 2022 •

edited

Loading