You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2021/05/14 00:01:54 UTC
[GitHub] [incubator-pinot] yupeng9 opened a new pull request #6918: Convert collections not unnested to json
yupeng9 opened a new pull request #6918:
URL: https://github.com/apache/incubator-pinot/pull/6918
## Description
Part of https://github.com/apache/incubator-pinot/issues/6904
This PR converts the collections that are not marked in `unnestFields` to JSON string. Note that this conversion does not apply on collections of primitive 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.
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
[GitHub] [incubator-pinot] amrishlal commented on a change in pull request #6918: Convert collections not unnested to json
Posted by GitBox <gi...@apache.org>.
amrishlal commented on a change in pull request #6918:
URL: https://github.com/apache/incubator-pinot/pull/6918#discussion_r633788767
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformer.java
##########
@@ -256,23 +318,73 @@ private void flattenMap(String path, Map<String, Object> map, Collection<String>
flattenMap(concatName, map, innerMapFields);
}
} else if (value instanceof Collection && _unnestFields.contains(concatName)) {
- for (Object inner : (Collection) value) {
- if (inner instanceof Map) {
- Map<String, Object> innerMap = (Map<String, Object>) inner;
- flattenMap(concatName, innerMap, new ArrayList<>(innerMap.keySet()));
+ Collection collection = (Collection) value;
+ if (_unnestFields.contains(concatName)) {
+ for (Object inner : (Collection) value) {
+ if (inner instanceof Map) {
+ Map<String, Object> innerMap = (Map<String, Object>) inner;
+ flattenMap(concatName, innerMap, new ArrayList<>(innerMap.keySet()));
+ }
+ }
+ } else if (shallConvertToJson(collection)) {
+ try {
+ // convert the collection to JSON string
+ String jsonString = JsonFunctions.jsonFormat(collection);
+ map.put(field, jsonString);
+ } catch (JsonProcessingException e) {
+ throw new RuntimeException(
+ String.format("Caught exception while converting value to JSON string %s", value), e);
}
}
- } else if (isArray(value) && _unnestFields.contains(concatName)) {
- for (Object inner : (Object[]) value) {
- if (inner instanceof Map) {
- Map<String, Object> innerMap = (Map<String, Object>) inner;
- flattenMap(concatName, innerMap, new ArrayList<>(innerMap.keySet()));
+ } else if (isArray(value)) {
+ Object[] array = (Object[]) value;
+ if (_unnestFields.contains(concatName)) {
+ for (Object inner : (Object[]) value) {
+ if (inner instanceof Map) {
+ Map<String, Object> innerMap = (Map<String, Object>) inner;
+ flattenMap(concatName, innerMap, new ArrayList<>(innerMap.keySet()));
+ }
+ }
+ } else if (shallConvertToJson(array)) {
+ try {
+ // convert the array to JSON string
+ String jsonString = JsonFunctions.jsonFormat(array);
+ map.put(field, jsonString);
+ } catch (JsonProcessingException e) {
+ throw new RuntimeException(
+ String.format("Caught exception while converting value to JSON string %s", value), e);
}
}
}
}
}
+ private boolean shallConvertToJson(Object[] value) {
+ switch (_collectionToJsonMode) {
+ case ALL:
+ return true;
+ case NONE:
+ return false;
+ case NON_PRIMITIVE:
+ return !containPrimitives(value);
+ default:
+ throw new IllegalArgumentException(String.format("Unsupported collectionToJsonMode %s", _collectionToJsonMode));
+ }
+ }
+
+ private boolean shallConvertToJson(Collection value) {
Review comment:
Instead of duplicating code, just do:
`return shallConvertToJson(value.toArray());`
--
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.
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
[GitHub] [incubator-pinot] yupeng9 commented on a change in pull request #6918: Convert collections not unnested to json
Posted by GitBox <gi...@apache.org>.
yupeng9 commented on a change in pull request #6918:
URL: https://github.com/apache/incubator-pinot/pull/6918#discussion_r634062517
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformer.java
##########
@@ -256,23 +318,73 @@ private void flattenMap(String path, Map<String, Object> map, Collection<String>
flattenMap(concatName, map, innerMapFields);
}
} else if (value instanceof Collection && _unnestFields.contains(concatName)) {
- for (Object inner : (Collection) value) {
- if (inner instanceof Map) {
- Map<String, Object> innerMap = (Map<String, Object>) inner;
- flattenMap(concatName, innerMap, new ArrayList<>(innerMap.keySet()));
+ Collection collection = (Collection) value;
+ if (_unnestFields.contains(concatName)) {
+ for (Object inner : (Collection) value) {
+ if (inner instanceof Map) {
+ Map<String, Object> innerMap = (Map<String, Object>) inner;
+ flattenMap(concatName, innerMap, new ArrayList<>(innerMap.keySet()));
+ }
+ }
+ } else if (shallConvertToJson(collection)) {
+ try {
+ // convert the collection to JSON string
+ String jsonString = JsonFunctions.jsonFormat(collection);
+ map.put(field, jsonString);
+ } catch (JsonProcessingException e) {
+ throw new RuntimeException(
+ String.format("Caught exception while converting value to JSON string %s", value), e);
}
}
- } else if (isArray(value) && _unnestFields.contains(concatName)) {
- for (Object inner : (Object[]) value) {
- if (inner instanceof Map) {
- Map<String, Object> innerMap = (Map<String, Object>) inner;
- flattenMap(concatName, innerMap, new ArrayList<>(innerMap.keySet()));
+ } else if (isArray(value)) {
+ Object[] array = (Object[]) value;
+ if (_unnestFields.contains(concatName)) {
+ for (Object inner : (Object[]) value) {
+ if (inner instanceof Map) {
+ Map<String, Object> innerMap = (Map<String, Object>) inner;
+ flattenMap(concatName, innerMap, new ArrayList<>(innerMap.keySet()));
+ }
+ }
+ } else if (shallConvertToJson(array)) {
+ try {
+ // convert the array to JSON string
+ String jsonString = JsonFunctions.jsonFormat(array);
+ map.put(field, jsonString);
+ } catch (JsonProcessingException e) {
+ throw new RuntimeException(
+ String.format("Caught exception while converting value to JSON string %s", value), e);
}
}
}
}
}
+ private boolean shallConvertToJson(Object[] value) {
+ switch (_collectionToJsonMode) {
+ case ALL:
+ return true;
+ case NONE:
+ return false;
+ case NON_PRIMITIVE:
+ return !containPrimitives(value);
+ default:
+ throw new IllegalArgumentException(String.format("Unsupported collectionToJsonMode %s", _collectionToJsonMode));
+ }
+ }
+
+ private boolean shallConvertToJson(Collection value) {
Review comment:
I meant `value.toArray()` will create a copy.
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformer.java
##########
@@ -212,25 +234,65 @@ protected void flattenMap(GenericRow record, List<String> columns) {
}
}
flattenMap(record, mapColumns);
- } else if (value instanceof Collection && _unnestFields.contains(column)) {
- for (Object inner : (Collection) value) {
- if (inner instanceof Map) {
- Map<String, Object> innerMap = (Map<String, Object>) inner;
- flattenMap(column, innerMap, new ArrayList<>(innerMap.keySet()));
+ } else if (value instanceof Collection) {
+ Collection collection = (Collection) value;
+ if (_unnestFields.contains(column)) {
+ for (Object inner : collection) {
+ if (inner instanceof Map) {
+ Map<String, Object> innerMap = (Map<String, Object>) inner;
+ flattenMap(column, innerMap, new ArrayList<>(innerMap.keySet()));
+ }
+ }
+ } else if (shallConvertToJson(collection)) {
+ try {
+ // convert the collection to JSON string
+ String jsonString = JsonFunctions.jsonFormat(collection);
+ record.putValue(column, jsonString);
+ } catch (JsonProcessingException e) {
+ throw new RuntimeException(
+ String.format("Caught exception while converting value to JSON string %s", value), e);
}
}
- } else if (isArray(value) && _unnestFields.contains(column)) {
- for (Object inner : (Object[]) value) {
- if (inner instanceof Map) {
- Map<String, Object> innerMap = (Map<String, Object>) inner;
- flattenMap(column, innerMap, new ArrayList<>(innerMap.keySet()));
+ } else if (isArray(value)) {
+ Object[] array = (Object[]) value;
+ if (_unnestFields.contains(column)) {
+ for (Object inner : array) {
+ if (inner instanceof Map) {
+ Map<String, Object> innerMap = (Map<String, Object>) inner;
+ flattenMap(column, innerMap, new ArrayList<>(innerMap.keySet()));
+ }
+ }
+ } else if (shallConvertToJson(array)) {
+ try {
+ // convert the array to JSON string
+ String jsonString = JsonFunctions.jsonFormat(array);
+ record.putValue(column, jsonString);
+ } catch (JsonProcessingException e) {
+ throw new RuntimeException(
+ String.format("Caught exception while converting value to JSON string %s", value), e);
}
}
}
}
}
- static private boolean isArray(Object obj) {
+ private boolean containPrimitives(Object[] value) {
+ if (value.length == 0) {
+ return true;
+ }
+ Object element = value[0];
+ return !(element instanceof Map || element instanceof Collection || isArray(element));
+ }
+
+ private boolean containPrimitives(Collection value) {
+ if (value.isEmpty()) {
+ return true;
+ }
+ Object element = value.iterator().next();
+ return !(element instanceof Map || element instanceof Collection || isArray(element));
+ }
Review comment:
Could be inlined. But this makes the code slightly readable given the negation is used in `shallConvertToJson `
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformer.java
##########
@@ -78,17 +81,27 @@
*/
public class ComplexTypeTransformer implements RecordTransformer {
private static final String DEFAULT_DELIMITER = ".";
+ public static final ComplexTypeConfig.CollectionToJsonMode DEFAULT_COLLECTION_TO_JSON_MODE =
+ ComplexTypeConfig.CollectionToJsonMode.NON_PRIMITIVE;
private final List<String> _unnestFields;
private final String _delimiter;
+ private final ComplexTypeConfig.CollectionToJsonMode _collectionToJsonMode;
public ComplexTypeTransformer(TableConfig tableConfig) {
- this(parseUnnestFields(tableConfig), parseDelimiter(tableConfig));
+ this(parseUnnestFields(tableConfig), parseDelimiter(tableConfig), parseCollectionToJsonMode(tableConfig));
Review comment:
I feel it's not that bad. Usually I do what you suggest, but in this one there's some other state change in the constructor.
--
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.
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
[GitHub] [incubator-pinot] yupeng9 commented on a change in pull request #6918: Convert collections not unnested to json
Posted by GitBox <gi...@apache.org>.
yupeng9 commented on a change in pull request #6918:
URL: https://github.com/apache/incubator-pinot/pull/6918#discussion_r632813364
##########
File path: pinot-segment-local/src/test/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformerTest.java
##########
@@ -237,4 +238,76 @@ public void testUnnestCollection() {
next = itr.next();
Assert.assertEquals("v2", next.getValue("array.a"));
}
+
+ @Test
+ public void testConvertCollectionToString() {
+ // json convert inner collections
+ // {
+ // "array":[
+ // {
+ // "array1":[
+ // {
+ // "b":"v1"
+ // }
+ // ]
+ // }
+ // ]
+ // }
+ // is converted to
+ // [{
+ // "array.array1":"[
+ // {
+ // "b":"v1"
+ // }
+ // ]"
+ // }]
+ ComplexTypeTransformer transformer = new ComplexTypeTransformer(Arrays.asList("array"), ".");
+ GenericRow genericRow = new GenericRow();
+ Map<String, Object> map = new HashMap<>();
+ Object[] array1 = new Object[1];
+ array1[0] = ImmutableMap.of("b", "v1");
+ map.put("array1", array1);
+ Object[] array = new Object[1];
+ array[0] = map;
+ genericRow.putValue("array", array);
+ transformer.transform(genericRow);
+ Assert.assertNotNull(genericRow.getValue(GenericRow.MULTIPLE_RECORDS_KEY));
+ Collection<GenericRow> collection = (Collection<GenericRow>) genericRow.getValue(GenericRow.MULTIPLE_RECORDS_KEY);
+ GenericRow row = collection.iterator().next();
+ Assert.assertTrue(row.getValue("array.array1") instanceof String);
+
+ // primitive array not converted
Review comment:
coz Pinot natively support multi-value column. Also, per @Jackie-Jiang's suggestion, I'll add a mode so user can choose JSON serialize all collections or collections of non-primitive 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.
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
[GitHub] [incubator-pinot] codecov-commenter edited a comment on pull request #6918: Convert collections not unnested to json
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #6918:
URL: https://github.com/apache/incubator-pinot/pull/6918#issuecomment-840920229
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#6918](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3d9afa7) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/08ed07ce27cd56b341fa634fa1cad9e440f39f6e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (08ed07c) will **increase** coverage by `22.27%`.
> The diff coverage is `45.34%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #6918 +/- ##
=============================================
+ Coverage 43.20% 65.47% +22.27%
- Complexity 7 12 +5
=============================================
Files 1431 1431
Lines 70615 70681 +66
Branches 10220 10233 +13
=============================================
+ Hits 30506 46280 +15774
+ Misses 37502 21088 -16414
- Partials 2607 3313 +706
```
| Flag | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| integration | `?` | `?` | |
| unittests | `65.47% <45.34%> (?)` | `12.00 <0.00> (?)` | |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...ocal/recordtransformer/ComplexTypeTransformer.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWNvcmR0cmFuc2Zvcm1lci9Db21wbGV4VHlwZVRyYW5zZm9ybWVyLmphdmE=) | `56.28% <41.97%> (+55.45%)` | `0.00 <0.00> (ø)` | |
| [.../spi/config/table/ingestion/ComplexTypeConfig.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL2luZ2VzdGlvbi9Db21wbGV4VHlwZUNvbmZpZy5qYXZh) | `100.00% <100.00%> (+100.00%)` | `0.00 <0.00> (ø)` | |
| [...a/org/apache/pinot/minion/metrics/MinionMeter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJRdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [.../apache/pinot/minion/metrics/MinionQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25RdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [...pache/pinot/common/utils/grpc/GrpcQueryClient.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvZ3JwYy9HcnBjUXVlcnlDbGllbnQuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [...pinot/minion/exception/TaskCancelledException.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhjZXB0aW9uL1Rhc2tDYW5jZWxsZWRFeGNlcHRpb24uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [...t/core/startree/plan/StarTreeDocIdSetPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlRG9jSWRTZXRQbGFuTm9kZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [.../core/startree/plan/StarTreeTransformPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlVHJhbnNmb3JtUGxhbk5vZGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [...core/startree/plan/StarTreeProjectionPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlUHJvamVjdGlvblBsYW5Ob2RlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| ... and [1069 more](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [08ed07c...3d9afa7](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
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
[GitHub] [incubator-pinot] yupeng9 commented on a change in pull request #6918: Convert collections not unnested to json
Posted by GitBox <gi...@apache.org>.
yupeng9 commented on a change in pull request #6918:
URL: https://github.com/apache/incubator-pinot/pull/6918#discussion_r632867121
##########
File path: pinot-segment-local/src/test/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformerTest.java
##########
@@ -237,4 +238,76 @@ public void testUnnestCollection() {
next = itr.next();
Assert.assertEquals("v2", next.getValue("array.a"));
}
+
+ @Test
+ public void testConvertCollectionToString() {
+ // json convert inner collections
+ // {
+ // "array":[
+ // {
+ // "array1":[
+ // {
+ // "b":"v1"
+ // }
+ // ]
+ // }
+ // ]
+ // }
+ // is converted to
+ // [{
+ // "array.array1":"[
+ // {
+ // "b":"v1"
+ // }
+ // ]"
+ // }]
+ ComplexTypeTransformer transformer = new ComplexTypeTransformer(Arrays.asList("array"), ".");
+ GenericRow genericRow = new GenericRow();
+ Map<String, Object> map = new HashMap<>();
+ Object[] array1 = new Object[1];
+ array1[0] = ImmutableMap.of("b", "v1");
+ map.put("array1", array1);
+ Object[] array = new Object[1];
+ array[0] = map;
+ genericRow.putValue("array", array);
+ transformer.transform(genericRow);
+ Assert.assertNotNull(genericRow.getValue(GenericRow.MULTIPLE_RECORDS_KEY));
+ Collection<GenericRow> collection = (Collection<GenericRow>) genericRow.getValue(GenericRow.MULTIPLE_RECORDS_KEY);
+ GenericRow row = collection.iterator().next();
+ Assert.assertTrue(row.getValue("array.array1") instanceof String);
+
+ // primitive array not converted
Review comment:
I see. The disconnect here is that there is no nested collection. All nested collection will be flattened to become top-level, and we have two choices on them: collections of pritimive values stay the same while collections of non-primitives convert to String
--
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.
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
[GitHub] [incubator-pinot] yupeng9 merged pull request #6918: Convert collections not unnested to json
Posted by GitBox <gi...@apache.org>.
yupeng9 merged pull request #6918:
URL: https://github.com/apache/incubator-pinot/pull/6918
--
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.
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
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6918: Convert collections not unnested to json
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6918:
URL: https://github.com/apache/incubator-pinot/pull/6918#discussion_r633942532
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformer.java
##########
@@ -78,17 +81,27 @@
*/
public class ComplexTypeTransformer implements RecordTransformer {
private static final String DEFAULT_DELIMITER = ".";
+ public static final ComplexTypeConfig.CollectionToJsonMode DEFAULT_COLLECTION_TO_JSON_MODE =
+ ComplexTypeConfig.CollectionToJsonMode.NON_PRIMITIVE;
private final List<String> _unnestFields;
private final String _delimiter;
+ private final ComplexTypeConfig.CollectionToJsonMode _collectionToJsonMode;
public ComplexTypeTransformer(TableConfig tableConfig) {
- this(parseUnnestFields(tableConfig), parseDelimiter(tableConfig));
+ this(parseUnnestFields(tableConfig), parseDelimiter(tableConfig), parseCollectionToJsonMode(tableConfig));
Review comment:
Suggest directly parsing table config here instead of reusing the other constructor. The parse functions seem very verbose to me
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformer.java
##########
@@ -212,25 +234,65 @@ protected void flattenMap(GenericRow record, List<String> columns) {
}
}
flattenMap(record, mapColumns);
- } else if (value instanceof Collection && _unnestFields.contains(column)) {
- for (Object inner : (Collection) value) {
- if (inner instanceof Map) {
- Map<String, Object> innerMap = (Map<String, Object>) inner;
- flattenMap(column, innerMap, new ArrayList<>(innerMap.keySet()));
+ } else if (value instanceof Collection) {
+ Collection collection = (Collection) value;
+ if (_unnestFields.contains(column)) {
+ for (Object inner : collection) {
+ if (inner instanceof Map) {
+ Map<String, Object> innerMap = (Map<String, Object>) inner;
+ flattenMap(column, innerMap, new ArrayList<>(innerMap.keySet()));
+ }
+ }
+ } else if (shallConvertToJson(collection)) {
+ try {
+ // convert the collection to JSON string
+ String jsonString = JsonFunctions.jsonFormat(collection);
+ record.putValue(column, jsonString);
+ } catch (JsonProcessingException e) {
+ throw new RuntimeException(
+ String.format("Caught exception while converting value to JSON string %s", value), e);
}
}
- } else if (isArray(value) && _unnestFields.contains(column)) {
- for (Object inner : (Object[]) value) {
- if (inner instanceof Map) {
- Map<String, Object> innerMap = (Map<String, Object>) inner;
- flattenMap(column, innerMap, new ArrayList<>(innerMap.keySet()));
+ } else if (isArray(value)) {
+ Object[] array = (Object[]) value;
+ if (_unnestFields.contains(column)) {
+ for (Object inner : array) {
+ if (inner instanceof Map) {
+ Map<String, Object> innerMap = (Map<String, Object>) inner;
+ flattenMap(column, innerMap, new ArrayList<>(innerMap.keySet()));
+ }
+ }
+ } else if (shallConvertToJson(array)) {
+ try {
+ // convert the array to JSON string
+ String jsonString = JsonFunctions.jsonFormat(array);
+ record.putValue(column, jsonString);
+ } catch (JsonProcessingException e) {
+ throw new RuntimeException(
+ String.format("Caught exception while converting value to JSON string %s", value), e);
}
}
}
}
}
- static private boolean isArray(Object obj) {
+ private boolean containPrimitives(Object[] value) {
Review comment:
We only check the first value. Please add some javadoc describing the behavior
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformer.java
##########
@@ -212,25 +234,65 @@ protected void flattenMap(GenericRow record, List<String> columns) {
}
}
flattenMap(record, mapColumns);
- } else if (value instanceof Collection && _unnestFields.contains(column)) {
- for (Object inner : (Collection) value) {
- if (inner instanceof Map) {
- Map<String, Object> innerMap = (Map<String, Object>) inner;
- flattenMap(column, innerMap, new ArrayList<>(innerMap.keySet()));
+ } else if (value instanceof Collection) {
+ Collection collection = (Collection) value;
+ if (_unnestFields.contains(column)) {
+ for (Object inner : collection) {
+ if (inner instanceof Map) {
+ Map<String, Object> innerMap = (Map<String, Object>) inner;
+ flattenMap(column, innerMap, new ArrayList<>(innerMap.keySet()));
+ }
+ }
+ } else if (shallConvertToJson(collection)) {
+ try {
+ // convert the collection to JSON string
+ String jsonString = JsonFunctions.jsonFormat(collection);
+ record.putValue(column, jsonString);
+ } catch (JsonProcessingException e) {
+ throw new RuntimeException(
+ String.format("Caught exception while converting value to JSON string %s", value), e);
}
}
- } else if (isArray(value) && _unnestFields.contains(column)) {
- for (Object inner : (Object[]) value) {
- if (inner instanceof Map) {
- Map<String, Object> innerMap = (Map<String, Object>) inner;
- flattenMap(column, innerMap, new ArrayList<>(innerMap.keySet()));
+ } else if (isArray(value)) {
+ Object[] array = (Object[]) value;
+ if (_unnestFields.contains(column)) {
+ for (Object inner : array) {
+ if (inner instanceof Map) {
+ Map<String, Object> innerMap = (Map<String, Object>) inner;
+ flattenMap(column, innerMap, new ArrayList<>(innerMap.keySet()));
+ }
+ }
+ } else if (shallConvertToJson(array)) {
+ try {
+ // convert the array to JSON string
+ String jsonString = JsonFunctions.jsonFormat(array);
+ record.putValue(column, jsonString);
+ } catch (JsonProcessingException e) {
+ throw new RuntimeException(
+ String.format("Caught exception while converting value to JSON string %s", value), e);
}
}
}
}
}
- static private boolean isArray(Object obj) {
+ private boolean containPrimitives(Object[] value) {
+ if (value.length == 0) {
+ return true;
+ }
+ Object element = value[0];
+ return !(element instanceof Map || element instanceof Collection || isArray(element));
+ }
+
+ private boolean containPrimitives(Collection value) {
+ if (value.isEmpty()) {
+ return true;
+ }
+ Object element = value.iterator().next();
+ return !(element instanceof Map || element instanceof Collection || isArray(element));
+ }
+
+ static boolean isArray(Object obj) {
Review comment:
Is this visible to test? Add `private` or annotation
--
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.
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
[GitHub] [incubator-pinot] codecov-commenter edited a comment on pull request #6918: Convert collections not unnested to json
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #6918:
URL: https://github.com/apache/incubator-pinot/pull/6918#issuecomment-840920229
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#6918](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1039a7b) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/8102e4361291860c7dff3d4558a969eed0b44d2d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8102e43) will **decrease** coverage by `0.13%`.
> The diff coverage is `37.70%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #6918 +/- ##
============================================
- Coverage 73.53% 73.40% -0.14%
Complexity 12 12
============================================
Files 1432 1432
Lines 70591 70635 +44
Branches 10205 10215 +10
============================================
- Hits 51908 51847 -61
- Misses 15250 15345 +95
- Partials 3433 3443 +10
```
| Flag | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| integration | `43.09% <0.00%> (-0.06%)` | `7.00 <0.00> (ø)` | |
| unittests | `65.42% <37.70%> (-0.03%)` | `12.00 <0.00> (ø)` | |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...ocal/recordtransformer/ComplexTypeTransformer.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWNvcmR0cmFuc2Zvcm1lci9Db21wbGV4VHlwZVRyYW5zZm9ybWVyLmphdmE=) | `55.82% <37.70%> (-9.72%)` | `0.00 <0.00> (ø)` | |
| [...data/manager/realtime/SegmentCommitterFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvU2VnbWVudENvbW1pdHRlckZhY3RvcnkuamF2YQ==) | `64.70% <0.00%> (-35.30%)` | `0.00% <0.00%> (ø%)` | |
| [.../impl/dictionary/LongOffHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvTG9uZ09mZkhlYXBNdXRhYmxlRGljdGlvbmFyeS5qYXZh) | `67.02% <0.00%> (-10.64%)` | `0.00% <0.00%> (ø%)` | |
| [...altime/ServerSegmentCompletionProtocolHandler.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VydmVyL3JlYWx0aW1lL1NlcnZlclNlZ21lbnRDb21wbGV0aW9uUHJvdG9jb2xIYW5kbGVyLmphdmE=) | `48.54% <0.00%> (-8.74%)` | `0.00% <0.00%> (ø%)` | |
| [...er/api/resources/LLCSegmentCompletionHandlers.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL0xMQ1NlZ21lbnRDb21wbGV0aW9uSGFuZGxlcnMuamF2YQ==) | `51.16% <0.00%> (-8.38%)` | `0.00% <0.00%> (ø%)` | |
| [...e/data/manager/realtime/SplitSegmentCommitter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvU3BsaXRTZWdtZW50Q29tbWl0dGVyLmphdmE=) | `53.84% <0.00%> (-7.70%)` | `0.00% <0.00%> (ø%)` | |
| [.../segment/local/segment/memory/PinotDataBuffer.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L21lbW9yeS9QaW5vdERhdGFCdWZmZXIuamF2YQ==) | `79.16% <0.00%> (-7.50%)` | `0.00% <0.00%> (ø%)` | |
| [...er/starter/helix/SegmentMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9TZWdtZW50TWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `69.33% <0.00%> (-6.67%)` | `0.00% <0.00%> (ø%)` | |
| [...e/impl/dictionary/LongOnHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvTG9uZ09uSGVhcE11dGFibGVEaWN0aW9uYXJ5LmphdmE=) | `63.85% <0.00%> (-6.03%)` | `0.00% <0.00%> (ø%)` | |
| [.../impl/dictionary/FloatOnHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvRmxvYXRPbkhlYXBNdXRhYmxlRGljdGlvbmFyeS5qYXZh) | `69.87% <0.00%> (-6.03%)` | `0.00% <0.00%> (ø%)` | |
| ... and [16 more](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [8102e43...1039a7b](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
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
[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #6918: Convert collections not unnested to json
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6918:
URL: https://github.com/apache/incubator-pinot/pull/6918#discussion_r632814695
##########
File path: pinot-segment-local/src/test/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformerTest.java
##########
@@ -237,4 +238,76 @@ public void testUnnestCollection() {
next = itr.next();
Assert.assertEquals("v2", next.getValue("array.a"));
}
+
+ @Test
+ public void testConvertCollectionToString() {
+ // json convert inner collections
+ // {
+ // "array":[
+ // {
+ // "array1":[
+ // {
+ // "b":"v1"
+ // }
+ // ]
+ // }
+ // ]
+ // }
+ // is converted to
+ // [{
+ // "array.array1":"[
+ // {
+ // "b":"v1"
+ // }
+ // ]"
+ // }]
+ ComplexTypeTransformer transformer = new ComplexTypeTransformer(Arrays.asList("array"), ".");
+ GenericRow genericRow = new GenericRow();
+ Map<String, Object> map = new HashMap<>();
+ Object[] array1 = new Object[1];
+ array1[0] = ImmutableMap.of("b", "v1");
+ map.put("array1", array1);
+ Object[] array = new Object[1];
+ array[0] = map;
+ genericRow.putValue("array", array);
+ transformer.transform(genericRow);
+ Assert.assertNotNull(genericRow.getValue(GenericRow.MULTIPLE_RECORDS_KEY));
+ Collection<GenericRow> collection = (Collection<GenericRow>) genericRow.getValue(GenericRow.MULTIPLE_RECORDS_KEY);
+ GenericRow row = collection.iterator().next();
+ Assert.assertTrue(row.getValue("array.array1") instanceof String);
+
+ // primitive array not converted
Review comment:
Yes but native multi-value column support for primitives should not be confused with array/collection inside a JSON object right even though the array/collection inside a JSON object could be of primitives. These two are completely different
--
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.
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
[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #6918: Convert collections not unnested to json
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6918:
URL: https://github.com/apache/incubator-pinot/pull/6918#discussion_r632854615
##########
File path: pinot-segment-local/src/test/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformerTest.java
##########
@@ -237,4 +238,76 @@ public void testUnnestCollection() {
next = itr.next();
Assert.assertEquals("v2", next.getValue("array.a"));
}
+
+ @Test
+ public void testConvertCollectionToString() {
+ // json convert inner collections
+ // {
+ // "array":[
+ // {
+ // "array1":[
+ // {
+ // "b":"v1"
+ // }
+ // ]
+ // }
+ // ]
+ // }
+ // is converted to
+ // [{
+ // "array.array1":"[
+ // {
+ // "b":"v1"
+ // }
+ // ]"
+ // }]
+ ComplexTypeTransformer transformer = new ComplexTypeTransformer(Arrays.asList("array"), ".");
+ GenericRow genericRow = new GenericRow();
+ Map<String, Object> map = new HashMap<>();
+ Object[] array1 = new Object[1];
+ array1[0] = ImmutableMap.of("b", "v1");
+ map.put("array1", array1);
+ Object[] array = new Object[1];
+ array[0] = map;
+ genericRow.putValue("array", array);
+ transformer.transform(genericRow);
+ Assert.assertNotNull(genericRow.getValue(GenericRow.MULTIPLE_RECORDS_KEY));
+ Collection<GenericRow> collection = (Collection<GenericRow>) genericRow.getValue(GenericRow.MULTIPLE_RECORDS_KEY);
+ GenericRow row = collection.iterator().next();
+ Assert.assertTrue(row.getValue("array.array1") instanceof String);
+
+ // primitive array not converted
Review comment:
Here we are talking about array inside a nested object which doesn't get the same storage format and treatment (at least in the current design) as an MV column in the schema.
If I have something like
```
{
name
age
salary
array of organizationIds (primitive integers)
array of my addresses (another nested struct)
}
```
Even though array of organizationIds is an array of primitive INTs, we currently don't store it in the same way as we would have done if it were an independent MV column in the schema. So the collection unnesting rule we have in design should be applicable to both collections in the same way. Similarly, I think if the user chooses to not unnest them then the behavior (json string serialization rule will apply) should be same for both collections regardless of whether one is for primitives and other is not.
--
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.
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
[GitHub] [incubator-pinot] Jackie-Jiang commented on pull request #6918: Convert collections not unnested to json
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on pull request #6918:
URL: https://github.com/apache/incubator-pinot/pull/6918#issuecomment-841423135
Maybe add a enum config and have `NONE`, `NON_PRIMITIVE` and `ALL`? Default can be `NON_PRIMITIVE` if that is the most common case
--
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.
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
[GitHub] [incubator-pinot] Jackie-Jiang commented on pull request #6918: Convert collections not unnested to json
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on pull request #6918:
URL: https://github.com/apache/incubator-pinot/pull/6918#issuecomment-840915175
This might not always be the desired behavior. Other transformers might expect collection as the input.
This can also be done by using `toJsonString()` in `ExpressionTransformer`
--
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.
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
[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #6918: Convert collections not unnested to json
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6918:
URL: https://github.com/apache/incubator-pinot/pull/6918#discussion_r632812214
##########
File path: pinot-segment-local/src/test/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformerTest.java
##########
@@ -237,4 +238,76 @@ public void testUnnestCollection() {
next = itr.next();
Assert.assertEquals("v2", next.getValue("array.a"));
}
+
+ @Test
+ public void testConvertCollectionToString() {
+ // json convert inner collections
+ // {
+ // "array":[
+ // {
+ // "array1":[
+ // {
+ // "b":"v1"
+ // }
+ // ]
+ // }
+ // ]
+ // }
+ // is converted to
+ // [{
+ // "array.array1":"[
+ // {
+ // "b":"v1"
+ // }
+ // ]"
+ // }]
+ ComplexTypeTransformer transformer = new ComplexTypeTransformer(Arrays.asList("array"), ".");
+ GenericRow genericRow = new GenericRow();
+ Map<String, Object> map = new HashMap<>();
+ Object[] array1 = new Object[1];
+ array1[0] = ImmutableMap.of("b", "v1");
+ map.put("array1", array1);
+ Object[] array = new Object[1];
+ array[0] = map;
+ genericRow.putValue("array", array);
+ transformer.transform(genericRow);
+ Assert.assertNotNull(genericRow.getValue(GenericRow.MULTIPLE_RECORDS_KEY));
+ Collection<GenericRow> collection = (Collection<GenericRow>) genericRow.getValue(GenericRow.MULTIPLE_RECORDS_KEY);
+ GenericRow row = collection.iterator().next();
+ Assert.assertTrue(row.getValue("array.array1") instanceof String);
+
+ // primitive array not converted
Review comment:
The rules aren't clear. As per the design doc (in the Collection handling section), if collection unnesting rule is not applied on a collection, then it will use JSON serialization rule right?
So why are we differentiating now on primitive collection v/s collection of non scalars for the collections that are not unnested?
--
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.
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
[GitHub] [incubator-pinot] codecov-commenter commented on pull request #6918: Convert collections not unnested to json
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #6918:
URL: https://github.com/apache/incubator-pinot/pull/6918#issuecomment-840920229
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#6918](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1039a7b) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/8102e4361291860c7dff3d4558a969eed0b44d2d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8102e43) will **decrease** coverage by `8.11%`.
> The diff coverage is `37.70%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #6918 +/- ##
============================================
- Coverage 73.53% 65.42% -8.12%
Complexity 12 12
============================================
Files 1432 1432
Lines 70591 70635 +44
Branches 10205 10215 +10
============================================
- Hits 51908 46210 -5698
- Misses 15250 21118 +5868
+ Partials 3433 3307 -126
```
| Flag | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| integration | `?` | `?` | |
| unittests | `65.42% <37.70%> (-0.03%)` | `12.00 <0.00> (ø)` | |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...ocal/recordtransformer/ComplexTypeTransformer.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWNvcmR0cmFuc2Zvcm1lci9Db21wbGV4VHlwZVRyYW5zZm9ybWVyLmphdmE=) | `55.82% <37.70%> (-9.72%)` | `0.00 <0.00> (ø)` | |
| [...a/org/apache/pinot/minion/metrics/MinionMeter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJRdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [.../apache/pinot/minion/metrics/MinionQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25RdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [...pache/pinot/common/utils/grpc/GrpcQueryClient.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvZ3JwYy9HcnBjUXVlcnlDbGllbnQuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [...pinot/minion/exception/TaskCancelledException.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhjZXB0aW9uL1Rhc2tDYW5jZWxsZWRFeGNlcHRpb24uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [...t/core/startree/plan/StarTreeDocIdSetPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlRG9jSWRTZXRQbGFuTm9kZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [.../core/startree/plan/StarTreeTransformPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlVHJhbnNmb3JtUGxhbk5vZGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [...core/startree/plan/StarTreeProjectionPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlUHJvamVjdGlvblBsYW5Ob2RlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [...t/minion/executor/MinionTaskZkMetadataManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhlY3V0b3IvTWluaW9uVGFza1prTWV0YWRhdGFNYW5hZ2VyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| ... and [336 more](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [8102e43...1039a7b](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
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
[GitHub] [incubator-pinot] yupeng9 commented on pull request #6918: Convert collections not unnested to json
Posted by GitBox <gi...@apache.org>.
yupeng9 commented on pull request #6918:
URL: https://github.com/apache/incubator-pinot/pull/6918#issuecomment-841425693
> Maybe add a enum config and have `NONE`, `NON_PRIMITIVE` and `ALL`? Default can be `NON_PRIMITIVE` if that is the most common case
Sounds good to me.
--
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.
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
[GitHub] [incubator-pinot] yupeng9 commented on a change in pull request #6918: Convert collections not unnested to json
Posted by GitBox <gi...@apache.org>.
yupeng9 commented on a change in pull request #6918:
URL: https://github.com/apache/incubator-pinot/pull/6918#discussion_r632827760
##########
File path: pinot-segment-local/src/test/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformerTest.java
##########
@@ -237,4 +238,76 @@ public void testUnnestCollection() {
next = itr.next();
Assert.assertEquals("v2", next.getValue("array.a"));
}
+
+ @Test
+ public void testConvertCollectionToString() {
+ // json convert inner collections
+ // {
+ // "array":[
+ // {
+ // "array1":[
+ // {
+ // "b":"v1"
+ // }
+ // ]
+ // }
+ // ]
+ // }
+ // is converted to
+ // [{
+ // "array.array1":"[
+ // {
+ // "b":"v1"
+ // }
+ // ]"
+ // }]
+ ComplexTypeTransformer transformer = new ComplexTypeTransformer(Arrays.asList("array"), ".");
+ GenericRow genericRow = new GenericRow();
+ Map<String, Object> map = new HashMap<>();
+ Object[] array1 = new Object[1];
+ array1[0] = ImmutableMap.of("b", "v1");
+ map.put("array1", array1);
+ Object[] array = new Object[1];
+ array[0] = map;
+ genericRow.putValue("array", array);
+ transformer.transform(genericRow);
+ Assert.assertNotNull(genericRow.getValue(GenericRow.MULTIPLE_RECORDS_KEY));
+ Collection<GenericRow> collection = (Collection<GenericRow>) genericRow.getValue(GenericRow.MULTIPLE_RECORDS_KEY);
+ GenericRow row = collection.iterator().next();
+ Assert.assertTrue(row.getValue("array.array1") instanceof String);
+
+ // primitive array not converted
Review comment:
then how is an array of integers in Avro ingested into Pinot, today?
--
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.
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
[GitHub] [incubator-pinot] amrishlal commented on a change in pull request #6918: Convert collections not unnested to json
Posted by GitBox <gi...@apache.org>.
amrishlal commented on a change in pull request #6918:
URL: https://github.com/apache/incubator-pinot/pull/6918#discussion_r633788767
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformer.java
##########
@@ -256,23 +318,73 @@ private void flattenMap(String path, Map<String, Object> map, Collection<String>
flattenMap(concatName, map, innerMapFields);
}
} else if (value instanceof Collection && _unnestFields.contains(concatName)) {
- for (Object inner : (Collection) value) {
- if (inner instanceof Map) {
- Map<String, Object> innerMap = (Map<String, Object>) inner;
- flattenMap(concatName, innerMap, new ArrayList<>(innerMap.keySet()));
+ Collection collection = (Collection) value;
+ if (_unnestFields.contains(concatName)) {
+ for (Object inner : (Collection) value) {
+ if (inner instanceof Map) {
+ Map<String, Object> innerMap = (Map<String, Object>) inner;
+ flattenMap(concatName, innerMap, new ArrayList<>(innerMap.keySet()));
+ }
+ }
+ } else if (shallConvertToJson(collection)) {
+ try {
+ // convert the collection to JSON string
+ String jsonString = JsonFunctions.jsonFormat(collection);
+ map.put(field, jsonString);
+ } catch (JsonProcessingException e) {
+ throw new RuntimeException(
+ String.format("Caught exception while converting value to JSON string %s", value), e);
}
}
- } else if (isArray(value) && _unnestFields.contains(concatName)) {
- for (Object inner : (Object[]) value) {
- if (inner instanceof Map) {
- Map<String, Object> innerMap = (Map<String, Object>) inner;
- flattenMap(concatName, innerMap, new ArrayList<>(innerMap.keySet()));
+ } else if (isArray(value)) {
+ Object[] array = (Object[]) value;
+ if (_unnestFields.contains(concatName)) {
+ for (Object inner : (Object[]) value) {
+ if (inner instanceof Map) {
+ Map<String, Object> innerMap = (Map<String, Object>) inner;
+ flattenMap(concatName, innerMap, new ArrayList<>(innerMap.keySet()));
+ }
+ }
+ } else if (shallConvertToJson(array)) {
+ try {
+ // convert the array to JSON string
+ String jsonString = JsonFunctions.jsonFormat(array);
+ map.put(field, jsonString);
+ } catch (JsonProcessingException e) {
+ throw new RuntimeException(
+ String.format("Caught exception while converting value to JSON string %s", value), e);
}
}
}
}
}
+ private boolean shallConvertToJson(Object[] value) {
+ switch (_collectionToJsonMode) {
+ case ALL:
+ return true;
+ case NONE:
+ return false;
+ case NON_PRIMITIVE:
+ return !containPrimitives(value);
+ default:
+ throw new IllegalArgumentException(String.format("Unsupported collectionToJsonMode %s", _collectionToJsonMode));
+ }
+ }
+
+ private boolean shallConvertToJson(Collection value) {
Review comment:
Instead of duplicating code, just make the caller do `shallConvertToJson(value.toArray());`
--
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.
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
[GitHub] [incubator-pinot] yupeng9 commented on pull request #6918: Convert collections not unnested to json
Posted by GitBox <gi...@apache.org>.
yupeng9 commented on pull request #6918:
URL: https://github.com/apache/incubator-pinot/pull/6918#issuecomment-841007940
> This might not always be the desired behavior. Other transformers might expect collection as the input.
> This can also be done by using `toJsonString()` in `ExpressionTransformer`
True. But this is convenient for some cases that users do not need to enumerate other collections. i.e. users need to only list the collections to unnest.
How about we add a config `convertCollectionsToJson` with a default value of `false`?
--
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.
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
[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #6918: Convert collections not unnested to json
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6918:
URL: https://github.com/apache/incubator-pinot/pull/6918#discussion_r632813843
##########
File path: pinot-segment-local/src/test/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformerTest.java
##########
@@ -237,4 +238,76 @@ public void testUnnestCollection() {
next = itr.next();
Assert.assertEquals("v2", next.getValue("array.a"));
}
+
+ @Test
+ public void testConvertCollectionToString() {
Review comment:
Can we please add a test where in a JSON object, one collection is unnested and another parallel collection at the same level in the root is not unnested ? I am assuming the one that is not unnested will still be repeated as many times based on the repetition level of the unnested collection.
--
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.
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
[GitHub] [incubator-pinot] codecov-commenter edited a comment on pull request #6918: Convert collections not unnested to json
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #6918:
URL: https://github.com/apache/incubator-pinot/pull/6918#issuecomment-840920229
--
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.
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
[GitHub] [incubator-pinot] amrishlal commented on a change in pull request #6918: Convert collections not unnested to json
Posted by GitBox <gi...@apache.org>.
amrishlal commented on a change in pull request #6918:
URL: https://github.com/apache/incubator-pinot/pull/6918#discussion_r633917206
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformer.java
##########
@@ -212,25 +234,65 @@ protected void flattenMap(GenericRow record, List<String> columns) {
}
}
flattenMap(record, mapColumns);
- } else if (value instanceof Collection && _unnestFields.contains(column)) {
- for (Object inner : (Collection) value) {
- if (inner instanceof Map) {
- Map<String, Object> innerMap = (Map<String, Object>) inner;
- flattenMap(column, innerMap, new ArrayList<>(innerMap.keySet()));
+ } else if (value instanceof Collection) {
+ Collection collection = (Collection) value;
+ if (_unnestFields.contains(column)) {
+ for (Object inner : collection) {
+ if (inner instanceof Map) {
+ Map<String, Object> innerMap = (Map<String, Object>) inner;
+ flattenMap(column, innerMap, new ArrayList<>(innerMap.keySet()));
+ }
+ }
+ } else if (shallConvertToJson(collection)) {
+ try {
+ // convert the collection to JSON string
+ String jsonString = JsonFunctions.jsonFormat(collection);
+ record.putValue(column, jsonString);
+ } catch (JsonProcessingException e) {
+ throw new RuntimeException(
+ String.format("Caught exception while converting value to JSON string %s", value), e);
}
}
- } else if (isArray(value) && _unnestFields.contains(column)) {
- for (Object inner : (Object[]) value) {
- if (inner instanceof Map) {
- Map<String, Object> innerMap = (Map<String, Object>) inner;
- flattenMap(column, innerMap, new ArrayList<>(innerMap.keySet()));
+ } else if (isArray(value)) {
+ Object[] array = (Object[]) value;
+ if (_unnestFields.contains(column)) {
+ for (Object inner : array) {
+ if (inner instanceof Map) {
+ Map<String, Object> innerMap = (Map<String, Object>) inner;
+ flattenMap(column, innerMap, new ArrayList<>(innerMap.keySet()));
+ }
+ }
+ } else if (shallConvertToJson(array)) {
+ try {
+ // convert the array to JSON string
+ String jsonString = JsonFunctions.jsonFormat(array);
+ record.putValue(column, jsonString);
+ } catch (JsonProcessingException e) {
+ throw new RuntimeException(
+ String.format("Caught exception while converting value to JSON string %s", value), e);
}
}
}
}
}
- static private boolean isArray(Object obj) {
+ private boolean containPrimitives(Object[] value) {
+ if (value.length == 0) {
+ return true;
+ }
+ Object element = value[0];
+ return !(element instanceof Map || element instanceof Collection || isArray(element));
+ }
+
+ private boolean containPrimitives(Collection value) {
+ if (value.isEmpty()) {
+ return true;
+ }
+ Object element = value.iterator().next();
+ return !(element instanceof Map || element instanceof Collection || isArray(element));
+ }
Review comment:
It seems like it should be possible to combine these two functions as well or merge them with `shallConvertToJson`.
--
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.
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
[GitHub] [incubator-pinot] amrishlal commented on a change in pull request #6918: Convert collections not unnested to json
Posted by GitBox <gi...@apache.org>.
amrishlal commented on a change in pull request #6918:
URL: https://github.com/apache/incubator-pinot/pull/6918#discussion_r633912560
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformer.java
##########
@@ -256,23 +318,73 @@ private void flattenMap(String path, Map<String, Object> map, Collection<String>
flattenMap(concatName, map, innerMapFields);
}
} else if (value instanceof Collection && _unnestFields.contains(concatName)) {
- for (Object inner : (Collection) value) {
- if (inner instanceof Map) {
- Map<String, Object> innerMap = (Map<String, Object>) inner;
- flattenMap(concatName, innerMap, new ArrayList<>(innerMap.keySet()));
+ Collection collection = (Collection) value;
+ if (_unnestFields.contains(concatName)) {
+ for (Object inner : (Collection) value) {
+ if (inner instanceof Map) {
+ Map<String, Object> innerMap = (Map<String, Object>) inner;
+ flattenMap(concatName, innerMap, new ArrayList<>(innerMap.keySet()));
+ }
+ }
+ } else if (shallConvertToJson(collection)) {
+ try {
+ // convert the collection to JSON string
+ String jsonString = JsonFunctions.jsonFormat(collection);
+ map.put(field, jsonString);
+ } catch (JsonProcessingException e) {
+ throw new RuntimeException(
+ String.format("Caught exception while converting value to JSON string %s", value), e);
}
}
- } else if (isArray(value) && _unnestFields.contains(concatName)) {
- for (Object inner : (Object[]) value) {
- if (inner instanceof Map) {
- Map<String, Object> innerMap = (Map<String, Object>) inner;
- flattenMap(concatName, innerMap, new ArrayList<>(innerMap.keySet()));
+ } else if (isArray(value)) {
+ Object[] array = (Object[]) value;
+ if (_unnestFields.contains(concatName)) {
+ for (Object inner : (Object[]) value) {
+ if (inner instanceof Map) {
+ Map<String, Object> innerMap = (Map<String, Object>) inner;
+ flattenMap(concatName, innerMap, new ArrayList<>(innerMap.keySet()));
+ }
+ }
+ } else if (shallConvertToJson(array)) {
+ try {
+ // convert the array to JSON string
+ String jsonString = JsonFunctions.jsonFormat(array);
+ map.put(field, jsonString);
+ } catch (JsonProcessingException e) {
+ throw new RuntimeException(
+ String.format("Caught exception while converting value to JSON string %s", value), e);
}
}
}
}
}
+ private boolean shallConvertToJson(Object[] value) {
+ switch (_collectionToJsonMode) {
+ case ALL:
+ return true;
+ case NONE:
+ return false;
+ case NON_PRIMITIVE:
+ return !containPrimitives(value);
+ default:
+ throw new IllegalArgumentException(String.format("Unsupported collectionToJsonMode %s", _collectionToJsonMode));
+ }
+ }
+
+ private boolean shallConvertToJson(Collection value) {
Review comment:
Hmm, Seems like this will work and not make a copy?
```
private <T> boolean shallConvertToJson(T value) {
switch (_collectionToJsonMode) {
case ALL:
return true;
case NONE:
return false;
case NON_PRIMITIVE:
return !containPrimitives(value);
default:
throw new IllegalArgumentException(String.format("Unsupported collectionToJsonMode %s", _collectionToJsonMode));
}
}
```
--
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.
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
[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #6918: Convert collections not unnested to json
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6918:
URL: https://github.com/apache/incubator-pinot/pull/6918#discussion_r632812214
##########
File path: pinot-segment-local/src/test/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformerTest.java
##########
@@ -237,4 +238,76 @@ public void testUnnestCollection() {
next = itr.next();
Assert.assertEquals("v2", next.getValue("array.a"));
}
+
+ @Test
+ public void testConvertCollectionToString() {
+ // json convert inner collections
+ // {
+ // "array":[
+ // {
+ // "array1":[
+ // {
+ // "b":"v1"
+ // }
+ // ]
+ // }
+ // ]
+ // }
+ // is converted to
+ // [{
+ // "array.array1":"[
+ // {
+ // "b":"v1"
+ // }
+ // ]"
+ // }]
+ ComplexTypeTransformer transformer = new ComplexTypeTransformer(Arrays.asList("array"), ".");
+ GenericRow genericRow = new GenericRow();
+ Map<String, Object> map = new HashMap<>();
+ Object[] array1 = new Object[1];
+ array1[0] = ImmutableMap.of("b", "v1");
+ map.put("array1", array1);
+ Object[] array = new Object[1];
+ array[0] = map;
+ genericRow.putValue("array", array);
+ transformer.transform(genericRow);
+ Assert.assertNotNull(genericRow.getValue(GenericRow.MULTIPLE_RECORDS_KEY));
+ Collection<GenericRow> collection = (Collection<GenericRow>) genericRow.getValue(GenericRow.MULTIPLE_RECORDS_KEY);
+ GenericRow row = collection.iterator().next();
+ Assert.assertTrue(row.getValue("array.array1") instanceof String);
+
+ // primitive array not converted
Review comment:
The rules aren't clear. As per the design doc (in the Collection handling section), if collection unnesting rule is not applied on a collection, then it will use JSON serialization rule right?
So why are we differentiating now on primitive collection v/s collection of non scalars ?
--
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.
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
[GitHub] [incubator-pinot] codecov-commenter edited a comment on pull request #6918: Convert collections not unnested to json
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #6918:
URL: https://github.com/apache/incubator-pinot/pull/6918#issuecomment-840920229
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#6918](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3d9afa7) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/08ed07ce27cd56b341fa634fa1cad9e440f39f6e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (08ed07c) will **increase** coverage by `30.24%`.
> The diff coverage is `45.34%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #6918 +/- ##
=============================================
+ Coverage 43.20% 73.44% +30.24%
- Complexity 7 12 +5
=============================================
Files 1431 1431
Lines 70615 70681 +66
Branches 10220 10233 +13
=============================================
+ Hits 30506 51915 +21409
+ Misses 37502 15320 -22182
- Partials 2607 3446 +839
```
| Flag | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| integration | `43.16% <1.16%> (-0.05%)` | `7.00 <0.00> (ø)` | |
| unittests | `65.47% <45.34%> (?)` | `12.00 <0.00> (?)` | |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...ocal/recordtransformer/ComplexTypeTransformer.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWNvcmR0cmFuc2Zvcm1lci9Db21wbGV4VHlwZVRyYW5zZm9ybWVyLmphdmE=) | `56.28% <41.97%> (+55.45%)` | `0.00 <0.00> (ø)` | |
| [.../spi/config/table/ingestion/ComplexTypeConfig.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL2luZ2VzdGlvbi9Db21wbGV4VHlwZUNvbmZpZy5qYXZh) | `100.00% <100.00%> (+100.00%)` | `0.00 <0.00> (ø)` | |
| [...data/manager/realtime/SegmentCommitterFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvU2VnbWVudENvbW1pdHRlckZhY3RvcnkuamF2YQ==) | `64.70% <0.00%> (-35.30%)` | `0.00% <0.00%> (ø%)` | |
| [...altime/ServerSegmentCompletionProtocolHandler.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VydmVyL3JlYWx0aW1lL1NlcnZlclNlZ21lbnRDb21wbGV0aW9uUHJvdG9jb2xIYW5kbGVyLmphdmE=) | `48.54% <0.00%> (-8.74%)` | `0.00% <0.00%> (ø%)` | |
| [...er/api/resources/LLCSegmentCompletionHandlers.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL0xMQ1NlZ21lbnRDb21wbGV0aW9uSGFuZGxlcnMuamF2YQ==) | `51.16% <0.00%> (-8.38%)` | `0.00% <0.00%> (ø%)` | |
| [...e/data/manager/realtime/SplitSegmentCommitter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvU3BsaXRTZWdtZW50Q29tbWl0dGVyLmphdmE=) | `53.84% <0.00%> (-7.70%)` | `0.00% <0.00%> (ø%)` | |
| [...impl/dictionary/DoubleOnHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvRG91YmxlT25IZWFwTXV0YWJsZURpY3Rpb25hcnkuamF2YQ==) | `46.98% <0.00%> (-6.03%)` | `0.00% <0.00%> (ø%)` | |
| [...e/impl/dictionary/LongOnHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvTG9uZ09uSGVhcE11dGFibGVEaWN0aW9uYXJ5LmphdmE=) | `63.85% <0.00%> (-6.03%)` | `0.00% <0.00%> (ø%)` | |
| [...impl/dictionary/FloatOffHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvRmxvYXRPZmZIZWFwTXV0YWJsZURpY3Rpb25hcnkuamF2YQ==) | `72.34% <0.00%> (-5.32%)` | `0.00% <0.00%> (ø%)` | |
| [...cal/startree/v2/builder/BaseSingleTreeBuilder.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zdGFydHJlZS92Mi9idWlsZGVyL0Jhc2VTaW5nbGVUcmVlQnVpbGRlci5qYXZh) | `88.39% <0.00%> (-4.47%)` | `0.00% <0.00%> (ø%)` | |
| ... and [898 more](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [08ed07c...3d9afa7](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
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
[GitHub] [incubator-pinot] codecov-commenter edited a comment on pull request #6918: Convert collections not unnested to json
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #6918:
URL: https://github.com/apache/incubator-pinot/pull/6918#issuecomment-840920229
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#6918](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4b8b288) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/59e342df9ba7895e1d404c7f681b7c8ab664d7de?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (59e342d) will **increase** coverage by `0.03%`.
> The diff coverage is `45.34%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #6918 +/- ##
============================================
+ Coverage 73.49% 73.53% +0.03%
Complexity 12 12
============================================
Files 1432 1432
Lines 70603 70669 +66
Branches 10206 10219 +13
============================================
+ Hits 51892 51967 +75
+ Misses 15277 15260 -17
- Partials 3434 3442 +8
```
| Flag | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| integration | `43.09% <1.16%> (-0.14%)` | `7.00 <0.00> (ø)` | |
| unittests | `65.40% <45.34%> (-0.02%)` | `12.00 <0.00> (ø)` | |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...ocal/recordtransformer/ComplexTypeTransformer.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWNvcmR0cmFuc2Zvcm1lci9Db21wbGV4VHlwZVRyYW5zZm9ybWVyLmphdmE=) | `56.28% <41.97%> (-9.01%)` | `0.00 <0.00> (ø)` | |
| [.../spi/config/table/ingestion/ComplexTypeConfig.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL2luZ2VzdGlvbi9Db21wbGV4VHlwZUNvbmZpZy5qYXZh) | `100.00% <100.00%> (ø)` | `0.00 <0.00> (ø)` | |
| [...a/manager/realtime/RealtimeSegmentDataManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvUmVhbHRpbWVTZWdtZW50RGF0YU1hbmFnZXIuamF2YQ==) | `28.57% <0.00%> (-28.58%)` | `0.00% <0.00%> (ø%)` | |
| [.../org/apache/pinot/core/startree/StarTreeUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9TdGFyVHJlZVV0aWxzLmphdmE=) | `73.58% <0.00%> (-3.78%)` | `0.00% <0.00%> (ø%)` | |
| [.../java/org/apache/pinot/spi/data/TimeFieldSpec.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9UaW1lRmllbGRTcGVjLmphdmE=) | `88.63% <0.00%> (-2.28%)` | `0.00% <0.00%> (ø%)` | |
| [...g/apache/pinot/common/utils/helix/HelixHelper.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvaGVsaXgvSGVsaXhIZWxwZXIuamF2YQ==) | `55.81% <0.00%> (-1.75%)` | `0.00% <0.00%> (ø%)` | |
| [.../impl/dictionary/FloatOnHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvRmxvYXRPbkhlYXBNdXRhYmxlRGljdGlvbmFyeS5qYXZh) | `74.69% <0.00%> (-1.21%)` | `0.00% <0.00%> (ø%)` | |
| [...a/org/apache/pinot/core/common/DataBlockCache.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9jb21tb24vRGF0YUJsb2NrQ2FjaGUuamF2YQ==) | `96.21% <0.00%> (-0.76%)` | `0.00% <0.00%> (ø%)` | |
| [...ation/function/MinMaxRangeAggregationFunction.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9mdW5jdGlvbi9NaW5NYXhSYW5nZUFnZ3JlZ2F0aW9uRnVuY3Rpb24uamF2YQ==) | `88.75% <0.00%> (+1.25%)` | `0.00% <0.00%> (ø%)` | |
| [...nMaxValueBasedSelectionOrderByCombineOperator.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9jb21iaW5lL01pbk1heFZhbHVlQmFzZWRTZWxlY3Rpb25PcmRlckJ5Q29tYmluZU9wZXJhdG9yLmphdmE=) | `70.14% <0.00%> (+1.49%)` | `0.00% <0.00%> (ø%)` | |
| ... and [11 more](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [59e342d...4b8b288](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
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
[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #6918: Convert collections not unnested to json
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6918:
URL: https://github.com/apache/incubator-pinot/pull/6918#discussion_r632854615
##########
File path: pinot-segment-local/src/test/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformerTest.java
##########
@@ -237,4 +238,76 @@ public void testUnnestCollection() {
next = itr.next();
Assert.assertEquals("v2", next.getValue("array.a"));
}
+
+ @Test
+ public void testConvertCollectionToString() {
+ // json convert inner collections
+ // {
+ // "array":[
+ // {
+ // "array1":[
+ // {
+ // "b":"v1"
+ // }
+ // ]
+ // }
+ // ]
+ // }
+ // is converted to
+ // [{
+ // "array.array1":"[
+ // {
+ // "b":"v1"
+ // }
+ // ]"
+ // }]
+ ComplexTypeTransformer transformer = new ComplexTypeTransformer(Arrays.asList("array"), ".");
+ GenericRow genericRow = new GenericRow();
+ Map<String, Object> map = new HashMap<>();
+ Object[] array1 = new Object[1];
+ array1[0] = ImmutableMap.of("b", "v1");
+ map.put("array1", array1);
+ Object[] array = new Object[1];
+ array[0] = map;
+ genericRow.putValue("array", array);
+ transformer.transform(genericRow);
+ Assert.assertNotNull(genericRow.getValue(GenericRow.MULTIPLE_RECORDS_KEY));
+ Collection<GenericRow> collection = (Collection<GenericRow>) genericRow.getValue(GenericRow.MULTIPLE_RECORDS_KEY);
+ GenericRow row = collection.iterator().next();
+ Assert.assertTrue(row.getValue("array.array1") instanceof String);
+
+ // primitive array not converted
Review comment:
Here we are talking about array inside a nested object which doesn't get the same storage format and treatment (at least in the current design) as an MV column in the schema.
If I have something like
```
myjson
{
name
age
salary
array of organizationIds (primitive integers)
array of my addresses (
city,
zip,
array (phones) primitive
)
}
```
Even though array of organizationIds is an array of primitive INTs, we currently don't store it in the same way as we would have done if it were an independent MV column in the schema. So the collection unnesting rule we have in design should be applicable to both collections in the same way. Similarly, I think if the user chooses to not unnest them then the behavior (json string serialization rule will apply) should be same for both collections regardless of whether one is for primitives and other is not.
--
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.
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
[GitHub] [incubator-pinot] yupeng9 commented on a change in pull request #6918: Convert collections not unnested to json
Posted by GitBox <gi...@apache.org>.
yupeng9 commented on a change in pull request #6918:
URL: https://github.com/apache/incubator-pinot/pull/6918#discussion_r633797199
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformer.java
##########
@@ -256,23 +318,73 @@ private void flattenMap(String path, Map<String, Object> map, Collection<String>
flattenMap(concatName, map, innerMapFields);
}
} else if (value instanceof Collection && _unnestFields.contains(concatName)) {
- for (Object inner : (Collection) value) {
- if (inner instanceof Map) {
- Map<String, Object> innerMap = (Map<String, Object>) inner;
- flattenMap(concatName, innerMap, new ArrayList<>(innerMap.keySet()));
+ Collection collection = (Collection) value;
+ if (_unnestFields.contains(concatName)) {
+ for (Object inner : (Collection) value) {
+ if (inner instanceof Map) {
+ Map<String, Object> innerMap = (Map<String, Object>) inner;
+ flattenMap(concatName, innerMap, new ArrayList<>(innerMap.keySet()));
+ }
+ }
+ } else if (shallConvertToJson(collection)) {
+ try {
+ // convert the collection to JSON string
+ String jsonString = JsonFunctions.jsonFormat(collection);
+ map.put(field, jsonString);
+ } catch (JsonProcessingException e) {
+ throw new RuntimeException(
+ String.format("Caught exception while converting value to JSON string %s", value), e);
}
}
- } else if (isArray(value) && _unnestFields.contains(concatName)) {
- for (Object inner : (Object[]) value) {
- if (inner instanceof Map) {
- Map<String, Object> innerMap = (Map<String, Object>) inner;
- flattenMap(concatName, innerMap, new ArrayList<>(innerMap.keySet()));
+ } else if (isArray(value)) {
+ Object[] array = (Object[]) value;
+ if (_unnestFields.contains(concatName)) {
+ for (Object inner : (Object[]) value) {
+ if (inner instanceof Map) {
+ Map<String, Object> innerMap = (Map<String, Object>) inner;
+ flattenMap(concatName, innerMap, new ArrayList<>(innerMap.keySet()));
+ }
+ }
+ } else if (shallConvertToJson(array)) {
+ try {
+ // convert the array to JSON string
+ String jsonString = JsonFunctions.jsonFormat(array);
+ map.put(field, jsonString);
+ } catch (JsonProcessingException e) {
+ throw new RuntimeException(
+ String.format("Caught exception while converting value to JSON string %s", value), e);
}
}
}
}
}
+ private boolean shallConvertToJson(Object[] value) {
+ switch (_collectionToJsonMode) {
+ case ALL:
+ return true;
+ case NONE:
+ return false;
+ case NON_PRIMITIVE:
+ return !containPrimitives(value);
+ default:
+ throw new IllegalArgumentException(String.format("Unsupported collectionToJsonMode %s", _collectionToJsonMode));
+ }
+ }
+
+ private boolean shallConvertToJson(Collection value) {
Review comment:
True. But this needs to make a shall copy?
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformer.java
##########
@@ -256,23 +318,73 @@ private void flattenMap(String path, Map<String, Object> map, Collection<String>
flattenMap(concatName, map, innerMapFields);
}
} else if (value instanceof Collection && _unnestFields.contains(concatName)) {
- for (Object inner : (Collection) value) {
- if (inner instanceof Map) {
- Map<String, Object> innerMap = (Map<String, Object>) inner;
- flattenMap(concatName, innerMap, new ArrayList<>(innerMap.keySet()));
+ Collection collection = (Collection) value;
+ if (_unnestFields.contains(concatName)) {
+ for (Object inner : (Collection) value) {
+ if (inner instanceof Map) {
+ Map<String, Object> innerMap = (Map<String, Object>) inner;
+ flattenMap(concatName, innerMap, new ArrayList<>(innerMap.keySet()));
+ }
+ }
+ } else if (shallConvertToJson(collection)) {
+ try {
+ // convert the collection to JSON string
+ String jsonString = JsonFunctions.jsonFormat(collection);
+ map.put(field, jsonString);
+ } catch (JsonProcessingException e) {
+ throw new RuntimeException(
+ String.format("Caught exception while converting value to JSON string %s", value), e);
}
}
- } else if (isArray(value) && _unnestFields.contains(concatName)) {
- for (Object inner : (Object[]) value) {
- if (inner instanceof Map) {
- Map<String, Object> innerMap = (Map<String, Object>) inner;
- flattenMap(concatName, innerMap, new ArrayList<>(innerMap.keySet()));
+ } else if (isArray(value)) {
+ Object[] array = (Object[]) value;
+ if (_unnestFields.contains(concatName)) {
+ for (Object inner : (Object[]) value) {
+ if (inner instanceof Map) {
+ Map<String, Object> innerMap = (Map<String, Object>) inner;
+ flattenMap(concatName, innerMap, new ArrayList<>(innerMap.keySet()));
+ }
+ }
+ } else if (shallConvertToJson(array)) {
+ try {
+ // convert the array to JSON string
+ String jsonString = JsonFunctions.jsonFormat(array);
+ map.put(field, jsonString);
+ } catch (JsonProcessingException e) {
+ throw new RuntimeException(
+ String.format("Caught exception while converting value to JSON string %s", value), e);
}
}
}
}
}
+ private boolean shallConvertToJson(Object[] value) {
+ switch (_collectionToJsonMode) {
+ case ALL:
+ return true;
+ case NONE:
+ return false;
+ case NON_PRIMITIVE:
+ return !containPrimitives(value);
+ default:
+ throw new IllegalArgumentException(String.format("Unsupported collectionToJsonMode %s", _collectionToJsonMode));
+ }
+ }
+
+ private boolean shallConvertToJson(Collection value) {
Review comment:
True. But this needs to make a shallow copy?
--
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.
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
[GitHub] [incubator-pinot] yupeng9 commented on a change in pull request #6918: Convert collections not unnested to json
Posted by GitBox <gi...@apache.org>.
yupeng9 commented on a change in pull request #6918:
URL: https://github.com/apache/incubator-pinot/pull/6918#discussion_r632873961
##########
File path: pinot-segment-local/src/test/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformerTest.java
##########
@@ -237,4 +238,76 @@ public void testUnnestCollection() {
next = itr.next();
Assert.assertEquals("v2", next.getValue("array.a"));
}
+
+ @Test
+ public void testConvertCollectionToString() {
Review comment:
Yes. This is covered by the test `testUnnestCollection ` above
--
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.
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
[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #6918: Convert collections not unnested to json
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6918:
URL: https://github.com/apache/incubator-pinot/pull/6918#discussion_r632854615
##########
File path: pinot-segment-local/src/test/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformerTest.java
##########
@@ -237,4 +238,76 @@ public void testUnnestCollection() {
next = itr.next();
Assert.assertEquals("v2", next.getValue("array.a"));
}
+
+ @Test
+ public void testConvertCollectionToString() {
+ // json convert inner collections
+ // {
+ // "array":[
+ // {
+ // "array1":[
+ // {
+ // "b":"v1"
+ // }
+ // ]
+ // }
+ // ]
+ // }
+ // is converted to
+ // [{
+ // "array.array1":"[
+ // {
+ // "b":"v1"
+ // }
+ // ]"
+ // }]
+ ComplexTypeTransformer transformer = new ComplexTypeTransformer(Arrays.asList("array"), ".");
+ GenericRow genericRow = new GenericRow();
+ Map<String, Object> map = new HashMap<>();
+ Object[] array1 = new Object[1];
+ array1[0] = ImmutableMap.of("b", "v1");
+ map.put("array1", array1);
+ Object[] array = new Object[1];
+ array[0] = map;
+ genericRow.putValue("array", array);
+ transformer.transform(genericRow);
+ Assert.assertNotNull(genericRow.getValue(GenericRow.MULTIPLE_RECORDS_KEY));
+ Collection<GenericRow> collection = (Collection<GenericRow>) genericRow.getValue(GenericRow.MULTIPLE_RECORDS_KEY);
+ GenericRow row = collection.iterator().next();
+ Assert.assertTrue(row.getValue("array.array1") instanceof String);
+
+ // primitive array not converted
Review comment:
As a MV column. But here we are talking about array inside a nested object which doesn't get the same storage format and treatment (at least in the current design) as an MV column in the schema.
If I have something like
```
{
name
age
salary
array of organizationIds (primitive integers)
array of my addresses (another nested struct)
}
```
Even though array of organizationIds is an array of primitive INTs, we currently don't store it in the same way as we would have done if it were an independent MV column in the schema. So the collection unnesting rule we have in design should be applicable to both collections in the same way. Similarly, I think if the user chooses to not unnest them then the behavior (json string serialization rule will apply) should be same for both collections regardless of whether one is for primitives and other is not.
--
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.
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
[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #6918: Convert collections not unnested to json
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6918:
URL: https://github.com/apache/incubator-pinot/pull/6918#discussion_r632854615
##########
File path: pinot-segment-local/src/test/java/org/apache/pinot/segment/local/recordtransformer/ComplexTypeTransformerTest.java
##########
@@ -237,4 +238,76 @@ public void testUnnestCollection() {
next = itr.next();
Assert.assertEquals("v2", next.getValue("array.a"));
}
+
+ @Test
+ public void testConvertCollectionToString() {
+ // json convert inner collections
+ // {
+ // "array":[
+ // {
+ // "array1":[
+ // {
+ // "b":"v1"
+ // }
+ // ]
+ // }
+ // ]
+ // }
+ // is converted to
+ // [{
+ // "array.array1":"[
+ // {
+ // "b":"v1"
+ // }
+ // ]"
+ // }]
+ ComplexTypeTransformer transformer = new ComplexTypeTransformer(Arrays.asList("array"), ".");
+ GenericRow genericRow = new GenericRow();
+ Map<String, Object> map = new HashMap<>();
+ Object[] array1 = new Object[1];
+ array1[0] = ImmutableMap.of("b", "v1");
+ map.put("array1", array1);
+ Object[] array = new Object[1];
+ array[0] = map;
+ genericRow.putValue("array", array);
+ transformer.transform(genericRow);
+ Assert.assertNotNull(genericRow.getValue(GenericRow.MULTIPLE_RECORDS_KEY));
+ Collection<GenericRow> collection = (Collection<GenericRow>) genericRow.getValue(GenericRow.MULTIPLE_RECORDS_KEY);
+ GenericRow row = collection.iterator().next();
+ Assert.assertTrue(row.getValue("array.array1") instanceof String);
+
+ // primitive array not converted
Review comment:
As a MV column. But here we are talking about array inside a nested object which doesn't get the same storage format and treatment (at least in the current design) as an MV column in the schema.
If I have something like
```
{
name
age
salary
array of organizationIds (primitive integers)
array of my addresses (another nested struct)
}
```
Even though array of organizationIds is an array of primitive INTs, we currently don't store it in the same way as we would have done if it were an independent MV column in the schema. So the collection unnesting rule we have in design should be applicable to both collections in the same way. Similarly, I think if the user chooses to not unnest them then the behavior should be same for both collections regardless of whether one is for primitives and other is not.
--
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.
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
[GitHub] [incubator-pinot] codecov-commenter edited a comment on pull request #6918: Convert collections not unnested to json
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #6918:
URL: https://github.com/apache/incubator-pinot/pull/6918#issuecomment-840920229
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#6918](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4b8b288) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/59e342df9ba7895e1d404c7f681b7c8ab664d7de?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (59e342d) will **decrease** coverage by `8.09%`.
> The diff coverage is `45.34%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #6918 +/- ##
============================================
- Coverage 73.49% 65.40% -8.10%
Complexity 12 12
============================================
Files 1432 1432
Lines 70603 70669 +66
Branches 10206 10219 +13
============================================
- Hits 51892 46223 -5669
- Misses 15277 21133 +5856
+ Partials 3434 3313 -121
```
| Flag | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| integration | `?` | `?` | |
| unittests | `65.40% <45.34%> (-0.02%)` | `12.00 <0.00> (ø)` | |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...ocal/recordtransformer/ComplexTypeTransformer.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWNvcmR0cmFuc2Zvcm1lci9Db21wbGV4VHlwZVRyYW5zZm9ybWVyLmphdmE=) | `56.28% <41.97%> (-9.01%)` | `0.00 <0.00> (ø)` | |
| [.../spi/config/table/ingestion/ComplexTypeConfig.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL2luZ2VzdGlvbi9Db21wbGV4VHlwZUNvbmZpZy5qYXZh) | `100.00% <100.00%> (ø)` | `0.00 <0.00> (ø)` | |
| [...a/org/apache/pinot/minion/metrics/MinionMeter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJRdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [.../apache/pinot/minion/metrics/MinionQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25RdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [...pache/pinot/common/utils/grpc/GrpcQueryClient.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvZ3JwYy9HcnBjUXVlcnlDbGllbnQuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [...pinot/minion/exception/TaskCancelledException.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhjZXB0aW9uL1Rhc2tDYW5jZWxsZWRFeGNlcHRpb24uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [...t/core/startree/plan/StarTreeDocIdSetPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlRG9jSWRTZXRQbGFuTm9kZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [.../core/startree/plan/StarTreeTransformPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlVHJhbnNmb3JtUGxhbk5vZGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [...core/startree/plan/StarTreeProjectionPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlUHJvamVjdGlvblBsYW5Ob2RlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| ... and [339 more](https://codecov.io/gh/apache/incubator-pinot/pull/6918/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [59e342d...4b8b288](https://codecov.io/gh/apache/incubator-pinot/pull/6918?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
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