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/06/08 06:08:41 UTC
[GitHub] [pinot] walterddr opened a new pull request, #8860: fix data table vs. block format issue
walterddr opened a new pull request, #8860:
URL: https://github.com/apache/pinot/pull/8860
previously multi-stage engine assumes no backward compatibility issue with existing pinot query server. however we are still depending on pinot-server to return `DataTableImplV3` byte format to intermediate multi-stage engine.
To avoid duplicate ser/de we decided to backward compatible support these old ser/de formats
- old 8-bytes floating point
also, this
- doesn't work with `OBJECT`/`BYTES_ARRAY` type
- doesn't support `BYTES` type as old bytes are converted into hexString and encoded as STRING column.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on pull request #8860: fix data table vs. block format issue
Posted by GitBox <gi...@apache.org>.
walterddr commented on PR #8860:
URL: https://github.com/apache/pinot/pull/8860#issuecomment-1150484456
> There are several inefficiency in the current data table implementation. I'd suggest making a V4 to address the inefficiency instead of making the new code follow the bad format.
>
> Things we want to improve:
>
> * Store float as 4 bytes
> * Do not serialize bytes to string
> * For each column, add an optional bitmap to store the nulls for the column
ok. in this case I would suggest we add FLOAT/BYTES to the ignore list and still merge the integration test PR.
--
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 #8860: fix data table vs. block format issue
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #8860:
URL: https://github.com/apache/pinot/pull/8860#issuecomment-1149527449
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8860?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 [#8860](https://codecov.io/gh/apache/pinot/pull/8860?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b681aa9) into [master](https://codecov.io/gh/apache/pinot/commit/81bda1d26a5c5173864e6edc3081e7135041f4f1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (81bda1d) will **decrease** coverage by `0.01%`.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## master #8860 +/- ##
============================================
- Coverage 15.48% 15.46% -0.02%
Complexity 168 168
============================================
Files 1756 1756
Lines 91680 91682 +2
Branches 13701 13701
============================================
- Hits 14195 14178 -17
- Misses 76453 76480 +27
+ Partials 1032 1024 -8
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests2 | `15.46% <100.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/8860?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...he/pinot/core/common/datatable/DataTableUtils.java](https://codecov.io/gh/apache/pinot/pull/8860/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9jb21tb24vZGF0YXRhYmxlL0RhdGFUYWJsZVV0aWxzLmphdmE=) | `0.00% <ø> (ø)` | |
| [...va/org/apache/pinot/query/runtime/QueryRunner.java](https://codecov.io/gh/apache/pinot/pull/8860/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-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9RdWVyeVJ1bm5lci5qYXZh) | `86.66% <ø> (ø)` | |
| [...e/pinot/query/runtime/blocks/DataBlockBuilder.java](https://codecov.io/gh/apache/pinot/pull/8860/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-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9ibG9ja3MvRGF0YUJsb2NrQnVpbGRlci5qYXZh) | `74.46% <100.00%> (+0.21%)` | :arrow_up: |
| [...che/pinot/query/runtime/blocks/DataBlockUtils.java](https://codecov.io/gh/apache/pinot/pull/8860/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-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9ibG9ja3MvRGF0YUJsb2NrVXRpbHMuamF2YQ==) | `80.95% <100.00%> (ø)` | |
| [...r/validation/RealtimeSegmentValidationManager.java](https://codecov.io/gh/apache/pinot/pull/8860/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci92YWxpZGF0aW9uL1JlYWx0aW1lU2VnbWVudFZhbGlkYXRpb25NYW5hZ2VyLmphdmE=) | `43.66% <0.00%> (-30.99%)` | :arrow_down: |
| [...controller/helix/core/minion/PinotTaskManager.java](https://codecov.io/gh/apache/pinot/pull/8860/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL21pbmlvbi9QaW5vdFRhc2tNYW5hZ2VyLmphdmE=) | `38.67% <0.00%> (-1.58%)` | :arrow_down: |
| [...ces/PinotSegmentUploadDownloadRestletResource.java](https://codecov.io/gh/apache/pinot/pull/8860/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90U2VnbWVudFVwbG9hZERvd25sb2FkUmVzdGxldFJlc291cmNlLmphdmE=) | `40.92% <0.00%> (+0.84%)` | :arrow_up: |
| [...lix/core/realtime/PinotRealtimeSegmentManager.java](https://codecov.io/gh/apache/pinot/pull/8860/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JlYWx0aW1lL1Bpbm90UmVhbHRpbWVTZWdtZW50TWFuYWdlci5qYXZh) | `81.15% <0.00%> (+3.14%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8860?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/8860?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 [81bda1d...b681aa9](https://codecov.io/gh/apache/pinot/pull/8860?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] walterddr closed pull request #8860: fix data table vs. block format issue
Posted by GitBox <gi...@apache.org>.
walterddr closed pull request #8860: fix data table vs. block format issue
URL: https://github.com/apache/pinot/pull/8860
--
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] walterddr commented on pull request #8860: fix data table vs. block format issue
Posted by GitBox <gi...@apache.org>.
walterddr commented on PR #8860:
URL: https://github.com/apache/pinot/pull/8860#issuecomment-1150534643
> > ok. in this case I would suggest we add FLOAT/BYTES to the ignore list and still merge the integration test PR.
>
> How much effort does it take to make the new engine work on a new data table format if we add one? The goal is to avoid spending a lot of effort for backward compatibility when we upgrade the new format. Since the engine is new added, it would be good to make it directly work on the latest format
no effort. as it is already assuming new format.
the problem is backward compatibility is not handled correctly when move from V2 to V3 --> the data format are still managed in a common Utils class. I will refactor our the common util class and make 2 versions one for V2, V3 and one for V4.
--
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 #8860: fix data table vs. block format issue
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on PR #8860:
URL: https://github.com/apache/pinot/pull/8860#issuecomment-1150532662
> ok. in this case I would suggest we add FLOAT/BYTES to the ignore list and still merge the integration test PR.
How much effort does it take to make the new engine work on a new data table format if we add one? The goal is to avoid spending a lot of effort for backward compatibility when we upgrade the new format. Since the engine is new added, it would be good to make it directly work on the latest format
--
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