You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "timsants (via GitHub)" <gi...@apache.org> on 2023/05/04 05:16:27 UTC
[GitHub] [pinot] timsants opened a new pull request, #10721: Making NormalizedDateSegmentNameGenerator use joda time to keep consistency other time handling
timsants opened a new pull request, #10721:
URL: https://github.com/apache/pinot/pull/10721
**Context**
There is a use case using Kafka. The time column was configured with a time format that was working as expected. But when using a minion task leveraging the SegmentProcessorFramework, a date format exception was thrown:
```
Caught exception while parsing simple date format: yyyy-MM-dd'T'HH:mm:ss.SSSSSSSSSZ with value: 2023-03-07T21:18:05.072784135Z at org.apache.pinot.segment.spi.creator.name.NormalizedDateSegmentNameGenerator.getNormalizedDate(NormalizedDateSegmentNameGenerator.java:142)
```
Specifically, `SegmentColumnarIndexCreator` uses joda time DateFormatter for parsing time format into epoch millis (See [SegmentColumnarIndexCreator](https://github.com/apache/pinot/blob/master/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java#L446)).
While any segment generation configured to use the `NormalizedDateSegmentNameGenerator` may run into an issue where segment generation may fail since it uses another date time format library, `java.text` `SimpleDateFormat`. This library seems to be stricter that the joda time library and if a time column values has literal ‘Z’ in it, the time format must have the `Z` in single quotes. E.g. "2023-01-01T12:00:00.111111111Z" needs a time format of "yyyy-MM-dd'T'HH:mm:ss.SSSSSSSSS’Z’" otherwise it will fail.
**Changes**
This PR makes the NormalizedDateSegmentNameGenerator also use the joda date time formatter. Joda is no longer maintained but making the Pinot wide migration may be a larger effort at the moment. This PR aims to unify the time library until we are ready to make the switch to use java time.
Instructions:
1. The PR has to be tagged with at least one of the following labels (*):
1. `feature`
2. `bugfix`
3. `performance`
4. `ui`
5. `backward-incompat`
6. `release-notes` (**)
2. Remove these instructions before publishing the PR.
(*) Other labels to consider:
- `testing`
- `dependencies`
- `docker`
- `kubernetes`
- `observability`
- `security`
- `code-style`
- `extension-point`
- `refactor`
- `cleanup`
(**) Use `release-notes` label for scenarios like:
- New configuration options
- Deprecation of configurations
- Signature changes to public methods/interfaces
- New plugins added or old plugins removed
--
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] timsants commented on pull request #10721: Making NormalizedDateSegmentNameGenerator use joda time to keep consistency other time handling
Posted by "timsants (via GitHub)" <gi...@apache.org>.
timsants commented on PR #10721:
URL: https://github.com/apache/pinot/pull/10721#issuecomment-1571087416
> Thank you for quickly addressing this!
>
> Can you add the testing case where the format works in joda while it's not working in java to cover this case?
Will do. Also I checked out the other references and it doesn't seem like they will be impacted by this case since they are formatting a constant string.
--
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 merged pull request #10721: Making NormalizedDateSegmentNameGenerator use joda time to keep consistency other time handling
Posted by "snleee (via GitHub)" <gi...@apache.org>.
snleee merged PR #10721:
URL: https://github.com/apache/pinot/pull/10721
--
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 #10721: Making NormalizedDateSegmentNameGenerator use joda time to keep consistency other time handling
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10721:
URL: https://github.com/apache/pinot/pull/10721#issuecomment-1534151800
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/10721?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 [#10721](https://app.codecov.io/gh/apache/pinot/pull/10721?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f7de6fb) into [master](https://app.codecov.io/gh/apache/pinot/commit/b190599cf1ac2a16fc3ab43320dd7fde092dd734?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b190599) will **decrease** coverage by `54.63%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10721 +/- ##
=============================================
- Coverage 68.47% 13.85% -54.63%
+ Complexity 6450 439 -6011
=============================================
Files 2119 2065 -54
Lines 114203 111718 -2485
Branches 17244 16947 -297
=============================================
- Hits 78202 15473 -62729
- Misses 30441 94978 +64537
+ Partials 5560 1267 -4293
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| unittests1 | `?` | |
| unittests2 | `13.85% <0.00%> (+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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/pinot/pull/10721?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...eator/name/NormalizedDateSegmentNameGenerator.java](https://app.codecov.io/gh/apache/pinot/pull/10721?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2NyZWF0b3IvbmFtZS9Ob3JtYWxpemVkRGF0ZVNlZ21lbnROYW1lR2VuZXJhdG9yLmphdmE=) | `0.00% <0.00%> (-83.34%)` | :arrow_down: |
| [.../org/apache/pinot/spi/data/DateTimeFormatSpec.java](https://app.codecov.io/gh/apache/pinot/pull/10721?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9EYXRlVGltZUZvcm1hdFNwZWMuamF2YQ==) | `0.00% <0.00%> (-82.93%)` | :arrow_down: |
... and [1629 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/10721/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