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/10 05:18:48 UTC
[GitHub] [pinot] klsince opened a new pull request, #8879: [Draft] extract sort fields into a temp file for better data locality for sorting
klsince opened a new pull request, #8879:
URL: https://github.com/apache/pinot/pull/8879
This PR tries to optimize the SegmentProcessorFramework that's used when to generate segments. Today, quicksort is conducted over the data file that's memory mapped. If the data file is huge, random reads lead to page faults that slow down sorting. This PR tries an idea to extract the sort fields into a temp file for better data locality for sorting, mainly for use cases where the input files are full of very wide rows but just sort on a single 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] klsince commented on pull request #8879: [Draft] extract sort fields into a temp file for better data locality for sorting
Posted by GitBox <gi...@apache.org>.
klsince commented on PR #8879:
URL: https://github.com/apache/pinot/pull/8879#issuecomment-1152542139
> For dedup mode the sort is over all dimensions, so will this help in that case?
Not much help for dedup or basically cases where many fields are used to sort. For dedup or rollup, will consider using hashmap w/o sorting.
If sorting with multi fields is anyway wanted, one quick though is to sort the data file in chunks (with current method of quicksort and an auxiliary index array), then copy the sorted chunks into temp files (one file per sorted chunk), then merge those temp files up for following processing. Compared with quicksort over large data file directly, this might reduce page faults, by tuning the chunk size and chunk number.
--
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 pull request #8879: [Draft] extract sort fields into a temp file for better data locality for sorting
Posted by GitBox <gi...@apache.org>.
richardstartin commented on PR #8879:
URL: https://github.com/apache/pinot/pull/8879#issuecomment-1152108665
For dedup mode the sort is over all dimensions, so will this help in that case?
--
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] kishoreg commented on pull request #8879: [Draft] extract sort fields into a temp file for better data locality for sorting
Posted by GitBox <gi...@apache.org>.
kishoreg commented on PR #8879:
URL: https://github.com/apache/pinot/pull/8879#issuecomment-1152868478
Another idea along the lines of what @richardstartin mentioned but it does not have to be dictionary encoded and potentially avoid the need to sort the file on disk and sort complexity reduced from a function of num_rows to cardinality of the sort column
In the first pass,
create a map of <key, bitmap> sort this map/array on key.
you can now create a sorted doc id array (this is also optional)
you can simply create a record reader and pass the hashmap that will dictate the reading order.
We do something like this in real-time segment converter when we flush real-time to disk.
--
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 pull request #8879: [Draft] extract sort fields into a temp file for better data locality for sorting
Posted by GitBox <gi...@apache.org>.
richardstartin commented on PR #8879:
URL: https://github.com/apache/pinot/pull/8879#issuecomment-1152554035
If you have dictionaries for the sort dimension, you could sometimes (whenever the sort key would fit into 64 bits) define a sort key the same way we define a group key in the group by. Then you could pass through the file once, extract the sort dimensions, compute the sort key, store it in an array, then sort that array, updating the sortedRowIds array as necessary. You could consider using radix sort here, which might be faster than quicksort. This way, you only read the file once during the sort, and read it sequentially, which will reduce the number of page faults drastically.
--
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] zhtaoxiang commented on a diff in pull request #8879: [Draft] extract sort fields into a temp file for better data locality for sorting
Posted by GitBox <gi...@apache.org>.
zhtaoxiang commented on code in PR #8879:
URL: https://github.com/apache/pinot/pull/8879#discussion_r895231923
##########
pinot-core/src/main/java/org/apache/pinot/core/segment/processing/genericrow/GenericRowSerializer.java:
##########
@@ -237,4 +173,72 @@ public byte[] serialize(GenericRow row) {
return serializedBytes;
}
+
+ public void serializeField(GenericRow row, ByteBuffer byteBuffer, int fieldIndex) {
+ Object value = row.getValue(_fieldNames[fieldIndex]);
+
+ if (_isSingleValueFields[fieldIndex]) {
+ switch (_storedTypes[fieldIndex]) {
+ case INT:
+ byteBuffer.putInt((int) value);
+ break;
+ case LONG:
+ byteBuffer.putLong((long) value);
+ break;
+ case FLOAT:
+ byteBuffer.putFloat((float) value);
+ break;
+ case DOUBLE:
+ byteBuffer.putDouble((double) value);
+ break;
+ case STRING:
+ byte[] stringBytes = (byte[]) _stringBytes[fieldIndex];
+ byteBuffer.putInt(stringBytes.length);
Review Comment:
this line throws NPE during the test.
--
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 diff in pull request #8879: [Draft] extract sort fields into a temp file for better data locality for sorting
Posted by GitBox <gi...@apache.org>.
richardstartin commented on code in PR #8879:
URL: https://github.com/apache/pinot/pull/8879#discussion_r894281662
##########
pinot-core/src/main/java/org/apache/pinot/core/segment/processing/genericrow/GenericRowFileReader.java:
##########
@@ -77,6 +133,13 @@ public void read(int rowId, GenericRow buffer) {
* Compares the rows at the given row ids. Only compare the values for the sort fields.
*/
public int compare(int rowId1, int rowId2) {
+ // Sort with the extracted sort fields for better data locality.
+ if (_sortFieldsOffsetBuffer != null) {
+ long offset1 = _sortFieldsOffsetBuffer.getLong((long) rowId1 << 3); // rowId1 * Long.BYTES
+ long offset2 = _sortFieldsOffsetBuffer.getLong((long) rowId2 << 3); // rowId2 * Long.BYTES
Review Comment:
because `Long.BYTES` is a constant (it is `static final`) `* Long.BYTES` will get strength reduced to `<< 3` anyway.
--
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 #8879: [Draft] extract sort fields into a temp file for better data locality for sorting
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #8879:
URL: https://github.com/apache/pinot/pull/8879#issuecomment-1151974962
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8879?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 [#8879](https://codecov.io/gh/apache/pinot/pull/8879?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c04f88a) into [master](https://codecov.io/gh/apache/pinot/commit/d1d2ddbc116f6dddbefdf1e69dd409434eaae0e4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d1d2ddb) will **decrease** coverage by `54.36%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #8879 +/- ##
=============================================
- Coverage 69.80% 15.44% -54.37%
+ Complexity 4659 170 -4489
=============================================
Files 1741 1758 +17
Lines 91476 92087 +611
Branches 13677 13784 +107
=============================================
- Hits 63854 14220 -49634
- Misses 23199 76838 +53639
+ Partials 4423 1029 -3394
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `15.44% <0.00%> (+1.30%)` | :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/8879?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../org/apache/pinot/core/common/MinionConstants.java](https://codecov.io/gh/apache/pinot/pull/8879/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9jb21tb24vTWluaW9uQ29uc3RhbnRzLmphdmE=) | `0.00% <ø> (ø)` | |
| [...t/processing/framework/SegmentProcessorConfig.java](https://codecov.io/gh/apache/pinot/pull/8879/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zZWdtZW50L3Byb2Nlc3NpbmcvZnJhbWV3b3JrL1NlZ21lbnRQcm9jZXNzb3JDb25maWcuamF2YQ==) | `0.00% <0.00%> (-95.66%)` | :arrow_down: |
| [.../processing/genericrow/GenericRowDeserializer.java](https://codecov.io/gh/apache/pinot/pull/8879/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zZWdtZW50L3Byb2Nlc3NpbmcvZ2VuZXJpY3Jvdy9HZW5lcmljUm93RGVzZXJpYWxpemVyLmphdmE=) | `0.00% <0.00%> (-89.42%)` | :arrow_down: |
| [...t/processing/genericrow/GenericRowFileManager.java](https://codecov.io/gh/apache/pinot/pull/8879/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zZWdtZW50L3Byb2Nlc3NpbmcvZ2VuZXJpY3Jvdy9HZW5lcmljUm93RmlsZU1hbmFnZXIuamF2YQ==) | `0.00% <0.00%> (-84.85%)` | :arrow_down: |
| [...nt/processing/genericrow/GenericRowFileReader.java](https://codecov.io/gh/apache/pinot/pull/8879/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zZWdtZW50L3Byb2Nlc3NpbmcvZ2VuZXJpY3Jvdy9HZW5lcmljUm93RmlsZVJlYWRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...nt/processing/genericrow/GenericRowSerializer.java](https://codecov.io/gh/apache/pinot/pull/8879/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zZWdtZW50L3Byb2Nlc3NpbmcvZ2VuZXJpY3Jvdy9HZW5lcmljUm93U2VyaWFsaXplci5qYXZh) | `0.00% <0.00%> (-93.28%)` | :arrow_down: |
| [.../core/segment/processing/mapper/SegmentMapper.java](https://codecov.io/gh/apache/pinot/pull/8879/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zZWdtZW50L3Byb2Nlc3NpbmcvbWFwcGVyL1NlZ21lbnRNYXBwZXIuamF2YQ==) | `0.00% <0.00%> (-88.14%)` | :arrow_down: |
| [...src/main/java/org/apache/pinot/sql/FilterKind.java](https://codecov.io/gh/apache/pinot/pull/8879/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/8879/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/8879/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: |
| ... and [1436 more](https://codecov.io/gh/apache/pinot/pull/8879/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/8879?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/8879?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 [d1d2ddb...c04f88a](https://codecov.io/gh/apache/pinot/pull/8879?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