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 2022/04/27 06:54:43 UTC
[GitHub] [pinot] KKcorps opened a new pull request, #8600: Bug fix: Complex type transformer should not be created for empty confi
KKcorps opened a new pull request, #8600:
URL: https://github.com/apache/pinot/pull/8600
Currently, if someone specifies empty `complexTypeConfig` in table config, we create a transformer for it. This can cause unintended issues in flows such as parsing JSON data in schema which starts returning `null` instead of proper value. Currently the only solution is to just remove the config key from the table config if not required. This PR however fixes this issue.
Sample config which can be used to reproduce the issue
```json
{
"tableName": "myObjects",
"tableType": "REALTIME",
"segmentsConfig": {
"timeType": "MILLISECONDS",
"schemaName": "myObjects",
"retentionTimeUnit": "DAYS",
"retentionTimeValue": "365",
"timeColumnName": "lastModified",
"allowNullTimeValue": false,
"replicasPerPartition": "1"
},
"tenants": {
},
"tableIndexConfig": {
"rangeIndexVersion": 2,
"jsonIndexColumns": [
"jsonObject"
],
"autoGeneratedInvertedIndex": false,
"createInvertedIndexDuringSegmentGeneration": false,
"loadMode": "MMAP",
"noDictionaryColumns": [
"lastModified",
"jsonObject"
],
"enableDefaultStarTree": false,
"enableDynamicStarTreeCreation": false,
"segmentPartitionConfig": {
"columnPartitionMap": {
"objectId": {
"functionName": "Murmur",
"numPartitions": 2
}
}
},
"aggregateMetrics": false,
"nullHandlingEnabled": false
},
"metadata": {
"customConfigs": {}
},
"ingestionConfig": {
"streamIngestionConfig": {
"streamConfigMaps": [
{
"streamType": "kafka",
"stream.kafka.consumer.type": "lowlevel",
"stream.kafka.topic.name": "my_objects",
"stream.kafka.decoder.class.name": "org.apache.pinot.plugin.inputformat.json.JSONMessageDecoder",
"stream.kafka.consumer.factory.class.name": "org.apache.pinot.plugin.stream.kafka20.KafkaConsumerFactory",
"stream.kafka.broker.list": "localhost:19092",
"realtime.segment.flush.threshold.rows": "0",
"realtime.segment.flush.threshold.time": "24h",
"realtime.segment.flush.threshold.segment.size": "200M",
"realtime.segment.flush.autotune.initialRows": "2000000",
"stream.kafka.consumer.prop.auto.offset.reset": "smallest"
}
]
},
"transformConfigs": [],
"complexTypeConfig": {}
},
"isDimTable": false
}
```
Schema -
```json
{
"schemaName": "myObjects",
"dimensionFieldSpecs": [
{
"name": "objectId",
"dataType": "STRING"
},
{
"name": "jsonObject",
"dataType": "JSON"
}
],
"dateTimeFieldSpecs": [
{
"name": "lastModified",
"dataType": "LONG",
"format": "1:MILLISECONDS:EPOCH",
"granularity": "1:DAYS"
}
]
}
```
Record
```
{ "lastModified":1651001043557, "objectId": "00000000-0000-0000-0000-000000000000", "jsonObject": {"values": [ {"id": "bob", "names": ["a","b","c","d","e"] } ] }}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] npawar commented on pull request #8600: Bug fix: Complex type transformer should not be created for empty config
Posted by GitBox <gi...@apache.org>.
npawar commented on PR #8600:
URL: https://github.com/apache/pinot/pull/8600#issuecomment-1118109762
@KKcorps let's revert this change, until we find a better way to fix. As per docs, https://docs.pinot.apache.org/basics/data-import/complex-type#handle-the-complex-type-with-ingestion-configurations basic flattening is triggered by having complexConfig, and fieldsToUnnest is just an addition for collections.
I don't remember what was the exact issue the user was facing, maybe we can address it in a different way in the code within ComplexTransformer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] yupeng9 commented on pull request #8600: Bug fix: Complex type transformer should not be created for empty config
Posted by GitBox <gi...@apache.org>.
yupeng9 commented on PR #8600:
URL: https://github.com/apache/pinot/pull/8600#issuecomment-1118137565
yup, @Jackie-Jiang is right that this means users want to enable the flattening
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #8600: Bug fix: Complex type transformer should not be created for empty config
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #8600:
URL: https://github.com/apache/pinot/pull/8600#issuecomment-1110763304
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8600?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 [#8600](https://codecov.io/gh/apache/pinot/pull/8600?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (bf05f43) into [master](https://codecov.io/gh/apache/pinot/commit/b0144cfba1959f7eb800450c2b3f44c3d85dfeaa?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b0144cf) will **decrease** coverage by `34.00%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #8600 +/- ##
=============================================
- Coverage 70.65% 36.65% -34.01%
+ Complexity 4314 84 -4230
=============================================
Files 1692 1692
Lines 88751 88752 +1
Branches 13468 13469 +1
=============================================
- Hits 62708 32532 -30176
- Misses 21657 53592 +31935
+ Partials 4386 2628 -1758
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `27.23% <0.00%> (-0.01%)` | :arrow_down: |
| integration2 | `25.65% <0.00%> (-0.04%)` | :arrow_down: |
| unittests1 | `?` | |
| unittests2 | `14.17% <0.00%> (-0.01%)` | :arrow_down: |
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/pinot/pull/8600?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ocal/recordtransformer/ComplexTypeTransformer.java](https://codecov.io/gh/apache/pinot/pull/8600/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=) | `0.00% <0.00%> (-58.58%)` | :arrow_down: |
| [...in/java/org/apache/pinot/spi/utils/StringUtil.java](https://codecov.io/gh/apache/pinot/pull/8600/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvU3RyaW5nVXRpbC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/spi/utils/BooleanUtils.java](https://codecov.io/gh/apache/pinot/pull/8600/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQm9vbGVhblV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/BaseRecording.java](https://codecov.io/gh/apache/pinot/pull/8600/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvQmFzZVJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/NoOpRecording.java](https://codecov.io/gh/apache/pinot/pull/8600/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvTm9PcFJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/config/table/FSTType.java](https://codecov.io/gh/apache/pinot/pull/8600/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL0ZTVFR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/data/MetricFieldSpec.java](https://codecov.io/gh/apache/pinot/pull/8600/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9NZXRyaWNGaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...va/org/apache/pinot/spi/utils/BigDecimalUtils.java](https://codecov.io/gh/apache/pinot/pull/8600/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQmlnRGVjaW1hbFV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/common/tier/TierFactory.java](https://codecov.io/gh/apache/pinot/pull/8600/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdGllci9UaWVyRmFjdG9yeS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...a/org/apache/pinot/spi/config/table/TableType.java](https://codecov.io/gh/apache/pinot/pull/8600/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL1RhYmxlVHlwZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [968 more](https://codecov.io/gh/apache/pinot/pull/8600/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/pinot/pull/8600?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/pinot/pull/8600?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 [89836a5...bf05f43](https://codecov.io/gh/apache/pinot/pull/8600?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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] xiangfu0 merged pull request #8600: Bug fix: Complex type transformer should not be created for empty config
Posted by GitBox <gi...@apache.org>.
xiangfu0 merged PR #8600:
URL: https://github.com/apache/pinot/pull/8600
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] KKcorps commented on pull request #8600: Bug fix: Complex type transformer should not be created for empty config
Posted by GitBox <gi...@apache.org>.
KKcorps commented on PR #8600:
URL: https://github.com/apache/pinot/pull/8600#issuecomment-1118198026
Ohh. I wasn't aware of the basic flattening. I will revert this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on pull request #8600: Bug fix: Complex type transformer should not be created for empty config
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on PR #8600:
URL: https://github.com/apache/pinot/pull/8600#issuecomment-1111554014
I don't think this is the correct fix.
It is valid to have `fieldsToUnnest` as `null` when we don't want to unnest any field.
I don't think this is a bug, but a user config error. When user puts the config, it means user wants to flatten the json field, but unnest is optional. I don't see a good way to prevent this. One option is to add a flag to enable the map flattening, but that is backward incompatible change, and IMO that should be implicit when user puts the complex config.
cc @yupeng9
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] KKcorps commented on pull request #8600: Bug fix: Complex type transformer should not be created for empty config
Posted by GitBox <gi...@apache.org>.
KKcorps commented on PR #8600:
URL: https://github.com/apache/pinot/pull/8600#issuecomment-1118205623
That being said. Isn't an empty config doing something incorrect choice in itself? I guess there at least should be an 'enabled' / 'disabled' field.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org