You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "snleee (via GitHub)" <gi...@apache.org> on 2023/03/25 00:57:47 UTC
[GitHub] [pinot] snleee opened a new pull request, #10476: Remove "isSorted()" precondition check in the ForwardIndexHandler
snleee opened a new pull request, #10476:
URL: https://github.com/apache/pinot/pull/10476
Current code has a precondition check for no-dictionary colums and we fail the load if the no-dictionary columns are sorted when the table config has changed from `no-dict` -> `dict`. This PR unblocks the failure issue.
--
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] snleee commented on a diff in pull request #10476: Remove "isSorted()" precondition check in the ForwardIndexHandler
Posted by "snleee (via GitHub)" <gi...@apache.org>.
snleee commented on code in PR #10476:
URL: https://github.com/apache/pinot/pull/10476#discussion_r1148238096
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java:
##########
@@ -302,8 +302,13 @@ Map<String, List<Operation>> computeOperations(SegmentDirectory.Reader segmentRe
}
// Note that RAW columns cannot be sorted.
+ // TODO: revisit the the possibility of converting noDictionary -> Dictionary when the column is sorted.
ColumnMetadata existingColMetadata = _segmentDirectory.getSegmentMetadata().getColumnMetadataFor(column);
- Preconditions.checkState(!existingColMetadata.isSorted(), "Raw column=" + column + " cannot be sorted.");
+ if (existingColMetadata.isSorted()) {
+ LOGGER.warn("Raw column={} cannot be sorted and the dictionary won't be created. Falling back to use "
+ + "noDictionaryColumn..", column);
+ continue;
+ }
Review Comment:
done
--
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 #10476: Remove "isSorted()" precondition check in the ForwardIndexHandler
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10476:
URL: https://github.com/apache/pinot/pull/10476#discussion_r1148224873
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java:
##########
@@ -302,8 +302,13 @@ Map<String, List<Operation>> computeOperations(SegmentDirectory.Reader segmentRe
}
// Note that RAW columns cannot be sorted.
+ // TODO: revisit the the possibility of converting noDictionary -> Dictionary when the column is sorted.
ColumnMetadata existingColMetadata = _segmentDirectory.getSegmentMetadata().getColumnMetadataFor(column);
- Preconditions.checkState(!existingColMetadata.isSorted(), "Raw column=" + column + " cannot be sorted.");
+ if (existingColMetadata.isSorted()) {
+ LOGGER.warn("Raw column={} cannot be sorted and the dictionary won't be created. Falling back to use "
+ + "noDictionaryColumn..", column);
+ continue;
+ }
Review Comment:
We don't want to skip this column 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] vvivekiyer commented on a diff in pull request #10476: Remove "isSorted()" precondition check in the ForwardIndexHandler
Posted by "vvivekiyer (via GitHub)" <gi...@apache.org>.
vvivekiyer commented on code in PR #10476:
URL: https://github.com/apache/pinot/pull/10476#discussion_r1148245243
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java:
##########
@@ -301,10 +301,6 @@ Map<String, List<Operation>> computeOperations(SegmentDirectory.Reader segmentRe
continue;
}
- // Note that RAW columns cannot be sorted.
Review Comment:
nit:
In L757 (`createDictBasedForwardIndex()`), can we remove the comment and also name the ForwardIndex tmp file appropriately?
--
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 #10476: Remove "isSorted()" precondition check in the ForwardIndexHandler
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10476:
URL: https://github.com/apache/pinot/pull/10476#discussion_r1148222477
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java:
##########
@@ -302,8 +302,13 @@ Map<String, List<Operation>> computeOperations(SegmentDirectory.Reader segmentRe
}
// Note that RAW columns cannot be sorted.
+ // TODO: revisit the the possibility of converting noDictionary -> Dictionary when the column is sorted.
ColumnMetadata existingColMetadata = _segmentDirectory.getSegmentMetadata().getColumnMetadataFor(column);
- Preconditions.checkState(!existingColMetadata.isSorted(), "Raw column=" + column + " cannot be sorted.");
+ if (existingColMetadata.isSorted()) {
+ LOGGER.warn("Raw column={} cannot be sorted and the dictionary won't be created. Falling back to use "
+ + "noDictionaryColumn..", column);
+ continue;
+ }
Review Comment:
I think we can remove the whole check. This check itself doesn't provide extra value
--
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] vvivekiyer commented on a diff in pull request #10476: Remove "isSorted()" precondition check in the ForwardIndexHandler
Posted by "vvivekiyer (via GitHub)" <gi...@apache.org>.
vvivekiyer commented on code in PR #10476:
URL: https://github.com/apache/pinot/pull/10476#discussion_r1148245243
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java:
##########
@@ -301,10 +301,6 @@ Map<String, List<Operation>> computeOperations(SegmentDirectory.Reader segmentRe
continue;
}
- // Note that RAW columns cannot be sorted.
Review Comment:
nit:
In L757, can we remove the comment and also name the ForwardIndex tmp file appropriately?
--
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] vvivekiyer commented on a diff in pull request #10476: Remove "isSorted()" precondition check in the ForwardIndexHandler
Posted by "vvivekiyer (via GitHub)" <gi...@apache.org>.
vvivekiyer commented on code in PR #10476:
URL: https://github.com/apache/pinot/pull/10476#discussion_r1148191376
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java:
##########
@@ -302,8 +302,11 @@ Map<String, List<Operation>> computeOperations(SegmentDirectory.Reader segmentRe
}
// Note that RAW columns cannot be sorted.
+ // TODO: revisit the the possibility of converting noDictionary -> Dictionary when the column is sorted.
ColumnMetadata existingColMetadata = _segmentDirectory.getSegmentMetadata().getColumnMetadataFor(column);
- Preconditions.checkState(!existingColMetadata.isSorted(), "Raw column=" + column + " cannot be sorted.");
+ if (existingColMetadata.isSorted()) {
Review Comment:
Can we please add a LOGGER.warn here with the column name?
--
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] snleee commented on a diff in pull request #10476: Remove "isSorted()" precondition check in the ForwardIndexHandler
Posted by "snleee (via GitHub)" <gi...@apache.org>.
snleee commented on code in PR #10476:
URL: https://github.com/apache/pinot/pull/10476#discussion_r1148194354
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java:
##########
@@ -302,8 +302,11 @@ Map<String, List<Operation>> computeOperations(SegmentDirectory.Reader segmentRe
}
// Note that RAW columns cannot be sorted.
+ // TODO: revisit the the possibility of converting noDictionary -> Dictionary when the column is sorted.
ColumnMetadata existingColMetadata = _segmentDirectory.getSegmentMetadata().getColumnMetadataFor(column);
- Preconditions.checkState(!existingColMetadata.isSorted(), "Raw column=" + column + " cannot be sorted.");
+ if (existingColMetadata.isSorted()) {
Review Comment:
added!
--
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 #10476: Remove "isSorted()" precondition check in the ForwardIndexHandler
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10476:
URL: https://github.com/apache/pinot/pull/10476#issuecomment-1483673127
## [Codecov](https://codecov.io/gh/apache/pinot/pull/10476?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 [#10476](https://codecov.io/gh/apache/pinot/pull/10476?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (41b4bec) into [master](https://codecov.io/gh/apache/pinot/commit/3f36623c563b5ad495a502a00f627b44bfc0861d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3f36623) will **decrease** coverage by `56.27%`.
> The diff coverage is `0.00%`.
> :exclamation: Current head 41b4bec differs from pull request most recent head da5acbc. Consider uploading reports for the commit da5acbc to get more accurate results
```diff
@@ Coverage Diff @@
## master #10476 +/- ##
=============================================
- Coverage 70.25% 13.99% -56.27%
+ Complexity 6114 237 -5877
=============================================
Files 2070 2016 -54
Lines 111924 109464 -2460
Branches 17045 16752 -293
=============================================
- Hits 78635 15316 -63319
- Misses 27749 92899 +65150
+ Partials 5540 1249 -4291
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `13.99% <0.00%> (-0.01%)` | :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/10476?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ocal/segment/index/loader/ForwardIndexHandler.java](https://codecov.io/gh/apache/pinot/pull/10476?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2xvYWRlci9Gb3J3YXJkSW5kZXhIYW5kbGVyLmphdmE=) | `0.00% <0.00%> (-82.22%)` | :arrow_down: |
... and [1612 files with indirect coverage changes](https://codecov.io/gh/apache/pinot/pull/10476/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?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] xiangfu0 merged pull request #10476: Remove "isSorted()" precondition check in the ForwardIndexHandler
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #10476:
URL: https://github.com/apache/pinot/pull/10476
--
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