You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "J-HowHuang (via GitHub)" <gi...@apache.org> on 2025/08/18 21:26:17 UTC
[PR] Keep original value of necessary columns only in ComplexTypeTranformer [pinot]
J-HowHuang opened a new pull request, #16624:
URL: https://github.com/apache/pinot/pull/16624
## Description
Current `ComplexTypeTranformer` keeps original values of all unnest fields (see https://github.com/apache/pinot/pull/13490).
It introduced issue: https://github.com/apache/pinot/issues/15665, where resources were eaten up by keeping these original values.
This PR adds a method
```
default void withInputColumnsOfDownStreamTransformers(Collection<String> inputColumnsOfDownstream) {
}
```
in `org.apache.pinot.spi.recordtransformer.RecordTransformer`
It provides hint to the transformer that which columns are required as input across all the downstream transformers in the `TransformPipeline`.
In `ComplexTypeTranformer`, if a unnest field is not needed as input in any downstream transformer, we don't keep its original value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Keep value of necessary columns only in ComplexTypeTranformer [pinot]
Posted by "J-HowHuang (via GitHub)" <gi...@apache.org>.
J-HowHuang commented on code in PR #16624:
URL: https://github.com/apache/pinot/pull/16624#discussion_r2283702867
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformer.java:
##########
@@ -233,6 +241,14 @@ public List<GenericRow> transform(List<GenericRow> records) {
}
}
}
+ if (_fieldsNeededForDownstreamTransformers != null) {
Review Comment:
An unnecessary column could be added even it's not the unnest column. For example:
```
"complexTypeConfig": {
"fieldsToUnnest": [
"payload.commits"
]
}
```
With input
```
{
"payload": {
"col_unnecessary": {
"colA": "123"
},
"commits": {
"colA": "123",
"colB": "456"
}
}
}
```
We'll have `payload.col_unnecessary` in the transformed `GenericRow`. This is because during the flattening, it's using BFS and adding one level at a time--`payload.col_unnecessary` and `payload.commits` were added to `GenericRow`, then `flattenMap` recursively called on these two column, then `payload.col_unnecessary.colA`, `payload.commits.colA`, `payload.commits.colB` were added.
We can't avoid adding `payload.col_unnecessary` into `GenericRow` unless we re-write the flattening here with DFS approach
Plus we'll have to rename the prefix first to know if that field is unnecessary or not since the columns passed in with `withInputColumnsOfDownStreamTransformers` is in the context of the names after prefix-renaming.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Keep value of necessary columns only in ComplexTypeTranformer [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #16624:
URL: https://github.com/apache/pinot/pull/16624#discussion_r2286447288
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/readers/GenericRow.java:
##########
@@ -164,7 +164,7 @@ public GenericRow copy() {
/**
* @return a deep copy of the generic row for the given fields
*/
- public GenericRow copy(List<String> fieldsToCopy) {
+ public GenericRow copy(Collection<String> fieldsToCopy) {
Review Comment:
(minor) No need to change this. This breaks backward compatibility test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Keep value of necessary columns only in ComplexTypeTranformer [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #16624:
URL: https://github.com/apache/pinot/pull/16624#discussion_r2286447818
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformer.java:
##########
@@ -358,22 +382,30 @@ protected void flattenMap(GenericRow record, List<String> columns) {
void renamePrefixes(GenericRow record) {
assert !_prefixesToRename.isEmpty();
List<String> fields = new ArrayList<>(record.getFieldToValueMap().keySet());
+ for (String field : fields) {
+ String newName = renamePrefix(field);
+ if (newName == null) {
+ continue;
+ }
+ Object value = record.removeValue(field);
+ if (newName.isEmpty() || record.getValue(newName) != null) {
+ throw new RuntimeException(
+ String.format("Name conflict after attempting to rename field %s to %s", field, newName));
+ }
+ record.putValue(newName, value);
+ }
+ }
+
+ private String renamePrefix(String field) {
Review Comment:
(minor) Annotate return as `@Nullable`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Keep value of necessary columns only in ComplexTypeTranformer [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #16624:
URL: https://github.com/apache/pinot/pull/16624
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Keep value of necessary columns only in ComplexTypeTranformer [pinot]
Posted by "J-HowHuang (via GitHub)" <gi...@apache.org>.
J-HowHuang commented on code in PR #16624:
URL: https://github.com/apache/pinot/pull/16624#discussion_r2283666695
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformer.java:
##########
@@ -180,6 +183,11 @@ public ComplexTypeTransformer build() {
}
}
+ @Override
+ public void withInputColumnsOfDownStreamTransformers(Collection<String> columns) {
+ _fieldsNeededForDownstreamTransformers = new HashSet<>(columns);
Review Comment:
We pass in the cumulative set, which will change when the set roll up to upstream transformers, so we need a deep copy here, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Keep value of necessary columns only in ComplexTypeTranformer [pinot]
Posted by "J-HowHuang (via GitHub)" <gi...@apache.org>.
J-HowHuang commented on code in PR #16624:
URL: https://github.com/apache/pinot/pull/16624#discussion_r2286434650
##########
pinot-spi/src/main/java/org/apache/pinot/spi/recordtransformer/RecordTransformer.java:
##########
@@ -40,6 +41,11 @@ default Collection<String> getInputColumns() {
return List.of();
}
+ /// Provides hint to the transformer that which columns are required as input across all the downstream transformers
+ /// in the TransformPipeline.
+ default void withInputColumnsForDownstreamTransformers(Set<String> inputColumnsOfDownstream) {
Review Comment:
done
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformer.java:
##########
@@ -195,18 +207,20 @@ public List<GenericRow> transform(List<GenericRow> records) {
flattenMap(record, columns);
transformedRecords.add(record);
} else {
- Map<String, Object> originalValues = record.copy(_fieldsToUnnest).getFieldToValueMap();
+ Map<String, Object> originalValues = record.copy(_fieldsToUnnestAndKeepOriginalValue).getFieldToValueMap();
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Keep value of necessary columns only in ComplexTypeTranformer [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #16624:
URL: https://github.com/apache/pinot/pull/16624#discussion_r2286377964
##########
pinot-spi/src/main/java/org/apache/pinot/spi/recordtransformer/RecordTransformer.java:
##########
@@ -40,6 +41,11 @@ default Collection<String> getInputColumns() {
return List.of();
}
+ /// Provides hint to the transformer that which columns are required as input across all the downstream transformers
+ /// in the TransformPipeline.
+ default void withInputColumnsForDownstreamTransformers(Set<String> inputColumnsOfDownstream) {
Review Comment:
(minor)
```suggestion
default void withInputColumnsForDownstreamTransformers(Set<String> inputColumns) {
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformer.java:
##########
@@ -195,18 +207,20 @@ public List<GenericRow> transform(List<GenericRow> records) {
flattenMap(record, columns);
transformedRecords.add(record);
} else {
- Map<String, Object> originalValues = record.copy(_fieldsToUnnest).getFieldToValueMap();
+ Map<String, Object> originalValues = record.copy(_fieldsToUnnestAndKeepOriginalValue).getFieldToValueMap();
Review Comment:
Check if it is empty before making the copy
##########
pinot-spi/src/main/java/org/apache/pinot/spi/recordtransformer/RecordTransformer.java:
##########
@@ -40,6 +41,11 @@ default Collection<String> getInputColumns() {
return List.of();
}
+ /// Provides hint to the transformer that which columns are required as input across all the downstream transformers
+ /// in the TransformPipeline.
+ default void withInputColumnsForDownstreamTransformers(Set<String> inputColumnsOfDownstream) {
Review Comment:
Let's also add some javadoc about the input columns can be modified
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformer.java:
##########
@@ -358,22 +378,30 @@ protected void flattenMap(GenericRow record, List<String> columns) {
void renamePrefixes(GenericRow record) {
assert !_prefixesToRename.isEmpty();
List<String> fields = new ArrayList<>(record.getFieldToValueMap().keySet());
+ for (String field : fields) {
+ String newName = renamePrefix(field);
+ if (newName.equals(field)) {
+ continue;
+ }
+ Object value = record.removeValue(field);
+ if (newName.isEmpty() || record.getValue(newName) != null) {
+ throw new RuntimeException(
+ String.format("Name conflict after attempting to rename field %s to %s", field, newName));
+ }
+ record.putValue(newName, value);
+ }
+ }
+
+ private String renamePrefix(String field) {
for (Map.Entry<String, String> entry : _prefixesToRename.entrySet()) {
- for (String field : fields) {
- String prefix = entry.getKey();
+ String prefix = entry.getKey();
+ if (field.startsWith(prefix)) {
String replacementPrefix = entry.getValue();
- if (field.startsWith(prefix)) {
- Object value = record.removeValue(field);
- String remainingColumnName = field.substring(prefix.length());
- String newName = replacementPrefix + remainingColumnName;
- if (newName.isEmpty() || record.getValue(newName) != null) {
- throw new RuntimeException(
- String.format("Name conflict after attempting to rename field %s to %s", field, newName));
- }
- record.putValue(newName, value);
- }
+ String remainingColumnName = field.substring(prefix.length());
+ return replacementPrefix + remainingColumnName;
}
}
+ return field;
Review Comment:
To reduce overhead, we can return `null` here when there is no prefix match
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformer.java:
##########
@@ -92,11 +95,12 @@ public class ComplexTypeTransformer implements RecordTransformer {
public static final String DEFAULT_DELIMITER = ".";
public static final CollectionNotUnnestedToJson DEFAULT_COLLECTION_TO_JSON_MODE =
CollectionNotUnnestedToJson.NON_PRIMITIVE;
- private final List<String> _fieldsToUnnest;
+ private final SortedSet<String> _fieldsToUnnest;
Review Comment:
We should be able to make both `_fieldsToUnnest` and `_fieldsToUnnestAndKeepOriginalValue` as `List<String>` to reduce overhead when processing the rows
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Keep value of necessary columns only in ComplexTypeTranformer [pinot]
Posted by "J-HowHuang (via GitHub)" <gi...@apache.org>.
J-HowHuang commented on code in PR #16624:
URL: https://github.com/apache/pinot/pull/16624#discussion_r2286470541
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/readers/GenericRow.java:
##########
@@ -164,7 +164,7 @@ public GenericRow copy() {
/**
* @return a deep copy of the generic row for the given fields
*/
- public GenericRow copy(List<String> fieldsToCopy) {
+ public GenericRow copy(Collection<String> fieldsToCopy) {
Review Comment:
done
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformer.java:
##########
@@ -358,22 +382,30 @@ protected void flattenMap(GenericRow record, List<String> columns) {
void renamePrefixes(GenericRow record) {
assert !_prefixesToRename.isEmpty();
List<String> fields = new ArrayList<>(record.getFieldToValueMap().keySet());
+ for (String field : fields) {
+ String newName = renamePrefix(field);
+ if (newName == null) {
+ continue;
+ }
+ Object value = record.removeValue(field);
+ if (newName.isEmpty() || record.getValue(newName) != null) {
+ throw new RuntimeException(
+ String.format("Name conflict after attempting to rename field %s to %s", field, newName));
+ }
+ record.putValue(newName, value);
+ }
+ }
+
+ private String renamePrefix(String field) {
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Keep value of necessary columns only in ComplexTypeTranformer [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #16624:
URL: https://github.com/apache/pinot/pull/16624#discussion_r2285872946
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformer.java:
##########
@@ -233,6 +241,14 @@ public List<GenericRow> transform(List<GenericRow> records) {
}
}
}
+ if (_fieldsNeededForDownstreamTransformers != null) {
Review Comment:
On line 206, we keep a copy of the original values, and add them back to the unnested records in the end. We can use the intersection of `_fieldsToUnnest` and downstream columns to maintain the original values
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Keep value of necessary columns only in ComplexTypeTranformer [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #16624:
URL: https://github.com/apache/pinot/pull/16624#issuecomment-3198687398
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/16624?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `92.30769%` with `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 56.41%. Comparing base ([`3d44c91`](https://app.codecov.io/gh/apache/pinot/commit/3d44c91e3c034c54daaeab8d0bceea20e7000ca6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`2075feb`](https://app.codecov.io/gh/apache/pinot/commit/2075feb81a73e883b6133c66c4f80e5f58263888?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 13 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/16624?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...ocal/recordtransformer/ComplexTypeTransformer.java](https://app.codecov.io/gh/apache/pinot/pull/16624?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Frecordtransformer%2FComplexTypeTransformer.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWNvcmR0cmFuc2Zvcm1lci9Db21wbGV4VHlwZVRyYW5zZm9ybWVyLmphdmE=) | 85.71% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16624?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (3d44c91) and HEAD (2075feb). Click for more details.
>
> <details><summary>HEAD has 32 uploads less than BASE</summary>
>
>| Flag | BASE (3d44c91) | HEAD (2075feb) |
>|------|------|------|
>|java-21|4|1|
>|unittests1|2|1|
>|unittests|4|1|
>|temurin|9|1|
>|unittests2|2|0|
>|java-11|5|0|
>|integration|5|0|
>|integration1|2|0|
>|custom-integration1|2|0|
>|integration2|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #16624 +/- ##
============================================
- Coverage 63.37% 56.41% -6.96%
+ Complexity 1379 699 -680
============================================
Files 3020 2371 -649
Lines 175601 131464 -44137
Branches 26918 20957 -5961
============================================
- Hits 111280 74166 -37114
+ Misses 55828 51146 -4682
+ Partials 8493 6152 -2341
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/16624/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/16624/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/16624/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/16624/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/16624/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/16624/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/16624/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.41% <92.30%> (-6.88%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/16624/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.41% <92.30%> (-6.96%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/16624/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.41% <92.30%> (-6.96%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/16624/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.41% <92.30%> (-0.06%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/16624/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/16624?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Keep value of necessary columns only in ComplexTypeTranformer [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #16624:
URL: https://github.com/apache/pinot/pull/16624#discussion_r2283650928
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformer.java:
##########
@@ -180,6 +183,11 @@ public ComplexTypeTransformer build() {
}
}
+ @Override
+ public void withInputColumnsOfDownStreamTransformers(Collection<String> columns) {
+ _fieldsNeededForDownstreamTransformers = new HashSet<>(columns);
Review Comment:
Shouldn't need to make a copy
##########
pinot-spi/src/main/java/org/apache/pinot/spi/recordtransformer/RecordTransformer.java:
##########
@@ -40,6 +40,11 @@ default Collection<String> getInputColumns() {
return List.of();
}
+ /// Provides hint to the transformer that which columns are required as input across all the downstream transformers
+ /// in the TransformPipeline.
+ default void withInputColumnsOfDownStreamTransformers(Collection<String> inputColumnsOfDownstream) {
Review Comment:
Directly takes a `Set<String>` as argument.
(minor) Suggest renaming it to `withInputColumnsForDownstreamTransformers`, changing `of` to `for`, and treat downstream as one work
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/TransformPipeline.java:
##########
@@ -53,11 +53,14 @@ public TransformPipeline(String tableNameWithType, List<RecordTransformer> trans
_tableNameWithType = tableNameWithType;
_transformers = transformers;
FilterTransformer filterTransformer = null;
- for (RecordTransformer recordTransformer : transformers) {
+ Set<String> cumulativeInputColumns = new HashSet<>();
Review Comment:
You can cache this, as the return of `getInputColumns()`
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformer.java:
##########
@@ -233,6 +241,14 @@ public List<GenericRow> transform(List<GenericRow> records) {
}
}
}
+ if (_fieldsNeededForDownstreamTransformers != null) {
Review Comment:
Let's not add the column, instead of removing them in the end. Removing all other columns are more expensive, and way more dangerous than just not adding the unnested column when it is not included
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Keep value of necessary columns only in ComplexTypeTranformer [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #16624:
URL: https://github.com/apache/pinot/pull/16624#discussion_r2285861163
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformer.java:
##########
@@ -180,6 +183,11 @@ public ComplexTypeTransformer build() {
}
}
+ @Override
+ public void withInputColumnsOfDownStreamTransformers(Collection<String> columns) {
+ _fieldsNeededForDownstreamTransformers = new HashSet<>(columns);
Review Comment:
Good point. We should also add this to the javadoc of the interface where the passed in set can be modified.
I'm thinking making a `Set<String>` of common columns of `_fieldsToUnnest` and this set, which is used to decide wether to keep the original column
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Keep value of necessary columns only in ComplexTypeTranformer [pinot]
Posted by "J-HowHuang (via GitHub)" <gi...@apache.org>.
J-HowHuang commented on code in PR #16624:
URL: https://github.com/apache/pinot/pull/16624#discussion_r2286435525
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformer.java:
##########
@@ -358,22 +378,30 @@ protected void flattenMap(GenericRow record, List<String> columns) {
void renamePrefixes(GenericRow record) {
assert !_prefixesToRename.isEmpty();
List<String> fields = new ArrayList<>(record.getFieldToValueMap().keySet());
+ for (String field : fields) {
+ String newName = renamePrefix(field);
+ if (newName.equals(field)) {
+ continue;
+ }
+ Object value = record.removeValue(field);
+ if (newName.isEmpty() || record.getValue(newName) != null) {
+ throw new RuntimeException(
+ String.format("Name conflict after attempting to rename field %s to %s", field, newName));
+ }
+ record.putValue(newName, value);
+ }
+ }
+
+ private String renamePrefix(String field) {
for (Map.Entry<String, String> entry : _prefixesToRename.entrySet()) {
- for (String field : fields) {
- String prefix = entry.getKey();
+ String prefix = entry.getKey();
+ if (field.startsWith(prefix)) {
String replacementPrefix = entry.getValue();
- if (field.startsWith(prefix)) {
- Object value = record.removeValue(field);
- String remainingColumnName = field.substring(prefix.length());
- String newName = replacementPrefix + remainingColumnName;
- if (newName.isEmpty() || record.getValue(newName) != null) {
- throw new RuntimeException(
- String.format("Name conflict after attempting to rename field %s to %s", field, newName));
- }
- record.putValue(newName, value);
- }
+ String remainingColumnName = field.substring(prefix.length());
+ return replacementPrefix + remainingColumnName;
}
}
+ return field;
Review Comment:
done
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformer.java:
##########
@@ -92,11 +95,12 @@ public class ComplexTypeTransformer implements RecordTransformer {
public static final String DEFAULT_DELIMITER = ".";
public static final CollectionNotUnnestedToJson DEFAULT_COLLECTION_TO_JSON_MODE =
CollectionNotUnnestedToJson.NON_PRIMITIVE;
- private final List<String> _fieldsToUnnest;
+ private final SortedSet<String> _fieldsToUnnest;
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org