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 2023/05/22 07:59:36 UTC
[GitHub] [pinot] gortiz opened a new pull request, #10792: Add required JSON annotation in H3IndexResolution
gortiz opened a new pull request, #10792:
URL: https://github.com/apache/pinot/pull/10792
This is a bugfix.
H3 resolutions were not able to be deserialized with Jackson, which was incompatible with the new index configuration syntax. The problem was the lack of a `@JsonProperty` annotation in the constructor.
A couple of tests were added to verify that the new code is working and prevent that error in the future
--
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 #10792: Add required JSON annotation in H3IndexResolution
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10792:
URL: https://github.com/apache/pinot/pull/10792#issuecomment-1564652325
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/10792?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#10792](https://app.codecov.io/gh/apache/pinot/pull/10792?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (a106b9e) into [master](https://app.codecov.io/gh/apache/pinot/commit/3851f3309457340d83f9341c8eb981da1b4f8bb1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (3851f33) will **decrease** coverage by `36.95%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10792 +/- ##
=============================================
- Coverage 68.51% 31.57% -36.95%
+ Complexity 6479 453 -6026
=============================================
Files 2159 2164 +5
Lines 116044 116325 +281
Branches 17569 17592 +23
=============================================
- Hits 79510 36725 -42785
- Misses 30899 76282 +45383
+ Partials 5635 3318 -2317
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `24.10% <0.00%> (+0.10%)` | :arrow_up: |
| unittests1 | `?` | |
| unittests2 | `13.66% <0.00%> (-0.02%)` | :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=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/pinot/pull/10792?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...ot/segment/spi/index/reader/H3IndexResolution.java](https://app.codecov.io/gh/apache/pinot/pull/10792?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2luZGV4L3JlYWRlci9IM0luZGV4UmVzb2x1dGlvbi5qYXZh) | `0.00% <0.00%> (-73.92%)` | :arrow_down: |
... and [1285 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/10792/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] gortiz commented on a diff in pull request #10792: Add required JSON annotation in H3IndexResolution
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10792:
URL: https://github.com/apache/pinot/pull/10792#discussion_r1206336003
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/reader/H3IndexResolution.java:
##########
@@ -91,4 +94,11 @@ public boolean equals(Object o) {
public int hashCode() {
return Objects.hash(_resolutions);
}
+
+ public static class ToIntListConverted extends StdConverter<H3IndexResolution, List<Integer>> {
Review Comment:
With the custom deserializer, `@JsonCreator` is not needed (and in fact it may be confusing). I decided to serialize as list, but we can change that to deserialize as short if you think that is better. I guess neither short or list are human readable and short is always going to be smaller.
--
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 #10792: Add required JSON annotation in H3IndexResolution
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10792:
URL: https://github.com/apache/pinot/pull/10792#discussion_r1206334783
##########
pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/index/creator/H3IndexConfigTest.java:
##########
@@ -84,4 +84,16 @@ public void withSomeData()
Assert.assertEquals(resolution.getLowestResolution(), 5);
Assert.assertEquals(resolution.getResolutions(), Lists.newArrayList(5, 6, 13));
}
+
+ @Test
+ public void serde()
+ throws JsonProcessingException {
+ H3IndexConfig initialConf = new H3IndexConfig(new H3IndexResolution(Lists.newArrayList(5, 6, 13)));
+
+ String confAsJson = JsonUtils.objectToString(initialConf);
+ Assert.assertEquals(confAsJson, "{\"disabled\":false,\"resolution\":[5,6,13]}");
Review Comment:
The new version supports what you said (although the bit shifted version seems to be `8288`).
But we need to choose which representation are we going to use when serializing. I used the string version, but we can change that we prefer to use the int one.
--
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 #10792: Add required JSON annotation in H3IndexResolution
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10792:
URL: https://github.com/apache/pinot/pull/10792#discussion_r1206309308
##########
pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/index/creator/H3IndexConfigTest.java:
##########
@@ -84,4 +84,16 @@ public void withSomeData()
Assert.assertEquals(resolution.getLowestResolution(), 5);
Assert.assertEquals(resolution.getResolutions(), Lists.newArrayList(5, 6, 13));
}
+
+ @Test
+ public void serde()
+ throws JsonProcessingException {
+ H3IndexConfig initialConf = new H3IndexConfig(new H3IndexResolution(Lists.newArrayList(5, 6, 13)));
+
+ String confAsJson = JsonUtils.objectToString(initialConf);
+ Assert.assertEquals(confAsJson, "{\"disabled\":false,\"resolution\":[5,6,13]}");
Review Comment:
We would need to add a custom serializer and deserializer, but it is possible.
--
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 #10792: Add required JSON annotation in H3IndexResolution
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10792:
URL: https://github.com/apache/pinot/pull/10792#discussion_r1205738972
##########
pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/index/creator/H3IndexConfigTest.java:
##########
@@ -84,4 +84,16 @@ public void withSomeData()
Assert.assertEquals(resolution.getLowestResolution(), 5);
Assert.assertEquals(resolution.getResolutions(), Lists.newArrayList(5, 6, 13));
}
+
+ @Test
+ public void serde()
+ throws JsonProcessingException {
+ H3IndexConfig initialConf = new H3IndexConfig(new H3IndexResolution(Lists.newArrayList(5, 6, 13)));
+
+ String confAsJson = JsonUtils.objectToString(initialConf);
+ Assert.assertEquals(confAsJson, "{\"disabled\":false,\"resolution\":[5,6,13]}");
Review Comment:
can we support both
`\"resolution\":[5,6,13]` and `\"resolution\":8663` (which is the bit-shifted version of the array)
--
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 #10792: Add required JSON annotation in H3IndexResolution
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10792:
URL: https://github.com/apache/pinot/pull/10792#discussion_r1206988449
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/reader/H3IndexResolution.java:
##########
@@ -91,4 +94,11 @@ public boolean equals(Object o) {
public int hashCode() {
return Objects.hash(_resolutions);
}
+
+ public static class ToIntListConverted extends StdConverter<H3IndexResolution, List<Integer>> {
Review Comment:
do we need the list format? as both are not human readable eitherway. keeping it as short is good IMO.
as long as we can construct this from list or short we should be fine right?
--
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] yupeng9 commented on a diff in pull request #10792: Add required JSON annotation in H3IndexResolution
Posted by "yupeng9 (via GitHub)" <gi...@apache.org>.
yupeng9 commented on code in PR #10792:
URL: https://github.com/apache/pinot/pull/10792#discussion_r1213866750
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/reader/H3IndexResolution.java:
##########
@@ -91,4 +94,11 @@ public boolean equals(Object o) {
public int hashCode() {
return Objects.hash(_resolutions);
}
+
+ public static class ToIntListConverted extends StdConverter<H3IndexResolution, List<Integer>> {
Review Comment:
cool. good to see this improvement
--
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] yupeng9 merged pull request #10792: Add required JSON annotation in H3IndexResolution
Posted by "yupeng9 (via GitHub)" <gi...@apache.org>.
yupeng9 merged PR #10792:
URL: https://github.com/apache/pinot/pull/10792
--
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 #10792: Add required JSON annotation in H3IndexResolution
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10792:
URL: https://github.com/apache/pinot/pull/10792#discussion_r1206984595
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/reader/H3IndexResolution.java:
##########
@@ -91,4 +94,11 @@ public boolean equals(Object o) {
public int hashCode() {
return Objects.hash(_resolutions);
}
+
+ public static class ToIntListConverted extends StdConverter<H3IndexResolution, List<Integer>> {
Review Comment:
Before this PR, the actual value that was serialized was a string like `"5,6,13"`, whose size codified as a JSON is 8 characters. The new codification is the same (`[5,6,13]`). The larger value that can be represented like that is `[1,2,3,4,5,6,7,8,9,10,11,12,13,14,15]`, which is 35 characters (and TBH I don't know if it is a valid resolution).
On the other hand, the numeric codification is a short, whose larger utf8 value would be `-32768`, which is 6 characters.
Sometimes the array codification will be better. For example, `[14]` requires 4 chars, while it would be represented as `16384` in the short codification. But there the values where this happens is very small.
--
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 #10792: Add required JSON annotation in H3IndexResolution
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10792:
URL: https://github.com/apache/pinot/pull/10792#discussion_r1205737889
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/reader/H3IndexResolution.java:
##########
@@ -91,4 +94,11 @@ public boolean equals(Object o) {
public int hashCode() {
return Objects.hash(_resolutions);
}
+
+ public static class ToIntListConverted extends StdConverter<H3IndexResolution, List<Integer>> {
Review Comment:
was wondering if we can do
- serialize just the short format and not to List<Integer> serialization?
- still keep the JSON creator but also annotate the short constructor as JSON create?
--
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] yupeng9 commented on a diff in pull request #10792: Add required JSON annotation in H3IndexResolution
Posted by "yupeng9 (via GitHub)" <gi...@apache.org>.
yupeng9 commented on code in PR #10792:
URL: https://github.com/apache/pinot/pull/10792#discussion_r1206953061
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/reader/H3IndexResolution.java:
##########
@@ -91,4 +94,11 @@ public boolean equals(Object o) {
public int hashCode() {
return Objects.hash(_resolutions);
}
+
+ public static class ToIntListConverted extends StdConverter<H3IndexResolution, List<Integer>> {
Review Comment:
can we check the serialized size for comparison?
--
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