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/03/17 20:32:48 UTC
[GitHub] [pinot] dongxiaoman opened a new pull request #8364: [WIP] Refactor Segment creation to use TransformPipeline too
dongxiaoman opened a new pull request #8364:
URL: https://github.com/apache/pinot/pull/8364
## Description
<!-- Add a description of your PR here.
A good description should include pointers to an issue or design document, etc.
-->
## Upgrade Notes
Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
* [ ] Yes (Please label as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
Does this PR fix a zero-downtime upgrade introduced earlier?
* [ ] Yes (Please label this as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
Does this PR otherwise need attention when creating release notes? Things to consider:
- New configuration options
- Deprecation of configurations
- Signature changes to public methods/interfaces
- New plugins added or old plugins removed
* [ ] Yes (Please label this PR as **<code>release-notes</code>** and complete the section on Release Notes)
## Release Notes
<!-- If you have tagged this as either backward-incompat or release-notes,
you MUST add text here that you would like to see appear in release notes of the
next release. -->
<!-- If you have a series of commits adding or enabling a feature, then
add this section only in final commit that marks the feature completed.
Refer to earlier release notes to see examples of text.
-->
## Documentation
<!-- If you have introduced a new feature or configuration, please add it to the documentation as well.
See https://docs.pinot.apache.org/developers/developers-and-contributors/update-document
-->
--
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 edited a comment on pull request #8364: Refactor Segment creation to use TransformPipeline too
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #8364:
URL: https://github.com/apache/pinot/pull/8364#issuecomment-1071540686
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8364?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 [#8364](https://codecov.io/gh/apache/pinot/pull/8364?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8d4dc5e) into [master](https://codecov.io/gh/apache/pinot/commit/24d4fd268d28473ffd3ce1ce262322391810f356?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (24d4fd2) will **decrease** coverage by `43.23%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #8364 +/- ##
=============================================
- Coverage 70.79% 27.56% -43.24%
=============================================
Files 1640 1631 -9
Lines 85931 85707 -224
Branches 12922 12888 -34
=============================================
- Hits 60837 23622 -37215
- Misses 20899 59876 +38977
+ Partials 4195 2209 -1986
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `27.56% <0.00%> (+0.02%)` | :arrow_up: |
| 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/8364?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...manager/realtime/LLRealtimeSegmentDataManager.java](https://codecov.io/gh/apache/pinot/pull/8364/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvTExSZWFsdGltZVNlZ21lbnREYXRhTWFuYWdlci5qYXZh) | `56.87% <ø> (-14.82%)` | :arrow_down: |
| [.../pinot/segment/local/segment/creator/Fixtures.java](https://codecov.io/gh/apache/pinot/pull/8364/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvRml4dHVyZXMuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...creator/RecordReaderSegmentCreationDataSource.java](https://codecov.io/gh/apache/pinot/pull/8364/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvUmVjb3JkUmVhZGVyU2VnbWVudENyZWF0aW9uRGF0YVNvdXJjZS5qYXZh) | `0.00% <0.00%> (-71.06%)` | :arrow_down: |
| [...gment/local/segment/creator/TransformPipeline.java](https://codecov.io/gh/apache/pinot/pull/8364/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvVHJhbnNmb3JtUGlwZWxpbmUuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...t/creator/impl/SegmentIndexCreationDriverImpl.java](https://codecov.io/gh/apache/pinot/pull/8364/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9TZWdtZW50SW5kZXhDcmVhdGlvbkRyaXZlckltcGwuamF2YQ==) | `0.00% <0.00%> (-83.62%)` | :arrow_down: |
| [.../java/org/apache/pinot/spi/utils/BooleanUtils.java](https://codecov.io/gh/apache/pinot/pull/8364/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: |
| [...ava/org/apache/pinot/spi/config/table/FSTType.java](https://codecov.io/gh/apache/pinot/pull/8364/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/8364/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/8364/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/8364/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: |
| ... and [1194 more](https://codecov.io/gh/apache/pinot/pull/8364/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/8364?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/8364?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 [24d4fd2...8d4dc5e](https://codecov.io/gh/apache/pinot/pull/8364?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] dongxiaoman commented on a change in pull request #8364: Refactor Segment creation to use TransformPipeline too
Posted by GitBox <gi...@apache.org>.
dongxiaoman commented on a change in pull request #8364:
URL: https://github.com/apache/pinot/pull/8364#discussion_r829555011
##########
File path: pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/creator/TransformPipelineTest.java
##########
@@ -25,18 +25,16 @@
import org.testng.Assert;
import org.testng.annotations.Test;
-import static org.apache.pinot.core.data.manager.realtime.Fixtures.*;
Review comment:
fair enough. Changed 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] dongxiaoman commented on a change in pull request #8364: [WIP] Refactor Segment creation to use TransformPipeline too
Posted by GitBox <gi...@apache.org>.
dongxiaoman commented on a change in pull request #8364:
URL: https://github.com/apache/pinot/pull/8364#discussion_r829506556
##########
File path: pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/creator/TransformPipelineTest.java
##########
@@ -25,18 +25,16 @@
import org.testng.Assert;
import org.testng.annotations.Test;
-import static org.apache.pinot.core.data.manager.realtime.Fixtures.*;
Review comment:
I think this is because originally I did not use `Fixtures.createSchema()` in code?
After the refactor the IntelliJ editor tries to remove 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] codecov-commenter edited a comment on pull request #8364: Refactor Segment creation to use TransformPipeline too
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #8364:
URL: https://github.com/apache/pinot/pull/8364#issuecomment-1071540686
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8364?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 [#8364](https://codecov.io/gh/apache/pinot/pull/8364?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d88b61d) into [master](https://codecov.io/gh/apache/pinot/commit/764a330848447e4639fd36dafcef51192f9aa026?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (764a330) will **decrease** coverage by `5.68%`.
> The diff coverage is `100.00%`.
> :exclamation: Current head d88b61d differs from pull request most recent head 89975e5. Consider uploading reports for the commit 89975e5 to get more accurate results
```diff
@@ Coverage Diff @@
## master #8364 +/- ##
============================================
- Coverage 69.68% 63.99% -5.69%
- Complexity 4263 4273 +10
============================================
Files 1642 1598 -44
Lines 86047 84180 -1867
Branches 12934 12723 -211
============================================
- Hits 59958 53869 -6089
- Misses 21919 26445 +4526
+ Partials 4170 3866 -304
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| unittests1 | `66.93% <100.00%> (+0.02%)` | :arrow_up: |
| unittests2 | `14.18% <0.00%> (-0.02%)` | :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/8364?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...manager/realtime/LLRealtimeSegmentDataManager.java](https://codecov.io/gh/apache/pinot/pull/8364/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvTExSZWFsdGltZVNlZ21lbnREYXRhTWFuYWdlci5qYXZh) | `46.03% <ø> (-23.81%)` | :arrow_down: |
| [.../pinot/segment/local/segment/creator/Fixtures.java](https://codecov.io/gh/apache/pinot/pull/8364/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvRml4dHVyZXMuamF2YQ==) | `100.00% <100.00%> (ø)` | |
| [...creator/RecordReaderSegmentCreationDataSource.java](https://codecov.io/gh/apache/pinot/pull/8364/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvUmVjb3JkUmVhZGVyU2VnbWVudENyZWF0aW9uRGF0YVNvdXJjZS5qYXZh) | `75.86% <100.00%> (+4.80%)` | :arrow_up: |
| [...gment/local/segment/creator/TransformPipeline.java](https://codecov.io/gh/apache/pinot/pull/8364/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvVHJhbnNmb3JtUGlwZWxpbmUuamF2YQ==) | `91.42% <100.00%> (ø)` | |
| [...t/creator/impl/SegmentIndexCreationDriverImpl.java](https://codecov.io/gh/apache/pinot/pull/8364/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9TZWdtZW50SW5kZXhDcmVhdGlvbkRyaXZlckltcGwuamF2YQ==) | `84.75% <100.00%> (+1.14%)` | :arrow_up: |
| [...va/org/apache/pinot/common/config/NettyConfig.java](https://codecov.io/gh/apache/pinot/pull/8364/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL05ldHR5Q29uZmlnLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...a/org/apache/pinot/common/metrics/MinionMeter.java](https://codecov.io/gh/apache/pinot/pull/8364/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...g/apache/pinot/common/metrics/ControllerMeter.java](https://codecov.io/gh/apache/pinot/pull/8364/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Db250cm9sbGVyTWV0ZXIuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/pinot/pull/8364/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%)` | :arrow_down: |
| [.../apache/pinot/common/metrics/MinionQueryPhase.java](https://codecov.io/gh/apache/pinot/pull/8364/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9NaW5pb25RdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [345 more](https://codecov.io/gh/apache/pinot/pull/8364/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/8364?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/8364?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 [764a330...89975e5](https://codecov.io/gh/apache/pinot/pull/8364?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] richardstartin commented on a change in pull request #8364: [WIP] Refactor Segment creation to use TransformPipeline too
Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #8364:
URL: https://github.com/apache/pinot/pull/8364#discussion_r829480330
##########
File path: pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/creator/TransformPipelineTest.java
##########
@@ -25,18 +25,16 @@
import org.testng.Assert;
import org.testng.annotations.Test;
-import static org.apache.pinot.core.data.manager.realtime.Fixtures.*;
Review comment:
removing this static import doesn't improve this class
--
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] richardstartin commented on a change in pull request #8364: Refactor Segment creation to use TransformPipeline too
Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #8364:
URL: https://github.com/apache/pinot/pull/8364#discussion_r829540624
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/Fixtures.java
##########
@@ -67,8 +65,8 @@ private Fixtures() {
+ " \"stream.fakeStream.broker.list\":\"broker:7777\","
+ " \"stream.fakeStream.consumer.prop.auto.offset.reset\":\"smallest\","
+ " \"stream.fakeStream.consumer.type\":\"simple\","
- + " \"stream.fakeStream.consumer.factory.class.name\":\"" + CONSUMER_FACTORY_CLASS + "\","
- + " \"stream.fakeStream.decoder.class.name\":\"" + MESSAGE_DECODER_CLASS + "\","
+ + " \"stream.fakeStream.consumer.factory.class.name\":\"" + CONSUMER_FACTORY_CLASS_FORMATTER + "\","
+ + " \"stream.fakeStream.decoder.class.name\":\"" + MESSAGE_DECODER_CLASS_FORMATTER + "\","
Review comment:
Just embed the "%s" directly here?
```java
public static final String CONSUMER_FACTORY_CLASS_FORMATTER = "%s";
public static final String MESSAGE_DECODER_CLASS_FORMATTER = "%s";
```
looks weird
--
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] dongxiaoman commented on pull request #8364: [WIP] Refactor Segment creation to use TransformPipeline too
Posted by GitBox <gi...@apache.org>.
dongxiaoman commented on pull request #8364:
URL: https://github.com/apache/pinot/pull/8364#issuecomment-1071514091
cc @npawar The PR for the follow up
--
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] richardstartin commented on a change in pull request #8364: [WIP] Refactor Segment creation to use TransformPipeline too
Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #8364:
URL: https://github.com/apache/pinot/pull/8364#discussion_r829480673
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentIndexCreationDriverImpl.java
##########
@@ -212,40 +209,23 @@ public void build()
_recordReader.rewind();
LOGGER.info("Start building IndexCreator!");
GenericRow reuse = new GenericRow();
+ TransformPipeline.Result reusedResult = new TransformPipeline.Result();
while (_recordReader.hasNext()) {
long recordReadStartTime = System.currentTimeMillis();
long recordReadStopTime;
long indexStopTime;
reuse.clear();
GenericRow decodedRow = _recordReader.next(reuse);
- if (_complexTypeTransformer != null) {
- // TODO: consolidate complex type transformer into composite type transformer
- decodedRow = _complexTypeTransformer.transform(decodedRow);
- }
- if (decodedRow.getValue(GenericRow.MULTIPLE_RECORDS_KEY) != null) {
- recordReadStopTime = System.currentTimeMillis();
- _totalRecordReadTime += (recordReadStopTime - recordReadStartTime);
- for (Object singleRow : (Collection) decodedRow.getValue(GenericRow.MULTIPLE_RECORDS_KEY)) {
- recordReadStartTime = System.currentTimeMillis();
- GenericRow transformedRow = _recordTransformer.transform((GenericRow) singleRow);
- recordReadStopTime = System.currentTimeMillis();
- _totalRecordReadTime += (recordReadStopTime - recordReadStartTime);
- if (transformedRow != null && IngestionUtils.shouldIngestRow(transformedRow)) {
- _indexCreator.indexRow(transformedRow);
- indexStopTime = System.currentTimeMillis();
- _totalIndexTime += (indexStopTime - recordReadStopTime);
- }
- }
- } else {
- GenericRow transformedRow = _recordTransformer.transform(decodedRow);
- recordReadStopTime = System.currentTimeMillis();
- _totalRecordReadTime += (recordReadStopTime - recordReadStartTime);
- if (transformedRow != null && IngestionUtils.shouldIngestRow(transformedRow)) {
- _indexCreator.indexRow(transformedRow);
- indexStopTime = System.currentTimeMillis();
- _totalIndexTime += (indexStopTime - recordReadStopTime);
- }
+ recordReadStartTime = System.currentTimeMillis();
+ _transformPipeline.processRow(decodedRow, reusedResult);
Review comment:
👍🏻
--
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] dongxiaoman commented on a change in pull request #8364: [WIP] Refactor Segment creation to use TransformPipeline too
Posted by GitBox <gi...@apache.org>.
dongxiaoman commented on a change in pull request #8364:
URL: https://github.com/apache/pinot/pull/8364#discussion_r829506556
##########
File path: pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/creator/TransformPipelineTest.java
##########
@@ -25,18 +25,16 @@
import org.testng.Assert;
import org.testng.annotations.Test;
-import static org.apache.pinot.core.data.manager.realtime.Fixtures.*;
Review comment:
I think this is because originally I did not use `Fixtures.createSchema()` in code?
After the refactor the IntelliJ editor tries to remove this
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/Fixtures.java
##########
@@ -35,8 +33,8 @@ private Fixtures() {
public static final int MAX_ROWS_IN_SEGMENT = 250000;
public static final long MAX_TIME_FOR_SEGMENT_CLOSE_MS = 64368000L;
public static final String TOPIC_NAME = "someTopic";
- public static final String CONSUMER_FACTORY_CLASS = FakeStreamConsumerFactory.class.getName();
- public static final String MESSAGE_DECODER_CLASS = FakeStreamMessageDecoder.class.getName();
+ public static final String CONSUMER_FACTORY_CLASS = "some.consumer.factory.class";
+ public static final String MESSAGE_DECODER_CLASS = "some.message.decoder.class";
Review comment:
This is because I moved the classes from `pinot-core` to `pinot-segment-local`, and the TransformPipeline does not care about those two properties.
Somehow it turns out these two are used by the tests in `pinot-core` too.
Now I have changed the Fixtures to take two class names, so it can be customized properly
##########
File path: pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/creator/TransformPipelineTest.java
##########
@@ -25,18 +25,16 @@
import org.testng.Assert;
import org.testng.annotations.Test;
-import static org.apache.pinot.core.data.manager.realtime.Fixtures.*;
Review comment:
fair enough. Changed 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] Jackie-Jiang merged pull request #8364: Refactor Segment creation to use TransformPipeline too
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged pull request #8364:
URL: https://github.com/apache/pinot/pull/8364
--
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] richardstartin commented on a change in pull request #8364: Refactor Segment creation to use TransformPipeline too
Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #8364:
URL: https://github.com/apache/pinot/pull/8364#discussion_r829539859
##########
File path: pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/creator/TransformPipelineTest.java
##########
@@ -25,18 +25,16 @@
import org.testng.Assert;
import org.testng.annotations.Test;
-import static org.apache.pinot.core.data.manager.realtime.Fixtures.*;
Review comment:
I'm not too keen on automatic IDE actions, this and others are completely unrelated to the change in this PR which will clutter up the git history
--
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 change in pull request #8364: Refactor Segment creation to use TransformPipeline too
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #8364:
URL: https://github.com/apache/pinot/pull/8364#discussion_r830269326
##########
File path: pinot-core/src/test/java/org/apache/pinot/core/data/manager/realtime/LLRealtimeSegmentDataManagerTest.java
##########
@@ -58,10 +61,9 @@
import org.testng.annotations.BeforeClass;
import org.testng.annotations.Test;
-import static org.apache.pinot.core.data.manager.realtime.Fixtures.MAX_ROWS_IN_SEGMENT;
-import static org.apache.pinot.core.data.manager.realtime.Fixtures.MAX_TIME_FOR_SEGMENT_CLOSE_MS;
-import static org.apache.pinot.core.data.manager.realtime.Fixtures.createSchema;
-import static org.apache.pinot.core.data.manager.realtime.Fixtures.createTableConfig;
+import static org.apache.pinot.segment.local.segment.creator.Fixtures.MAX_ROWS_IN_SEGMENT;
+import static org.apache.pinot.segment.local.segment.creator.Fixtures.MAX_TIME_FOR_SEGMENT_CLOSE_MS;
+import static org.apache.pinot.segment.local.segment.creator.Fixtures.createSchema;
Review comment:
（minor) Removing the static import (especially the function)
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/TransformPipeline.java
##########
@@ -38,6 +38,10 @@
private final RecordTransformer _recordTransformer;
private final ComplexTypeTransformer _complexTypeTransformer;
+ public TransformPipeline(RecordTransformer recordTransformer, ComplexTypeTransformer complexTypeTransformer) {
Review comment:
Add some javadoc, and also annotate `complexTypeTransformer` as `Nullable`
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/TransformPipeline.java
##########
@@ -38,6 +38,10 @@
private final RecordTransformer _recordTransformer;
private final ComplexTypeTransformer _complexTypeTransformer;
+ public TransformPipeline(RecordTransformer recordTransformer, ComplexTypeTransformer complexTypeTransformer) {
+ _recordTransformer = recordTransformer;
+ _complexTypeTransformer = complexTypeTransformer;
+ }
Review comment:
(minor) Add an empty line after this
##########
File path: pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/creator/TransformPipelineTest.java
##########
@@ -25,18 +25,16 @@
import org.testng.Assert;
import org.testng.annotations.Test;
-import static org.apache.pinot.core.data.manager.realtime.Fixtures.*;
Review comment:
I'd suggest removing the static import especially on function usage though
--
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] dongxiaoman commented on pull request #8364: [WIP] Refactor Segment creation to use TransformPipeline too
Posted by GitBox <gi...@apache.org>.
dongxiaoman commented on pull request #8364:
URL: https://github.com/apache/pinot/pull/8364#issuecomment-1071514091
cc @npawar The PR for the follow up
--
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 #8364: Refactor Segment creation to use TransformPipeline too
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #8364:
URL: https://github.com/apache/pinot/pull/8364#issuecomment-1071540686
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8364?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 [#8364](https://codecov.io/gh/apache/pinot/pull/8364?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a9e0059) into [master](https://codecov.io/gh/apache/pinot/commit/7c54ca6271e10eba8a7267ab8737d91ecb42550d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7c54ca6) will **increase** coverage by `0.26%`.
> The diff coverage is `34.45%`.
> :exclamation: Current head a9e0059 differs from pull request most recent head fc6db35. Consider uploading reports for the commit fc6db35 to get more accurate results
```diff
@@ Coverage Diff @@
## master #8364 +/- ##
============================================
+ Coverage 69.48% 69.75% +0.26%
- Complexity 4251 4273 +22
============================================
Files 1637 1640 +3
Lines 85796 85933 +137
Branches 12906 12913 +7
============================================
+ Hits 59616 59942 +326
+ Misses 22020 21832 -188
+ Partials 4160 4159 -1
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `28.90% <0.00%> (?)` | |
| integration2 | `?` | |
| unittests1 | `66.98% <78.46%> (+0.04%)` | :arrow_up: |
| unittests2 | `14.21% <0.00%> (+<0.01%)` | :arrow_up: |
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/8364?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...manager/realtime/LLRealtimeSegmentDataManager.java](https://codecov.io/gh/apache/pinot/pull/8364/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvTExSZWFsdGltZVNlZ21lbnREYXRhTWFuYWdlci5qYXZh) | `69.57% <ø> (-0.14%)` | :arrow_down: |
| [...gin/stream/kinesis/server/KinesisDataProducer.java](https://codecov.io/gh/apache/pinot/pull/8364/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-cGlub3QtcGx1Z2lucy9waW5vdC1zdHJlYW0taW5nZXN0aW9uL3Bpbm90LWtpbmVzaXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9zdHJlYW0va2luZXNpcy9zZXJ2ZXIvS2luZXNpc0RhdGFQcm9kdWNlci5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...eam/kinesis/server/KinesisDataServerStartable.java](https://codecov.io/gh/apache/pinot/pull/8364/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-cGlub3QtcGx1Z2lucy9waW5vdC1zdHJlYW0taW5nZXN0aW9uL3Bpbm90LWtpbmVzaXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9zdHJlYW0va2luZXNpcy9zZXJ2ZXIvS2luZXNpc0RhdGFTZXJ2ZXJTdGFydGFibGUuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...he/pinot/segment/local/utils/TableConfigUtils.java](https://codecov.io/gh/apache/pinot/pull/8364/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91dGlscy9UYWJsZUNvbmZpZ1V0aWxzLmphdmE=) | `65.34% <30.00%> (-0.80%)` | :arrow_down: |
| [...ache/pinot/segment/local/utils/IngestionUtils.java](https://codecov.io/gh/apache/pinot/pull/8364/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91dGlscy9Jbmdlc3Rpb25VdGlscy5qYXZh) | `25.00% <66.66%> (+5.74%)` | :arrow_up: |
| [...ocal/recordtransformer/ComplexTypeTransformer.java](https://codecov.io/gh/apache/pinot/pull/8364/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=) | `58.57% <78.57%> (+2.89%)` | :arrow_up: |
| [.../pinot/segment/local/segment/creator/Fixtures.java](https://codecov.io/gh/apache/pinot/pull/8364/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvRml4dHVyZXMuamF2YQ==) | `100.00% <100.00%> (ø)` | |
| [...creator/RecordReaderSegmentCreationDataSource.java](https://codecov.io/gh/apache/pinot/pull/8364/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvUmVjb3JkUmVhZGVyU2VnbWVudENyZWF0aW9uRGF0YVNvdXJjZS5qYXZh) | `75.86% <100.00%> (+4.80%)` | :arrow_up: |
| [...gment/local/segment/creator/TransformPipeline.java](https://codecov.io/gh/apache/pinot/pull/8364/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvVHJhbnNmb3JtUGlwZWxpbmUuamF2YQ==) | `91.42% <100.00%> (ø)` | |
| [...t/creator/impl/SegmentIndexCreationDriverImpl.java](https://codecov.io/gh/apache/pinot/pull/8364/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9TZWdtZW50SW5kZXhDcmVhdGlvbkRyaXZlckltcGwuamF2YQ==) | `84.75% <100.00%> (+1.14%)` | :arrow_up: |
| ... and [177 more](https://codecov.io/gh/apache/pinot/pull/8364/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/8364?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/8364?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 [7c54ca6...fc6db35](https://codecov.io/gh/apache/pinot/pull/8364?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] dongxiaoman commented on a change in pull request #8364: [WIP] Refactor Segment creation to use TransformPipeline too
Posted by GitBox <gi...@apache.org>.
dongxiaoman commented on a change in pull request #8364:
URL: https://github.com/apache/pinot/pull/8364#discussion_r829522463
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/Fixtures.java
##########
@@ -35,8 +33,8 @@ private Fixtures() {
public static final int MAX_ROWS_IN_SEGMENT = 250000;
public static final long MAX_TIME_FOR_SEGMENT_CLOSE_MS = 64368000L;
public static final String TOPIC_NAME = "someTopic";
- public static final String CONSUMER_FACTORY_CLASS = FakeStreamConsumerFactory.class.getName();
- public static final String MESSAGE_DECODER_CLASS = FakeStreamMessageDecoder.class.getName();
+ public static final String CONSUMER_FACTORY_CLASS = "some.consumer.factory.class";
+ public static final String MESSAGE_DECODER_CLASS = "some.message.decoder.class";
Review comment:
This is because I moved the classes from `pinot-core` to `pinot-segment-local`, and the TransformPipeline does not care about those two properties.
Somehow it turns out these two are used by the tests in `pinot-core` too.
Now I have changed the Fixtures to take two class names, so it can be customized properly
--
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] richardstartin commented on a change in pull request #8364: [WIP] Refactor Segment creation to use TransformPipeline too
Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #8364:
URL: https://github.com/apache/pinot/pull/8364#discussion_r829481022
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/Fixtures.java
##########
@@ -35,8 +33,8 @@ private Fixtures() {
public static final int MAX_ROWS_IN_SEGMENT = 250000;
public static final long MAX_TIME_FOR_SEGMENT_CLOSE_MS = 64368000L;
public static final String TOPIC_NAME = "someTopic";
- public static final String CONSUMER_FACTORY_CLASS = FakeStreamConsumerFactory.class.getName();
- public static final String MESSAGE_DECODER_CLASS = FakeStreamMessageDecoder.class.getName();
+ public static final String CONSUMER_FACTORY_CLASS = "some.consumer.factory.class";
+ public static final String MESSAGE_DECODER_CLASS = "some.message.decoder.class";
Review comment:
why does this need to change?
--
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 #8364: Refactor Segment creation to use TransformPipeline too
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #8364:
URL: https://github.com/apache/pinot/pull/8364#issuecomment-1071540686
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8364?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 [#8364](https://codecov.io/gh/apache/pinot/pull/8364?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a9e0059) into [master](https://codecov.io/gh/apache/pinot/commit/7c54ca6271e10eba8a7267ab8737d91ecb42550d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7c54ca6) will **increase** coverage by `0.26%`.
> The diff coverage is `34.45%`.
> :exclamation: Current head a9e0059 differs from pull request most recent head fc6db35. Consider uploading reports for the commit fc6db35 to get more accurate results
```diff
@@ Coverage Diff @@
## master #8364 +/- ##
============================================
+ Coverage 69.48% 69.75% +0.26%
- Complexity 4251 4273 +22
============================================
Files 1637 1640 +3
Lines 85796 85933 +137
Branches 12906 12913 +7
============================================
+ Hits 59616 59942 +326
+ Misses 22020 21832 -188
+ Partials 4160 4159 -1
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `28.90% <0.00%> (?)` | |
| integration2 | `?` | |
| unittests1 | `66.98% <78.46%> (+0.04%)` | :arrow_up: |
| unittests2 | `14.21% <0.00%> (+<0.01%)` | :arrow_up: |
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/8364?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...manager/realtime/LLRealtimeSegmentDataManager.java](https://codecov.io/gh/apache/pinot/pull/8364/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvTExSZWFsdGltZVNlZ21lbnREYXRhTWFuYWdlci5qYXZh) | `69.57% <ø> (-0.14%)` | :arrow_down: |
| [...gin/stream/kinesis/server/KinesisDataProducer.java](https://codecov.io/gh/apache/pinot/pull/8364/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-cGlub3QtcGx1Z2lucy9waW5vdC1zdHJlYW0taW5nZXN0aW9uL3Bpbm90LWtpbmVzaXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9zdHJlYW0va2luZXNpcy9zZXJ2ZXIvS2luZXNpc0RhdGFQcm9kdWNlci5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...eam/kinesis/server/KinesisDataServerStartable.java](https://codecov.io/gh/apache/pinot/pull/8364/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-cGlub3QtcGx1Z2lucy9waW5vdC1zdHJlYW0taW5nZXN0aW9uL3Bpbm90LWtpbmVzaXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9zdHJlYW0va2luZXNpcy9zZXJ2ZXIvS2luZXNpc0RhdGFTZXJ2ZXJTdGFydGFibGUuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...he/pinot/segment/local/utils/TableConfigUtils.java](https://codecov.io/gh/apache/pinot/pull/8364/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91dGlscy9UYWJsZUNvbmZpZ1V0aWxzLmphdmE=) | `65.34% <30.00%> (-0.80%)` | :arrow_down: |
| [...ache/pinot/segment/local/utils/IngestionUtils.java](https://codecov.io/gh/apache/pinot/pull/8364/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91dGlscy9Jbmdlc3Rpb25VdGlscy5qYXZh) | `25.00% <66.66%> (+5.74%)` | :arrow_up: |
| [...ocal/recordtransformer/ComplexTypeTransformer.java](https://codecov.io/gh/apache/pinot/pull/8364/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=) | `58.57% <78.57%> (+2.89%)` | :arrow_up: |
| [.../pinot/segment/local/segment/creator/Fixtures.java](https://codecov.io/gh/apache/pinot/pull/8364/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvRml4dHVyZXMuamF2YQ==) | `100.00% <100.00%> (ø)` | |
| [...creator/RecordReaderSegmentCreationDataSource.java](https://codecov.io/gh/apache/pinot/pull/8364/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvUmVjb3JkUmVhZGVyU2VnbWVudENyZWF0aW9uRGF0YVNvdXJjZS5qYXZh) | `75.86% <100.00%> (+4.80%)` | :arrow_up: |
| [...gment/local/segment/creator/TransformPipeline.java](https://codecov.io/gh/apache/pinot/pull/8364/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvVHJhbnNmb3JtUGlwZWxpbmUuamF2YQ==) | `91.42% <100.00%> (ø)` | |
| [...t/creator/impl/SegmentIndexCreationDriverImpl.java](https://codecov.io/gh/apache/pinot/pull/8364/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9TZWdtZW50SW5kZXhDcmVhdGlvbkRyaXZlckltcGwuamF2YQ==) | `84.75% <100.00%> (+1.14%)` | :arrow_up: |
| ... and [177 more](https://codecov.io/gh/apache/pinot/pull/8364/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/8364?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/8364?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 [7c54ca6...fc6db35](https://codecov.io/gh/apache/pinot/pull/8364?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] richardstartin commented on a change in pull request #8364: [WIP] Refactor Segment creation to use TransformPipeline too
Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #8364:
URL: https://github.com/apache/pinot/pull/8364#discussion_r829480330
##########
File path: pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/creator/TransformPipelineTest.java
##########
@@ -25,18 +25,16 @@
import org.testng.Assert;
import org.testng.annotations.Test;
-import static org.apache.pinot.core.data.manager.realtime.Fixtures.*;
Review comment:
removing this static import doesn't improve this class
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentIndexCreationDriverImpl.java
##########
@@ -212,40 +209,23 @@ public void build()
_recordReader.rewind();
LOGGER.info("Start building IndexCreator!");
GenericRow reuse = new GenericRow();
+ TransformPipeline.Result reusedResult = new TransformPipeline.Result();
while (_recordReader.hasNext()) {
long recordReadStartTime = System.currentTimeMillis();
long recordReadStopTime;
long indexStopTime;
reuse.clear();
GenericRow decodedRow = _recordReader.next(reuse);
- if (_complexTypeTransformer != null) {
- // TODO: consolidate complex type transformer into composite type transformer
- decodedRow = _complexTypeTransformer.transform(decodedRow);
- }
- if (decodedRow.getValue(GenericRow.MULTIPLE_RECORDS_KEY) != null) {
- recordReadStopTime = System.currentTimeMillis();
- _totalRecordReadTime += (recordReadStopTime - recordReadStartTime);
- for (Object singleRow : (Collection) decodedRow.getValue(GenericRow.MULTIPLE_RECORDS_KEY)) {
- recordReadStartTime = System.currentTimeMillis();
- GenericRow transformedRow = _recordTransformer.transform((GenericRow) singleRow);
- recordReadStopTime = System.currentTimeMillis();
- _totalRecordReadTime += (recordReadStopTime - recordReadStartTime);
- if (transformedRow != null && IngestionUtils.shouldIngestRow(transformedRow)) {
- _indexCreator.indexRow(transformedRow);
- indexStopTime = System.currentTimeMillis();
- _totalIndexTime += (indexStopTime - recordReadStopTime);
- }
- }
- } else {
- GenericRow transformedRow = _recordTransformer.transform(decodedRow);
- recordReadStopTime = System.currentTimeMillis();
- _totalRecordReadTime += (recordReadStopTime - recordReadStartTime);
- if (transformedRow != null && IngestionUtils.shouldIngestRow(transformedRow)) {
- _indexCreator.indexRow(transformedRow);
- indexStopTime = System.currentTimeMillis();
- _totalIndexTime += (indexStopTime - recordReadStopTime);
- }
+ recordReadStartTime = System.currentTimeMillis();
+ _transformPipeline.processRow(decodedRow, reusedResult);
Review comment:
👍🏻
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/Fixtures.java
##########
@@ -35,8 +33,8 @@ private Fixtures() {
public static final int MAX_ROWS_IN_SEGMENT = 250000;
public static final long MAX_TIME_FOR_SEGMENT_CLOSE_MS = 64368000L;
public static final String TOPIC_NAME = "someTopic";
- public static final String CONSUMER_FACTORY_CLASS = FakeStreamConsumerFactory.class.getName();
- public static final String MESSAGE_DECODER_CLASS = FakeStreamMessageDecoder.class.getName();
+ public static final String CONSUMER_FACTORY_CLASS = "some.consumer.factory.class";
+ public static final String MESSAGE_DECODER_CLASS = "some.message.decoder.class";
Review comment:
why does this need to change?
##########
File path: pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/creator/TransformPipelineTest.java
##########
@@ -25,18 +25,16 @@
import org.testng.Assert;
import org.testng.annotations.Test;
-import static org.apache.pinot.core.data.manager.realtime.Fixtures.*;
Review comment:
I'm not too keen on automatic IDE actions, this and others are completely unrelated to the change in this PR which will clutter up the git history
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/Fixtures.java
##########
@@ -67,8 +65,8 @@ private Fixtures() {
+ " \"stream.fakeStream.broker.list\":\"broker:7777\","
+ " \"stream.fakeStream.consumer.prop.auto.offset.reset\":\"smallest\","
+ " \"stream.fakeStream.consumer.type\":\"simple\","
- + " \"stream.fakeStream.consumer.factory.class.name\":\"" + CONSUMER_FACTORY_CLASS + "\","
- + " \"stream.fakeStream.decoder.class.name\":\"" + MESSAGE_DECODER_CLASS + "\","
+ + " \"stream.fakeStream.consumer.factory.class.name\":\"" + CONSUMER_FACTORY_CLASS_FORMATTER + "\","
+ + " \"stream.fakeStream.decoder.class.name\":\"" + MESSAGE_DECODER_CLASS_FORMATTER + "\","
Review comment:
Just embed the "%s" directly here?
```java
public static final String CONSUMER_FACTORY_CLASS_FORMATTER = "%s";
public static final String MESSAGE_DECODER_CLASS_FORMATTER = "%s";
```
looks weird
--
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