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/19 18:50:04 UTC
[GitHub] [pinot] walterddr opened a new pull request, #8926: reduce positioning operation to single operation
walterddr opened a new pull request, #8926:
URL: https://github.com/apache/pinot/pull/8926
instead of positioning and seek to read value. This PR reduce the operation into direct access.
--
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 #8926: reduce positioning operation to single operation
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #8926:
URL: https://github.com/apache/pinot/pull/8926#issuecomment-1160720823
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8926?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 [#8926](https://codecov.io/gh/apache/pinot/pull/8926?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1f7f902) into [master](https://codecov.io/gh/apache/pinot/commit/eb5835fed9cf692e58ec643183ffe16953c95188?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (eb5835f) will **decrease** coverage by `54.84%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #8926 +/- ##
=============================================
- Coverage 69.69% 14.85% -54.85%
+ Complexity 4708 170 -4538
=============================================
Files 1813 1766 -47
Lines 94522 92446 -2076
Branches 14111 13884 -227
=============================================
- Hits 65881 13729 -52152
- Misses 24034 77727 +53693
+ Partials 4607 990 -3617
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `14.85% <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/8926?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...che/pinot/core/common/datablock/BaseDataBlock.java](https://codecov.io/gh/apache/pinot/pull/8926/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9jb21tb24vZGF0YWJsb2NrL0Jhc2VEYXRhQmxvY2suamF2YQ==) | `0.00% <0.00%> (-73.86%)` | :arrow_down: |
| [...pinot/core/common/datablock/ColumnarDataBlock.java](https://codecov.io/gh/apache/pinot/pull/8926/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9jb21tb24vZGF0YWJsb2NrL0NvbHVtbmFyRGF0YUJsb2NrLmphdmE=) | `0.00% <0.00%> (-58.63%)` | :arrow_down: |
| [...che/pinot/core/common/datablock/MetadataBlock.java](https://codecov.io/gh/apache/pinot/pull/8926/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9jb21tb24vZGF0YWJsb2NrL01ldGFkYXRhQmxvY2suamF2YQ==) | `0.00% <ø> (-55.56%)` | :arrow_down: |
| [...ache/pinot/core/common/datablock/RowDataBlock.java](https://codecov.io/gh/apache/pinot/pull/8926/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9jb21tb24vZGF0YWJsb2NrL1Jvd0RhdGFCbG9jay5qYXZh) | `0.00% <0.00%> (-68.43%)` | :arrow_down: |
| [...che/pinot/core/common/datatable/BaseDataTable.java](https://codecov.io/gh/apache/pinot/pull/8926/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9jb21tb24vZGF0YXRhYmxlL0Jhc2VEYXRhVGFibGUuamF2YQ==) | `0.00% <0.00%> (-79.72%)` | :arrow_down: |
| [...src/main/java/org/apache/pinot/sql/FilterKind.java](https://codecov.io/gh/apache/pinot/pull/8926/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvRmlsdGVyS2luZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ain/java/org/apache/pinot/core/data/table/Key.java](https://codecov.io/gh/apache/pinot/pull/8926/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL0tleS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...in/java/org/apache/pinot/spi/utils/StringUtil.java](https://codecov.io/gh/apache/pinot/pull/8926/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/8926/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/core/data/table/Record.java](https://codecov.io/gh/apache/pinot/pull/8926/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1JlY29yZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1382 more](https://codecov.io/gh/apache/pinot/pull/8926/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/8926?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/8926?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 [eb5835f...1f7f902](https://codecov.io/gh/apache/pinot/pull/8926?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 commented on a diff in pull request #8926: reduce positioning operation to single operation
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #8926:
URL: https://github.com/apache/pinot/pull/8926#discussion_r902027627
##########
pinot-core/src/main/java/org/apache/pinot/core/common/datablock/BaseDataBlock.java:
##########
@@ -221,7 +221,7 @@ public BaseDataBlock(ByteBuffer byteBuffer)
* @param colId column ID
* @return the length to extract from variable size buffer.
*/
- protected abstract int positionCursorInVariableBuffer(int rowId, int colId);
+ protected abstract int computePositionCursorInVariableBuffer(int rowId, int colId);
Review Comment:
yeah and return the length to read out of the var-byte buffer. let me rethink the naming
--
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 #8926: reduce positioning operation to single operation
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #8926:
URL: https://github.com/apache/pinot/pull/8926#discussion_r901961005
##########
pinot-core/src/main/java/org/apache/pinot/core/common/datablock/BaseDataBlock.java:
##########
@@ -221,7 +221,7 @@ public BaseDataBlock(ByteBuffer byteBuffer)
* @param colId column ID
* @return the length to extract from variable size buffer.
*/
- protected abstract int positionCursorInVariableBuffer(int rowId, int colId);
+ protected abstract int computePositionCursorInVariableBuffer(int rowId, int colId);
Review Comment:
Don't change this method name. It does not return the offset, but actually position the cursor
##########
pinot-core/src/main/java/org/apache/pinot/core/common/datatable/BaseDataTable.java:
##########
@@ -158,26 +158,22 @@ public int getNumberOfRows() {
@Override
public int getInt(int rowId, int colId) {
- _fixedSizeData.position(rowId * _rowSizeInBytes + _columnOffsets[colId]);
- return _fixedSizeData.getInt();
+ return _fixedSizeData.getInt(rowId * _rowSizeInBytes + _columnOffsets[colId]);
}
@Override
public long getLong(int rowId, int colId) {
- _fixedSizeData.position(rowId * _rowSizeInBytes + _columnOffsets[colId]);
- return _fixedSizeData.getLong();
+ return _fixedSizeData.getLong(rowId * _rowSizeInBytes + _columnOffsets[colId]);
}
@Override
public float getFloat(int rowId, int colId) {
- _fixedSizeData.position(rowId * _rowSizeInBytes + _columnOffsets[colId]);
- return _fixedSizeData.getFloat();
+ return _fixedSizeData.getFloat(rowId * _rowSizeInBytes + _columnOffsets[colId]);
}
@Override
public double getDouble(int rowId, int colId) {
- _fixedSizeData.position(rowId * _rowSizeInBytes + _columnOffsets[colId]);
- return _fixedSizeData.getDouble();
+ return _fixedSizeData.getDouble(rowId * _rowSizeInBytes + _columnOffsets[colId]);
}
@Override
Review Comment:
Let's also fix the `getString()`
##########
pinot-core/src/main/java/org/apache/pinot/core/common/datablock/BaseDataBlock.java:
##########
@@ -211,7 +211,7 @@ public BaseDataBlock(ByteBuffer byteBuffer)
* @param rowId row ID
* @param colId column ID
*/
- protected abstract void positionCursorInFixSizedBuffer(int rowId, int colId);
+ protected abstract int computePositionCursorInFixSizedBuffer(int rowId, int colId);
Review Comment:
Suggest renaming it to `getOffsetInFixedBuffer`
--
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] siddharthteotia commented on a diff in pull request #8926: reduce positioning operation to single operation
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #8926:
URL: https://github.com/apache/pinot/pull/8926#discussion_r901952028
##########
pinot-core/src/main/java/org/apache/pinot/core/common/datablock/BaseDataBlock.java:
##########
@@ -211,7 +211,7 @@ public BaseDataBlock(ByteBuffer byteBuffer)
* @param rowId row ID
* @param colId column ID
*/
- protected abstract void positionCursorInFixSizedBuffer(int rowId, int colId);
+ protected abstract int computePositionCursorInFixSizedBuffer(int rowId, int colId);
Review Comment:
(nit) update the jaavadoc to include the return value please ?
--
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 merged pull request #8926: reduce positioning operation to single operation
Posted by GitBox <gi...@apache.org>.
walterddr merged PR #8926:
URL: https://github.com/apache/pinot/pull/8926
--
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 a diff in pull request #8926: reduce positioning operation to single operation
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #8926:
URL: https://github.com/apache/pinot/pull/8926#discussion_r902027474
##########
pinot-core/src/main/java/org/apache/pinot/core/common/datatable/BaseDataTable.java:
##########
@@ -158,26 +158,22 @@ public int getNumberOfRows() {
@Override
public int getInt(int rowId, int colId) {
- _fixedSizeData.position(rowId * _rowSizeInBytes + _columnOffsets[colId]);
- return _fixedSizeData.getInt();
+ return _fixedSizeData.getInt(rowId * _rowSizeInBytes + _columnOffsets[colId]);
}
@Override
public long getLong(int rowId, int colId) {
- _fixedSizeData.position(rowId * _rowSizeInBytes + _columnOffsets[colId]);
- return _fixedSizeData.getLong();
+ return _fixedSizeData.getLong(rowId * _rowSizeInBytes + _columnOffsets[colId]);
}
@Override
public float getFloat(int rowId, int colId) {
- _fixedSizeData.position(rowId * _rowSizeInBytes + _columnOffsets[colId]);
- return _fixedSizeData.getFloat();
+ return _fixedSizeData.getFloat(rowId * _rowSizeInBytes + _columnOffsets[colId]);
}
@Override
public double getDouble(int rowId, int colId) {
- _fixedSizeData.position(rowId * _rowSizeInBytes + _columnOffsets[colId]);
- return _fixedSizeData.getDouble();
+ return _fixedSizeData.getDouble(rowId * _rowSizeInBytes + _columnOffsets[colId]);
}
@Override
Review Comment:
yeah dont know why I missed that.
--
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