You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "gortiz (via GitHub)" <gi...@apache.org> on 2025/08/14 10:24:12 UTC
[PR] Be sure block serialization can be interrupted [pinot]
gortiz opened a new pull request, #16595:
URL: https://github.com/apache/pinot/pull/16595
We have found issues cancelling queries while blocks are serialized. This is because the serialization process requires heavy computation whose cost is proportional to the length of the block, and there is no check while doing that on whether the thread has been interrupted or not.
The change is very simple: Divide the execution into two loops. The outer checks if the thread has been interrupted while the inner runs for a fixed number of rows. This reduces the cost of checking interruption while keeping the number of rows evaluated constant without checking.
This PR also improves the exception reported in two cases. Specifically, I found that the stack trace was lost when Exceptions are transformed into error blocks.
--
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
Re: [PR] Be sure block serialization can be interrupted [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz merged PR #16595:
URL: https://github.com/apache/pinot/pull/16595
--
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
Re: [PR] Be sure block serialization can be interrupted [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #16595:
URL: https://github.com/apache/pinot/pull/16595#discussion_r2276225282
##########
pinot-core/src/main/java/org/apache/pinot/core/common/datablock/DataBlockBuilder.java:
##########
@@ -258,195 +261,225 @@ private static void serializeColumnData(List<Object[]> columns, DataSchema dataS
// Single-value column
case INT: {
int nullPlaceholder = (int) storedType.getNullPlaceholder();
- for (int rowId = 0; rowId < numRows; rowId++) {
- Object value = column[rowId];
- if (value == null) {
- nullBitmap.add(rowId);
- fixedSize.putInt(nullPlaceholder);
- } else {
- fixedSize.putInt((int) value);
+ interruptableLoop(0, numRows, interruptableLoopStep, (start, end) -> {;
+ for (int rowId = start; rowId < end; rowId++) {
+ Object value = column[rowId];
+ if (value == null) {
+ nullBitmap.add(rowId);
+ fixedSize.putInt(nullPlaceholder);
+ } else {
+ fixedSize.putInt((int) value);
+ }
}
- }
+ });
break;
}
case LONG: {
long nullPlaceholder = (long) storedType.getNullPlaceholder();
- for (int rowId = 0; rowId < numRows; rowId++) {
- Object value = column[rowId];
- if (value == null) {
- nullBitmap.add(rowId);
- fixedSize.putLong(nullPlaceholder);
- } else {
- fixedSize.putLong((long) value);
+ interruptableLoop(0, numRows, interruptableLoopStep, (start, end) -> {
+ for (int rowId = start; rowId < end; rowId++) {
+ Object value = column[rowId];
+ if (value == null) {
+ nullBitmap.add(rowId);
+ fixedSize.putLong(nullPlaceholder);
+ } else {
+ fixedSize.putLong((long) value);
+ }
}
- }
+ });
break;
}
case FLOAT: {
float nullPlaceholder = (float) storedType.getNullPlaceholder();
- for (int rowId = 0; rowId < numRows; rowId++) {
- Object value = column[rowId];
- if (value == null) {
- nullBitmap.add(rowId);
- fixedSize.putFloat(nullPlaceholder);
- } else {
- fixedSize.putFloat((float) value);
+ interruptableLoop(0, numRows, interruptableLoopStep, (start, end) -> {
+ for (int rowId = start; rowId < end; rowId++) {
+ Object value = column[rowId];
+ if (value == null) {
+ nullBitmap.add(rowId);
+ fixedSize.putFloat(nullPlaceholder);
+ } else {
+ fixedSize.putFloat((float) value);
+ }
}
- }
+ });
break;
}
case DOUBLE: {
double nullPlaceholder = (double) storedType.getNullPlaceholder();
- for (int rowId = 0; rowId < numRows; rowId++) {
- Object value = column[rowId];
- if (value == null) {
- nullBitmap.add(rowId);
- fixedSize.putDouble(nullPlaceholder);
- } else {
- fixedSize.putDouble((double) value);
+ interruptableLoop(0, numRows, interruptableLoopStep, (start, end) -> {
+ for (int rowId = start; rowId < end; rowId++) {
+ Object value = column[rowId];
+ if (value == null) {
+ nullBitmap.add(rowId);
+ fixedSize.putDouble(nullPlaceholder);
+ } else {
+ fixedSize.putDouble((double) value);
+ }
}
- }
+ });
break;
}
case BIG_DECIMAL: {
BigDecimal nullPlaceholder = (BigDecimal) storedType.getNullPlaceholder();
- for (int rowId = 0; rowId < numRows; rowId++) {
- Object value = column[rowId];
- if (value == null) {
- nullBitmap.add(rowId);
- setColumn(fixedSize, varSize, nullPlaceholder);
- } else {
- setColumn(fixedSize, varSize, (BigDecimal) value);
+ interruptableLoop(0, numRows, interruptableLoopStep, (start, end) -> {
+ for (int rowId = 0; rowId < numRows; rowId++) {
Review Comment:
The BIG_DECIMAL case still uses the original loop bounds (0 to numRows) instead of the interruptable loop parameters (start to end), which defeats the purpose of the interruptable loop.
```suggestion
for (int rowId = start; rowId < end; rowId++) {
```
##########
pinot-core/src/main/java/org/apache/pinot/core/common/datablock/DataBlockBuilder.java:
##########
@@ -258,195 +261,225 @@ private static void serializeColumnData(List<Object[]> columns, DataSchema dataS
// Single-value column
case INT: {
int nullPlaceholder = (int) storedType.getNullPlaceholder();
- for (int rowId = 0; rowId < numRows; rowId++) {
- Object value = column[rowId];
- if (value == null) {
- nullBitmap.add(rowId);
- fixedSize.putInt(nullPlaceholder);
- } else {
- fixedSize.putInt((int) value);
+ interruptableLoop(0, numRows, interruptableLoopStep, (start, end) -> {;
Review Comment:
There is an extra semicolon after the opening brace of the lambda expression, which will cause a compilation error.
```suggestion
interruptableLoop(0, numRows, interruptableLoopStep, (start, end) -> {
```
##########
pinot-core/src/main/java/org/apache/pinot/core/common/datablock/DataBlockBuilder.java:
##########
@@ -622,4 +655,21 @@ private static void setNull(ByteBuffer fixedSize, PagedPinotOutputStream varSize
fixedSize.putInt(0);
varSize.writeInt(CustomObject.NULL_TYPE_VALUE);
}
+
+ /// Iterate using two loops.
+ /// The outer loop will iterate over a maximum of configurable step rows and check for the interruption flag,
+ /// calling the inner loop to process the rows without checking the interruption flag.
Review Comment:
Use standard Javadoc comments (/** */) instead of triple slash comments for method documentation.
```suggestion
/**
* Iterate using two loops.
* The outer loop will iterate over a maximum of configurable step rows and check for the interruption flag,
* calling the inner loop to process the rows without checking the interruption flag.
*/
```
--
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
Re: [PR] Be sure block serialization can be interrupted [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #16595:
URL: https://github.com/apache/pinot/pull/16595#discussion_r2281649489
##########
pinot-core/src/main/java/org/apache/pinot/core/common/datablock/DataBlockBuilder.java:
##########
@@ -91,93 +91,95 @@ public static RowDataBlock buildFromRows(List<Object[]> rows, DataSchema dataSch
PagedPinotOutputStream varSize = new PagedPinotOutputStream(allocator);
Object2IntOpenHashMap<String> dictionary = new Object2IntOpenHashMap<>();
- for (int rowId = 0; rowId < numRows; rowId++) {
- Object[] row = rows.get(rowId);
- for (int colId = 0; colId < numColumns; colId++) {
- Object value = row[colId];
- ColumnDataType storedType = storedTypes[colId];
+ interruptableLoop(0, numRows, 1000, (start, end) -> {
Review Comment:
Why is the step size different here?
--
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
Re: [PR] Be sure block serialization can be interrupted [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #16595:
URL: https://github.com/apache/pinot/pull/16595#discussion_r2299985655
##########
pinot-core/src/main/java/org/apache/pinot/core/common/datablock/DataBlockBuilder.java:
##########
@@ -91,93 +91,95 @@ public static RowDataBlock buildFromRows(List<Object[]> rows, DataSchema dataSch
PagedPinotOutputStream varSize = new PagedPinotOutputStream(allocator);
Object2IntOpenHashMap<String> dictionary = new Object2IntOpenHashMap<>();
- for (int rowId = 0; rowId < numRows; rowId++) {
- Object[] row = rows.get(rowId);
- for (int colId = 0; colId < numColumns; colId++) {
- Object value = row[colId];
- ColumnDataType storedType = storedTypes[colId];
+ interruptableLoop(0, numRows, 1000, (start, end) -> {
Review Comment:
The other loops use 10k step size because they just iterate over one column. Here we iterate over the whole table, so I decided to use 1k step size. I didn't run any benchmark, just decided to use a smaller number here for that reason.
--
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
Re: [PR] Be sure block serialization can be interrupted [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #16595:
URL: https://github.com/apache/pinot/pull/16595#issuecomment-3188280124
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/16595?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `95.48023%` with `8 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 56.34%. Comparing base ([`a9bf0cc`](https://app.codecov.io/gh/apache/pinot/commit/a9bf0cc886f5e65226957c69e9f70695b7152ee7?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`6f5e691`](https://app.codecov.io/gh/apache/pinot/commit/6f5e691f1b76da3056ef35249428e295e8e7b377?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/16595?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../pinot/core/common/datablock/DataBlockBuilder.java](https://app.codecov.io/gh/apache/pinot/pull/16595?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Fcommon%2Fdatablock%2FDataBlockBuilder.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9jb21tb24vZGF0YWJsb2NrL0RhdGFCbG9ja0J1aWxkZXIuamF2YQ==) | 95.42% | [4 Missing and 4 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16595?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (a9bf0cc) and HEAD (6f5e691). Click for more details.
>
> <details><summary>HEAD has 36 uploads less than BASE</summary>
>
>| Flag | BASE (a9bf0cc) | HEAD (6f5e691) |
>|------|------|------|
>|java-21|5|1|
>|unittests1|2|1|
>|unittests|4|1|
>|temurin|10|1|
>|java-11|5|0|
>|unittests2|2|0|
>|integration|6|0|
>|integration1|2|0|
>|custom-integration1|2|0|
>|integration2|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #16595 +/- ##
============================================
- Coverage 63.36% 56.34% -7.03%
+ Complexity 1380 699 -681
============================================
Files 3015 2359 -656
Lines 174794 129961 -44833
Branches 26788 20662 -6126
============================================
- Hits 110758 73226 -37532
+ Misses 55575 50693 -4882
+ Partials 8461 6042 -2419
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/16595/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/16595/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/16595/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/16595/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/16595/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/16595/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/16595/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.34% <95.48%> (-6.99%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/16595/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.34% <95.48%> (-7.03%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/16595/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.34% <95.48%> (-7.02%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/16595/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.34% <95.48%> (-0.07%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/16595/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
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=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/16595?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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