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/07/20 11:10:06 UTC
[GitHub] [pinot] lksvenoy-r7 opened a new pull request, #9077: Ensure conversion of field specs to avro schema is ordered
lksvenoy-r7 opened a new pull request, #9077:
URL: https://github.com/apache/pinot/pull/9077
This is a bug fix for the SegmentProcessorAvroUtils during the conversion of a pinot schema to an avro schema. Since the field specs are a collection, order is not guaranteed. Avro schema rely on order, which means that multiple invocations of this method may result in different ordering, causing serialization to break.
This change ensures the avro schema produced is predictable on all invocations by ordering the field specs prior to iterating them.
--
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 #9077: Ensure conversion of field specs to avro schema is ordered
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #9077:
URL: https://github.com/apache/pinot/pull/9077#issuecomment-1192397465
# [Codecov](https://codecov.io/gh/apache/pinot/pull/9077?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 [#9077](https://codecov.io/gh/apache/pinot/pull/9077?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (52c1c3d) into [master](https://codecov.io/gh/apache/pinot/commit/f0a4b443699bd550b1608e4dea7cd68bc9bc6a61?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f0a4b44) will **decrease** coverage by `41.63%`.
> The diff coverage is `13.36%`.
> :exclamation: Current head 52c1c3d differs from pull request most recent head 43d12ca. Consider uploading reports for the commit 43d12ca to get more accurate results
```diff
@@ Coverage Diff @@
## master #9077 +/- ##
=============================================
- Coverage 70.06% 28.42% -41.64%
+ Complexity 4743 51 -4692
=============================================
Files 1832 1823 -9
Lines 96971 96769 -202
Branches 14620 14594 -26
=============================================
- Hits 67939 27511 -40428
- Misses 24305 66662 +42357
+ Partials 4727 2596 -2131
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `26.35% <13.36%> (-0.04%)` | :arrow_down: |
| integration2 | `24.46% <11.38%> (-0.22%)` | :arrow_down: |
| unittests1 | `?` | |
| unittests2 | `?` | |
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/9077?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ache/pinot/broker/broker/AccessControlFactory.java](https://codecov.io/gh/apache/pinot/pull/9077/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FjY2Vzc0NvbnRyb2xGYWN0b3J5LmphdmE=) | `80.00% <ø> (ø)` | |
| [...broker/broker/ZkBasicAuthAccessControlFactory.java](https://codecov.io/gh/apache/pinot/pull/9077/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL1prQmFzaWNBdXRoQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `0.00% <ø> (ø)` | |
| [...e/pinot/broker/broker/helix/BaseBrokerStarter.java](https://codecov.io/gh/apache/pinot/pull/9077/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jhc2VCcm9rZXJTdGFydGVyLmphdmE=) | `77.93% <ø> (-1.88%)` | :arrow_down: |
| [.../BrokerResourceOnlineOfflineStateModelFactory.java](https://codecov.io/gh/apache/pinot/pull/9077/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclJlc291cmNlT25saW5lT2ZmbGluZVN0YXRlTW9kZWxGYWN0b3J5LmphdmE=) | `55.81% <ø> (ø)` | |
| [...quota/HelixExternalViewBasedQueryQuotaManager.java](https://codecov.io/gh/apache/pinot/pull/9077/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9IZWxpeEV4dGVybmFsVmlld0Jhc2VkUXVlcnlRdW90YU1hbmFnZXIuamF2YQ==) | `44.39% <ø> (-24.67%)` | :arrow_down: |
| [...che/pinot/broker/routing/BrokerRoutingManager.java](https://codecov.io/gh/apache/pinot/pull/9077/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9Ccm9rZXJSb3V0aW5nTWFuYWdlci5qYXZh) | `85.31% <ø> (ø)` | |
| [...elector/SegmentLineageBasedSegmentPreSelector.java](https://codecov.io/gh/apache/pinot/pull/9077/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJlc2VsZWN0b3IvU2VnbWVudExpbmVhZ2VCYXNlZFNlZ21lbnRQcmVTZWxlY3Rvci5qYXZh) | `100.00% <ø> (ø)` | |
| [.../segmentpreselector/SegmentPreSelectorFactory.java](https://codecov.io/gh/apache/pinot/pull/9077/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJlc2VsZWN0b3IvU2VnbWVudFByZVNlbGVjdG9yRmFjdG9yeS5qYXZh) | `100.00% <ø> (ø)` | |
| [...oker/routing/segmentpruner/EmptySegmentPruner.java](https://codecov.io/gh/apache/pinot/pull/9077/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL0VtcHR5U2VnbWVudFBydW5lci5qYXZh) | `0.00% <ø> (-87.94%)` | :arrow_down: |
| [...mentpruner/MultiPartitionColumnsSegmentPruner.java](https://codecov.io/gh/apache/pinot/pull/9077/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL011bHRpUGFydGl0aW9uQ29sdW1uc1NlZ21lbnRQcnVuZXIuamF2YQ==) | `0.00% <ø> (-73.92%)` | :arrow_down: |
| ... and [1353 more](https://codecov.io/gh/apache/pinot/pull/9077/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) | |
Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?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] Jackie-Jiang commented on a diff in pull request #9077: Ensure conversion of field specs to avro schema is ordered
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #9077:
URL: https://github.com/apache/pinot/pull/9077#discussion_r925970719
##########
pinot-core/src/main/java/org/apache/pinot/core/util/SegmentProcessorAvroUtils.java:
##########
@@ -74,7 +74,10 @@ public static GenericData.Record convertGenericRowToAvroRecord(GenericRow generi
public static Schema convertPinotSchemaToAvroSchema(org.apache.pinot.spi.data.Schema pinotSchema) {
SchemaBuilder.FieldAssembler<org.apache.avro.Schema> fieldAssembler = SchemaBuilder.record("record").fields();
- for (FieldSpec fieldSpec : pinotSchema.getAllFieldSpecs()) {
+ List<FieldSpec> orderedFieldSpecs = pinotSchema.getAllFieldSpecs().stream()
+ .sorted()
Review Comment:
Good catch! Here we want to order the fields by name. `FieldSpec` itself is not comparable
--
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] lksvenoy-r7 commented on a diff in pull request #9077: Ensure conversion of field specs to avro schema is ordered
Posted by GitBox <gi...@apache.org>.
lksvenoy-r7 commented on code in PR #9077:
URL: https://github.com/apache/pinot/pull/9077#discussion_r926370173
##########
pinot-core/src/main/java/org/apache/pinot/core/util/SegmentProcessorAvroUtils.java:
##########
@@ -74,7 +74,10 @@ public static GenericData.Record convertGenericRowToAvroRecord(GenericRow generi
public static Schema convertPinotSchemaToAvroSchema(org.apache.pinot.spi.data.Schema pinotSchema) {
SchemaBuilder.FieldAssembler<org.apache.avro.Schema> fieldAssembler = SchemaBuilder.record("record").fields();
- for (FieldSpec fieldSpec : pinotSchema.getAllFieldSpecs()) {
+ List<FieldSpec> orderedFieldSpecs = pinotSchema.getAllFieldSpecs().stream()
+ .sorted()
Review Comment:
Thanks for the comment @Jackie-Jiang, I overlooked that. I've fixed the sorting now.
--
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] lksvenoy-r7 commented on a diff in pull request #9077: Ensure conversion of field specs to avro schema is ordered
Posted by GitBox <gi...@apache.org>.
lksvenoy-r7 commented on code in PR #9077:
URL: https://github.com/apache/pinot/pull/9077#discussion_r927413238
##########
pinot-core/src/main/java/org/apache/pinot/core/util/SegmentProcessorAvroUtils.java:
##########
@@ -74,7 +74,10 @@ public static GenericData.Record convertGenericRowToAvroRecord(GenericRow generi
public static Schema convertPinotSchemaToAvroSchema(org.apache.pinot.spi.data.Schema pinotSchema) {
SchemaBuilder.FieldAssembler<org.apache.avro.Schema> fieldAssembler = SchemaBuilder.record("record").fields();
- for (FieldSpec fieldSpec : pinotSchema.getAllFieldSpecs()) {
+ List<FieldSpec> orderedFieldSpecs = pinotSchema.getAllFieldSpecs().stream()
+ .sorted()
Review Comment:
My bad @Jackie-Jiang. I've had some problems getting the project to work with maven lately, I'll get this sorted.
--
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 a diff in pull request #9077: Ensure conversion of field specs to avro schema is ordered
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #9077:
URL: https://github.com/apache/pinot/pull/9077#discussion_r926950462
##########
pinot-core/src/main/java/org/apache/pinot/core/util/SegmentProcessorAvroUtils.java:
##########
@@ -74,7 +74,10 @@ public static GenericData.Record convertGenericRowToAvroRecord(GenericRow generi
public static Schema convertPinotSchemaToAvroSchema(org.apache.pinot.spi.data.Schema pinotSchema) {
SchemaBuilder.FieldAssembler<org.apache.avro.Schema> fieldAssembler = SchemaBuilder.record("record").fields();
- for (FieldSpec fieldSpec : pinotSchema.getAllFieldSpecs()) {
+ List<FieldSpec> orderedFieldSpecs = pinotSchema.getAllFieldSpecs().stream()
+ .sorted()
Review Comment:
Can you please also fix the compile issue? I believe some classes are not imported
--
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 merged pull request #9077: Ensure conversion of field specs to avro schema is ordered
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged PR #9077:
URL: https://github.com/apache/pinot/pull/9077
--
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