You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Jackie-Jiang (via GitHub)" <gi...@apache.org> on 2024/04/04 00:34:48 UTC
[PR] Fix the time segment pruner on TIMESTAMP data type [pinot]
Jackie-Jiang opened a new pull request, #12789:
URL: https://github.com/apache/pinot/pull/12789
Fix #12721
When data type is TIMESTAMP, fix the format to be "TIMESTAMP" because the time format is already implicit from the data type.
--
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] Fix the time segment pruner on TIMESTAMP data type [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #12789:
URL: https://github.com/apache/pinot/pull/12789
--
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] Fix the time segment pruner on TIMESTAMP data type [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #12789:
URL: https://github.com/apache/pinot/pull/12789#discussion_r1552679449
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/DateTimeFieldSpec.java:
##########
@@ -74,6 +75,10 @@ public DateTimeFieldSpec(String name, DataType dataType, String format, String g
@Nullable Object sampleValue) {
super(name, dataType, true);
+ // Override format to be "TIMESTAMP" for TIMESTAMP data type because the format is implicit
+ if (dataType == DataType.TIMESTAMP) {
Review Comment:
This won't change existing schema. New uploaded/modified schema will be changed.
This reminds me that I do need to change the JSON setter so that create/update can be changed
--
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] Fix the time segment pruner on TIMESTAMP data type [pinot]
Posted by "jadami10 (via GitHub)" <gi...@apache.org>.
jadami10 commented on code in PR #12789:
URL: https://github.com/apache/pinot/pull/12789#discussion_r1552654653
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/DateTimeFieldSpec.java:
##########
@@ -74,6 +75,10 @@ public DateTimeFieldSpec(String name, DataType dataType, String format, String g
@Nullable Object sampleValue) {
super(name, dataType, true);
+ // Override format to be "TIMESTAMP" for TIMESTAMP data type because the format is implicit
+ if (dataType == DataType.TIMESTAMP) {
Review Comment:
will this change people's schemas as they're stored in ZK, or just modify them every time they're loaded in ZK?
I almost wonder if we should go the route of not letting create/update happen with this rather than silently changing it so folks are aware
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentpruner/TimeSegmentPruner.java:
##########
@@ -18,6 +18,7 @@
*/
package org.apache.pinot.broker.routing.segmentpruner;
+import com.google.common.base.Preconditions;
Review Comment:
this whole file is a refactor. you're getting the format from the `DateTimeFieldSpec` rather then unnecessarily passing it. but nothing is actually changing 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] Fix the time segment pruner on TIMESTAMP data type [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #12789:
URL: https://github.com/apache/pinot/pull/12789#issuecomment-2035913973
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/12789?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `58.33333%` with `20 lines` in your changes are missing coverage. Please review.
> Project coverage is 27.97%. Comparing base [(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`e9946fe`)](https://app.codecov.io/gh/apache/pinot/pull/12789?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 202 commits behind head on master.
| [Files](https://app.codecov.io/gh/apache/pinot/pull/12789?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...roker/routing/segmentpruner/TimeSegmentPruner.java](https://app.codecov.io/gh/apache/pinot/pull/12789?src=pr&el=tree&filepath=pinot-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fbroker%2Frouting%2Fsegmentpruner%2FTimeSegmentPruner.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL1RpbWVTZWdtZW50UHJ1bmVyLmphdmE=) | 47.05% | [9 Missing and 9 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12789?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...a/org/apache/pinot/spi/data/DateTimeFieldSpec.java](https://app.codecov.io/gh/apache/pinot/pull/12789?src=pr&el=tree&filepath=pinot-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fspi%2Fdata%2FDateTimeFieldSpec.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9EYXRlVGltZUZpZWxkU3BlYy5qYXZh) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12789?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #12789 +/- ##
=============================================
- Coverage 61.75% 27.97% -33.78%
+ Complexity 207 198 -9
=============================================
Files 2436 2461 +25
Lines 133233 134752 +1519
Branches 20636 20825 +189
=============================================
- Hits 82274 37703 -44571
- Misses 44911 94190 +49279
+ Partials 6048 2859 -3189
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/12789/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/12789/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (-0.01%)` | :arrow_down: |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/12789/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (-0.01%)` | :arrow_down: |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/12789/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (-0.01%)` | :arrow_down: |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/12789/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (ø)` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/12789/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (-61.71%)` | :arrow_down: |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/12789/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.97% <58.33%> (-33.65%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/12789/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (-61.75%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/12789/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.97% <58.33%> (+0.24%)` | :arrow_up: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/12789/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.97% <58.33%> (-33.78%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/12789/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.97% <58.33%> (-33.78%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/12789/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/12789/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.97% <58.33%> (+0.24%)` | :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.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/12789?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).
--
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] Fix the time segment pruner on TIMESTAMP data type [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #12789:
URL: https://github.com/apache/pinot/pull/12789#discussion_r1552670454
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentpruner/TimeSegmentPruner.java:
##########
@@ -18,6 +18,7 @@
*/
package org.apache.pinot.broker.routing.segmentpruner;
+import com.google.common.base.Preconditions;
Review Comment:
Correct. I started with making changes in this file, but then in the end find I can simply override the `format`
--
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