You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "ege-st (via GitHub)" <gi...@apache.org> on 2023/06/02 18:13:22 UTC
[GitHub] [pinot] ege-st opened a new pull request, #10835: [feature] Add flags to indicate if FST, H3, and Text indexes are on a segment
ege-st opened a new pull request, #10835:
URL: https://github.com/apache/pinot/pull/10835
This PR augments the Segment Metadata returned by the Controller REST API `/segments/{tableName}/{segmentName}/metadata` with flags indicating if columns in a segment have any of the following indexes: H3, FST, and Text. Currently, Pinot has flags for every index but these 3. Without this change, it is not possible to tell if these 3 indexes are on columns in a segment through the metadata returned by the Pinot Controller.
--
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 a diff in pull request #10835: [feature] Add flags to indicate if FST, H3, and Text indexes are on a segment
Posted by "klsince (via GitHub)" <gi...@apache.org>.
klsince commented on code in PR #10835:
URL: https://github.com/apache/pinot/pull/10835#discussion_r1214687960
##########
pinot-server/src/main/java/org/apache/pinot/server/api/resources/SegmentMetadataFetcher.java:
##########
@@ -152,6 +155,24 @@ private static Map<String, String> getColumnIndexes(DataSource dataSource) {
indexStatus.put(JSON_INDEX, INDEX_AVAILABLE);
}
+ if (Objects.isNull(dataSource.getH3Index())) {
+ indexStatus.put(H3_INDEX, INDEX_NOT_AVAILABLE);
+ } else {
+ indexStatus.put(H3_INDEX, INDEX_AVAILABLE);
+ }
+
+ if (Objects.isNull(dataSource.getFSTIndex())) {
+ indexStatus.put(FST_INDEX, INDEX_NOT_AVAILABLE);
+ } else {
+ indexStatus.put(FST_INDEX, INDEX_AVAILABLE);
+ }
+
+ if (Objects.isNull(dataSource.getTextIndex())) {
+ indexStatus.put(TEXT_INDEX, INDEX_NOT_AVAILABLE);
+ } else {
+ indexStatus.put(TEXT_INDEX, INDEX_AVAILABLE);
+ }
Review Comment:
good catch. but I think this getColumnIndexes() is simply broken after adding the new index-spi support, which allows one to add new index types, not in this hard coded list of index types. Perhaps need to use `IndexService.getInstance().getAllIndexes()` to refactor this method.
cc @gortiz who added index-spi support, to shed more light on this.
--
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] ege-st commented on a diff in pull request #10835: [feature] Add flags to indicate if FST, H3, and Text indexes are on a segment
Posted by "ege-st (via GitHub)" <gi...@apache.org>.
ege-st commented on code in PR #10835:
URL: https://github.com/apache/pinot/pull/10835#discussion_r1214690970
##########
pinot-server/src/main/java/org/apache/pinot/server/api/resources/SegmentMetadataFetcher.java:
##########
@@ -55,6 +55,9 @@ private SegmentMetadataFetcher() {
private static final String NULL_VALUE_VECTOR_READER = "null-value-vector-reader";
private static final String RANGE_INDEX = "range-index";
private static final String JSON_INDEX = "json-index";
+ private static final String H3_INDEX = "h3-index";
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] npawar commented on a diff in pull request #10835: [feature] Add flags to indicate if FST, H3, and Text indexes are on a segment
Posted by "npawar (via GitHub)" <gi...@apache.org>.
npawar commented on code in PR #10835:
URL: https://github.com/apache/pinot/pull/10835#discussion_r1214713039
##########
pinot-server/src/main/java/org/apache/pinot/server/api/resources/SegmentMetadataFetcher.java:
##########
@@ -152,6 +155,24 @@ private static Map<String, String> getColumnIndexes(DataSource dataSource) {
indexStatus.put(JSON_INDEX, INDEX_AVAILABLE);
}
+ if (Objects.isNull(dataSource.getH3Index())) {
+ indexStatus.put(H3_INDEX, INDEX_NOT_AVAILABLE);
+ } else {
+ indexStatus.put(H3_INDEX, INDEX_AVAILABLE);
+ }
+
+ if (Objects.isNull(dataSource.getFSTIndex())) {
+ indexStatus.put(FST_INDEX, INDEX_NOT_AVAILABLE);
+ } else {
+ indexStatus.put(FST_INDEX, INDEX_AVAILABLE);
+ }
+
+ if (Objects.isNull(dataSource.getTextIndex())) {
+ indexStatus.put(TEXT_INDEX, INDEX_NOT_AVAILABLE);
+ } else {
+ indexStatus.put(TEXT_INDEX, INDEX_AVAILABLE);
+ }
Review Comment:
good point.. i think it's safe to push this change regardless, and then refactor using getAllIndexes, as doing that will not change the final response returned but just how we reach that answer. Perhaps add a todo and take it as a followup?
--
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] npawar merged pull request #10835: [feature] Add flags to indicate if FST, H3, and Text indexes are on a segment
Posted by "npawar (via GitHub)" <gi...@apache.org>.
npawar merged PR #10835:
URL: https://github.com/apache/pinot/pull/10835
--
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 #10835: [feature] Add flags to indicate if FST, H3, and Text indexes are on a segment
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10835:
URL: https://github.com/apache/pinot/pull/10835#discussion_r1214680724
##########
pinot-server/src/main/java/org/apache/pinot/server/api/resources/SegmentMetadataFetcher.java:
##########
@@ -55,6 +55,9 @@ private SegmentMetadataFetcher() {
private static final String NULL_VALUE_VECTOR_READER = "null-value-vector-reader";
private static final String RANGE_INDEX = "range-index";
private static final String JSON_INDEX = "json-index";
+ private static final String H3_INDEX = "h3-index";
Review Comment:
can we add a test in `TableResourceTest.testSegmentMetadata` to indicate the behavior?
--
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] gortiz commented on a diff in pull request #10835: [feature] Add flags to indicate if FST, H3, and Text indexes are on a segment
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10835:
URL: https://github.com/apache/pinot/pull/10835#discussion_r1218389670
##########
pinot-server/src/main/java/org/apache/pinot/server/api/resources/SegmentMetadataFetcher.java:
##########
@@ -152,6 +155,24 @@ private static Map<String, String> getColumnIndexes(DataSource dataSource) {
indexStatus.put(JSON_INDEX, INDEX_AVAILABLE);
}
+ if (Objects.isNull(dataSource.getH3Index())) {
+ indexStatus.put(H3_INDEX, INDEX_NOT_AVAILABLE);
+ } else {
+ indexStatus.put(H3_INDEX, INDEX_AVAILABLE);
+ }
+
+ if (Objects.isNull(dataSource.getFSTIndex())) {
+ indexStatus.put(FST_INDEX, INDEX_NOT_AVAILABLE);
+ } else {
+ indexStatus.put(FST_INDEX, INDEX_AVAILABLE);
+ }
+
+ if (Objects.isNull(dataSource.getTextIndex())) {
+ indexStatus.put(TEXT_INDEX, INDEX_NOT_AVAILABLE);
+ } else {
+ indexStatus.put(TEXT_INDEX, INDEX_AVAILABLE);
+ }
Review Comment:
Yeah, the solution from @klsince is more generic (and easier to read!). I don't mind if we decide to do that in another PR, but I think it is something we should to soon or otherwise we are going to forget about that until we find another index that is not 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 #10835: [feature] Add flags to indicate if FST, H3, and Text indexes are on a segment
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10835:
URL: https://github.com/apache/pinot/pull/10835#issuecomment-1577304592
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/10835?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#10835](https://app.codecov.io/gh/apache/pinot/pull/10835?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (adf9048) into [master](https://app.codecov.io/gh/apache/pinot/commit/84688577e40ef34dcb2aadf558ec759ca80466da?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (8468857) will **decrease** coverage by `56.62%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #10835 +/- ##
=============================================
- Coverage 70.23% 13.62% -56.62%
+ Complexity 6585 439 -6146
=============================================
Files 2170 2116 -54
Lines 116665 114196 -2469
Branches 17656 17363 -293
=============================================
- Hits 81942 15558 -66384
- Misses 29014 97361 +68347
+ Partials 5709 1277 -4432
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `13.62% <ø> (+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=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
[see 1715 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/10835/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
: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=apache)
--
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 a diff in pull request #10835: [feature] Add flags to indicate if FST, H3, and Text indexes are on a segment
Posted by "klsince (via GitHub)" <gi...@apache.org>.
klsince commented on code in PR #10835:
URL: https://github.com/apache/pinot/pull/10835#discussion_r1214752506
##########
pinot-server/src/main/java/org/apache/pinot/server/api/resources/SegmentMetadataFetcher.java:
##########
@@ -152,6 +155,24 @@ private static Map<String, String> getColumnIndexes(DataSource dataSource) {
indexStatus.put(JSON_INDEX, INDEX_AVAILABLE);
}
+ if (Objects.isNull(dataSource.getH3Index())) {
+ indexStatus.put(H3_INDEX, INDEX_NOT_AVAILABLE);
+ } else {
+ indexStatus.put(H3_INDEX, INDEX_AVAILABLE);
+ }
+
+ if (Objects.isNull(dataSource.getFSTIndex())) {
+ indexStatus.put(FST_INDEX, INDEX_NOT_AVAILABLE);
+ } else {
+ indexStatus.put(FST_INDEX, INDEX_AVAILABLE);
+ }
+
+ if (Objects.isNull(dataSource.getTextIndex())) {
+ indexStatus.put(TEXT_INDEX, INDEX_NOT_AVAILABLE);
+ } else {
+ indexStatus.put(TEXT_INDEX, INDEX_AVAILABLE);
+ }
Review Comment:
sgtm
--
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