You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2020/05/01 18:19:40 UTC

[GitHub] [incubator-pinot] npawar opened a new pull request #5324: Timespec to datetimespec conversion utility

npawar opened a new pull request #5324:
URL: https://github.com/apache/incubator-pinot/pull/5324


   This is an item of project: https://github.com/apache/incubator-pinot/issues/2756
   We plan to internally start treating timeFieldSpec as dateTimeFieldSpec. This PR adds a utility function which converts a timeFieldSpec to an equivalent dateTimeFieldSpec.
   Note that dateTimeFieldSpec doesn't have the concept of incoming/outgoing, and hence we add a transform function to convey the conversion.


----------------------------------------------------------------
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.

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] [incubator-pinot] npawar commented on a change in pull request #5324: Timespec to datetimespec conversion utility

Posted by GitBox <gi...@apache.org>.
npawar commented on a change in pull request #5324:
URL: https://github.com/apache/incubator-pinot/pull/5324#discussion_r419634660



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/function/DateTimeFunctions.java
##########
@@ -35,9 +63,79 @@ static Long toEpochHours(Long millis) {
   }
 
   /**
-   * Convert epoch millis to epoch minutes, bucketed by given bucket granularity
+   * Convert epoch millis to epoch hours, bucketed by given bucket granularity
    */
-  static Long toEpochMinutes(Long millis, String bucket) {
-    return TimeUnit.MILLISECONDS.toMinutes(millis) / Integer.parseInt(bucket);
+  static Long toEpochHoursBucket(Long millis, String bucket) {
+    return TimeUnit.MILLISECONDS.toHours(millis) / Integer.parseInt(bucket);

Review comment:
       Ah didn't know about the caching done by valueOf. Thanks




----------------------------------------------------------------
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.

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] [incubator-pinot] mcvsubbu commented on pull request #5324: Timespec to datetimespec conversion utility

Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on pull request #5324:
URL: https://github.com/apache/incubator-pinot/pull/5324#issuecomment-622584455


   > > Is the new util class to be used inside the Schema class (and everyone else outside sees only DateTimeFieldSpec)?
   > 
   > That's right. Only Schema class should know about this converter. Maybe it can even go into the Schema class
   
   Yes, as methods in Schema class?


----------------------------------------------------------------
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.

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] [incubator-pinot] npawar commented on pull request #5324: Timespec to datetimespec conversion utility

Posted by GitBox <gi...@apache.org>.
npawar commented on pull request #5324:
URL: https://github.com/apache/incubator-pinot/pull/5324#issuecomment-622579288


   > Is the new util class to be used inside the Schema class (and everyone else outside sees only DateTimeFieldSpec)?
   
   That's right. Only Schema class should know about this converter. Maybe it can even go into the Schema class


----------------------------------------------------------------
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.

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] [incubator-pinot] mcvsubbu commented on a change in pull request #5324: Timespec to datetimespec conversion utility

Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #5324:
URL: https://github.com/apache/incubator-pinot/pull/5324#discussion_r419618729



##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/data/Schema.java
##########
@@ -631,4 +632,130 @@ public int hashCode() {
     result = EqualityUtils.hashCodeOf(result, _dateTimeFieldSpecs);
     return result;
   }
+
+  /**
+   * Helper method that converts a {@link TimeFieldSpec} to {@link DateTimeFieldSpec}
+   * 1) If timeFieldSpec contains only incoming granularity spec, directly convert it to a dateTimeFieldSpec
+   * 2) If timeFieldSpec contains incoming aas well as outgoing granularity spec, use the outgoing spec to construct the dateTimeFieldSpec,
+   *    and configure a transform function for the conversion from incoming
+   */
+  @VisibleForTesting
+  static DateTimeFieldSpec convertToDateTimeFieldSpec(TimeFieldSpec timeFieldSpec) {

Review comment:
       another way to do this is to keep it protected and let tests access it. Alternatively, keep it private and use introspection (makes it brittle, but we should discover very soon since the tests will fail).
   

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/function/DateTimeFunctions.java
##########
@@ -35,9 +63,79 @@ static Long toEpochHours(Long millis) {
   }
 
   /**
-   * Convert epoch millis to epoch minutes, bucketed by given bucket granularity
+   * Convert epoch millis to epoch hours, bucketed by given bucket granularity
    */
-  static Long toEpochMinutes(Long millis, String bucket) {
-    return TimeUnit.MILLISECONDS.toMinutes(millis) / Integer.parseInt(bucket);
+  static Long toEpochHoursBucket(Long millis, String bucket) {

Review comment:
       The term 'bucket' is not intuitive to me (but that could be just me). You may think of using something like:
   ```
   toEpochHoursRounded(Long millis, String granularity) {
   ```
   with the comment that the method converts the milis to hours, and rounds it down to the nearest granularity specified.
   
   You can decide, though.

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/function/DateTimeFunctions.java
##########
@@ -35,9 +63,79 @@ static Long toEpochHours(Long millis) {
   }
 
   /**
-   * Convert epoch millis to epoch minutes, bucketed by given bucket granularity
+   * Convert epoch millis to epoch hours, bucketed by given bucket granularity
    */
-  static Long toEpochMinutes(Long millis, String bucket) {
-    return TimeUnit.MILLISECONDS.toMinutes(millis) / Integer.parseInt(bucket);
+  static Long toEpochHoursBucket(Long millis, String bucket) {
+    return TimeUnit.MILLISECONDS.toHours(millis) / Integer.parseInt(bucket);

Review comment:
       Isnt it better to use Integer.valueOf()? Agreed it returns an object, but these objects are cached (for < 128 values, which is likely to be the case here). Otherwise, we will be parsing a string for every row ingested?




----------------------------------------------------------------
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.

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] [incubator-pinot] npawar commented on a change in pull request #5324: Timespec to datetimespec conversion utility

Posted by GitBox <gi...@apache.org>.
npawar commented on a change in pull request #5324:
URL: https://github.com/apache/incubator-pinot/pull/5324#discussion_r419635708



##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/data/Schema.java
##########
@@ -631,4 +632,130 @@ public int hashCode() {
     result = EqualityUtils.hashCodeOf(result, _dateTimeFieldSpecs);
     return result;
   }
+
+  /**
+   * Helper method that converts a {@link TimeFieldSpec} to {@link DateTimeFieldSpec}
+   * 1) If timeFieldSpec contains only incoming granularity spec, directly convert it to a dateTimeFieldSpec
+   * 2) If timeFieldSpec contains incoming aas well as outgoing granularity spec, use the outgoing spec to construct the dateTimeFieldSpec,
+   *    and configure a transform function for the conversion from incoming
+   */
+  @VisibleForTesting
+  static DateTimeFieldSpec convertToDateTimeFieldSpec(TimeFieldSpec timeFieldSpec) {

Review comment:
       My intention was to do the first of your options. I chose the "default" access modifier instead of protected, as it's one level more restrictive




----------------------------------------------------------------
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.

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] [incubator-pinot] codecov-io edited a comment on pull request #5324: Timespec to datetimespec conversion utility

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #5324:
URL: https://github.com/apache/incubator-pinot/pull/5324#issuecomment-622564254


   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5324?src=pr&el=h1) Report
   > Merging [#5324](https://codecov.io/gh/apache/incubator-pinot/pull/5324?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/bd79861e7bca87406fc184ec70ce504667f76daf&el=desc) will **decrease** coverage by `0.13%`.
   > The diff coverage is `73.79%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/5324/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz)](https://codecov.io/gh/apache/incubator-pinot/pull/5324?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #5324      +/-   ##
   ==========================================
   - Coverage   66.23%   66.09%   -0.14%     
   ==========================================
     Files        1066     1072       +6     
     Lines       54552    54653     +101     
     Branches     8128     8152      +24     
   ==========================================
   - Hits        36132    36125       -7     
   - Misses      15774    15874     +100     
   - Partials     2646     2654       +8     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/5324?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...e/pinot/broker/api/resources/PinotBrokerDebug.java](https://codecov.io/gh/apache/incubator-pinot/pull/5324/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdEJyb2tlckRlYnVnLmphdmE=) | `76.66% <ø> (ø)` | |
   | [.../BrokerResourceOnlineOfflineStateModelFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/5324/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclJlc291cmNlT25saW5lT2ZmbGluZVN0YXRlTW9kZWxGYWN0b3J5LmphdmE=) | `55.81% <ø> (ø)` | |
   | [...quota/HelixExternalViewBasedQueryQuotaManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/5324/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9IZWxpeEV4dGVybmFsVmlld0Jhc2VkUXVlcnlRdW90YU1hbmFnZXIuamF2YQ==) | `67.87% <ø> (ø)` | |
   | [...roker/requesthandler/BaseBrokerRequestHandler.java](https://codecov.io/gh/apache/incubator-pinot/pull/5324/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvQmFzZUJyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | `79.35% <ø> (ø)` | |
   | [...rg/apache/pinot/broker/routing/RoutingManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/5324/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9Sb3V0aW5nTWFuYWdlci5qYXZh) | `81.15% <ø> (ø)` | |
   | [...ting/instanceselector/InstanceSelectorFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/5324/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3JGYWN0b3J5LmphdmE=) | `71.42% <ø> (ø)` | |
   | [...er/routing/segmentpruner/SegmentPrunerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/5324/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL1NlZ21lbnRQcnVuZXJGYWN0b3J5LmphdmE=) | `83.33% <ø> (ø)` | |
   | [...outing/segmentselector/SegmentSelectorFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/5324/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50c2VsZWN0b3IvU2VnbWVudFNlbGVjdG9yRmFjdG9yeS5qYXZh) | `60.00% <ø> (ø)` | |
   | [...mmon/assignment/InstanceAssignmentConfigUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/5324/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZUFzc2lnbm1lbnRDb25maWdVdGlscy5qYXZh) | `72.50% <ø> (ø)` | |
   | [...not/common/assignment/InstancePartitionsUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/5324/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZVBhcnRpdGlvbnNVdGlscy5qYXZh) | `73.17% <ø> (ø)` | |
   | ... and [239 more](https://codecov.io/gh/apache/incubator-pinot/pull/5324/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5324?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5324?src=pr&el=footer). Last update [6a1565c...935731f](https://codecov.io/gh/apache/incubator-pinot/pull/5324?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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.

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] [incubator-pinot] mcvsubbu commented on pull request #5324: Timespec to datetimespec conversion utility

Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on pull request #5324:
URL: https://github.com/apache/incubator-pinot/pull/5324#issuecomment-622578253


   Is the new util class to be used inside the Schema class (and everyone else outside sees only DateTimeFieldSpec)?


----------------------------------------------------------------
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.

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] [incubator-pinot] npawar commented on a change in pull request #5324: Timespec to datetimespec conversion utility

Posted by GitBox <gi...@apache.org>.
npawar commented on a change in pull request #5324:
URL: https://github.com/apache/incubator-pinot/pull/5324#discussion_r419631892



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/function/DateTimeFunctions.java
##########
@@ -35,9 +63,79 @@ static Long toEpochHours(Long millis) {
   }
 
   /**
-   * Convert epoch millis to epoch minutes, bucketed by given bucket granularity
+   * Convert epoch millis to epoch hours, bucketed by given bucket granularity
    */
-  static Long toEpochMinutes(Long millis, String bucket) {
-    return TimeUnit.MILLISECONDS.toMinutes(millis) / Integer.parseInt(bucket);
+  static Long toEpochHoursBucket(Long millis, String bucket) {

Review comment:
       Agreed, I don't like "bucket" here either, but couldn't think of a better one. Rounded does sound better, but i wonder if it'll be confusing.
   For example, consider millis 1578685189000 (2020/01/10 11:39:49)
   `toEpochSeconds(millis) = 1578685189`
   
   What we want is - `toEpochSeconds(millis, 10)` = 1578685189/10 = **157868518** i.e. divide the seconds to create **tenSecondsSinceEpoch**
   Rounding can be confused with - _keep the same format, but find the nearest 10 minutes value_. 
   `toEpochSecondsRounded(millis, 10)` = (1578685189/10) * 10 = **1578685180**
   
   Maybe we should solicit more opinions? Once this function starts being used, it'll be with us forever
   




----------------------------------------------------------------
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.

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] [incubator-pinot] npawar commented on pull request #5324: Timespec to datetimespec conversion utility

Posted by GitBox <gi...@apache.org>.
npawar commented on pull request #5324:
URL: https://github.com/apache/incubator-pinot/pull/5324#issuecomment-623597429


   > > > Is the new util class to be used inside the Schema class (and everyone else outside sees only DateTimeFieldSpec)?
   > > 
   > > 
   > > That's right. Only Schema class should know about this converter. Maybe it can even go into the Schema class
   > 
   > Yes, as methods in Schema class?
   
   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.

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] [incubator-pinot] mcvsubbu commented on a change in pull request #5324: Timespec to datetimespec conversion utility

Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #5324:
URL: https://github.com/apache/incubator-pinot/pull/5324#discussion_r420253436



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/function/DateTimeFunctions.java
##########
@@ -35,9 +53,127 @@ static Long toEpochHours(Long millis) {
   }
 
   /**
-   * Convert epoch millis to epoch minutes, bucketed by given bucket granularity
+   * Convert epoch millis to epoch days
+   */
+  static Long toEpochDays(Long millis) {
+    return TimeUnit.MILLISECONDS.toDays(millis);
+  }
+
+  /**
+   * Convert epoch millis to epoch seconds, round to nearest rounding bucket
+   */
+  static Long toEpochSecondsRounded(Long millis, String roundingValue) {
+    int roundToNearest = Integer.parseInt(roundingValue);
+    return (TimeUnit.MILLISECONDS.toSeconds(millis) / roundToNearest) * roundToNearest;
+  }
+
+  /**
+   * Convert epoch millis to epoch minutes, round to nearest rounding bucket
+   */
+  static Long toEpochMinutesRounded(Long millis, String roundingValue) {
+    int roundToNearest = Integer.parseInt(roundingValue);
+    return (TimeUnit.MILLISECONDS.toMinutes(millis) / roundToNearest) * roundToNearest;
+  }
+
+  /**
+   * Convert epoch millis to epoch hours, round to nearest rounding bucket
+   */
+  static Long toEpochHoursRounded(Long millis, String roundingValue) {
+    int roundToNearest = Integer.parseInt(roundingValue);
+    return (TimeUnit.MILLISECONDS.toHours(millis) / roundToNearest) * roundToNearest;
+  }
+
+  /**
+   * Convert epoch millis to epoch days, round to nearest rounding bucket
+   */
+  static Long toEpochDaysRounded(Long millis, String roundingValue) {
+    int roundToNearest = Integer.parseInt(roundingValue);
+    return (TimeUnit.MILLISECONDS.toDays(millis) / roundToNearest) * roundToNearest;
+  }
+
+  // TODO: toEpochXXXBucket methods are only needed to convert from TimeFieldSpec to DateTimeFieldSpec.

Review comment:
       It will be nice if you also add here 
   (1) an example of the use case in time-field-spec that needs bucketing in order to be backward compatible. 
   (2) A comment that use of these bucket functions is discouraged unless you know what you are doing (e.g. 5-minutes-since-epoch does not make sense to someone looking at the timestamp, or writing queries. instead, Millis-since-epoch rounded to 5 minutes makes a lot more sense).




----------------------------------------------------------------
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.

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] [incubator-pinot] npawar commented on a change in pull request #5324: Timespec to datetimespec conversion utility

Posted by GitBox <gi...@apache.org>.
npawar commented on a change in pull request #5324:
URL: https://github.com/apache/incubator-pinot/pull/5324#discussion_r419717201



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/function/DateTimeFunctions.java
##########
@@ -35,9 +63,79 @@ static Long toEpochHours(Long millis) {
   }
 
   /**
-   * Convert epoch millis to epoch minutes, bucketed by given bucket granularity
+   * Convert epoch millis to epoch hours, bucketed by given bucket granularity
    */
-  static Long toEpochMinutes(Long millis, String bucket) {
-    return TimeUnit.MILLISECONDS.toMinutes(millis) / Integer.parseInt(bucket);
+  static Long toEpochHoursBucket(Long millis, String bucket) {

Review comment:
       As per our offline conversation, we now have toEpochXXX, toEpochXXXRounded, toEpochXXXBucket. For example,
   1) toEpochSeconds(millis) - converts from millis to  epoch seconds
   2) toEpochSecondsRounded (millis, 10) - converts from millis to epoch seconds, and rounds to the smaller 10th value i.e. (seconds/10) * 10
   3) toEpochSecondsBucket(millis, 10) - converts from millis to epoch seconds and divides by bucket, to get tenSecondsFromEpoch i.e. seconds / 10




----------------------------------------------------------------
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.

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] [incubator-pinot] codecov-io commented on pull request #5324: Timespec to datetimespec conversion utility

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #5324:
URL: https://github.com/apache/incubator-pinot/pull/5324#issuecomment-622564254


   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5324?src=pr&el=h1) Report
   > Merging [#5324](https://codecov.io/gh/apache/incubator-pinot/pull/5324?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/bd79861e7bca87406fc184ec70ce504667f76daf&el=desc) will **decrease** coverage by `9.34%`.
   > The diff coverage is `63.63%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/5324/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz)](https://codecov.io/gh/apache/incubator-pinot/pull/5324?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #5324      +/-   ##
   ==========================================
   - Coverage   66.23%   56.88%   -9.35%     
   ==========================================
     Files        1066     1073       +7     
     Lines       54552    54665     +113     
     Branches     8128     8154      +26     
   ==========================================
   - Hits        36132    31097    -5035     
   - Misses      15774    21124    +5350     
   + Partials     2646     2444     -202     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/5324?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...e/pinot/broker/api/resources/PinotBrokerDebug.java](https://codecov.io/gh/apache/incubator-pinot/pull/5324/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdEJyb2tlckRlYnVnLmphdmE=) | `0.00% <ø> (-76.67%)` | :arrow_down: |
   | [.../BrokerResourceOnlineOfflineStateModelFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/5324/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclJlc291cmNlT25saW5lT2ZmbGluZVN0YXRlTW9kZWxGYWN0b3J5LmphdmE=) | `41.86% <ø> (-13.96%)` | :arrow_down: |
   | [...quota/HelixExternalViewBasedQueryQuotaManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/5324/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9IZWxpeEV4dGVybmFsVmlld0Jhc2VkUXVlcnlRdW90YU1hbmFnZXIuamF2YQ==) | `64.84% <ø> (-3.04%)` | :arrow_down: |
   | [...roker/requesthandler/BaseBrokerRequestHandler.java](https://codecov.io/gh/apache/incubator-pinot/pull/5324/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvQmFzZUJyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | `20.10% <ø> (-59.25%)` | :arrow_down: |
   | [...rg/apache/pinot/broker/routing/RoutingManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/5324/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9Sb3V0aW5nTWFuYWdlci5qYXZh) | `66.53% <ø> (-14.62%)` | :arrow_down: |
   | [...ting/instanceselector/InstanceSelectorFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/5324/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3JGYWN0b3J5LmphdmE=) | `71.42% <ø> (ø)` | |
   | [...er/routing/segmentpruner/SegmentPrunerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/5324/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL1NlZ21lbnRQcnVuZXJGYWN0b3J5LmphdmE=) | `83.33% <ø> (ø)` | |
   | [...outing/segmentselector/SegmentSelectorFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/5324/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50c2VsZWN0b3IvU2VnbWVudFNlbGVjdG9yRmFjdG9yeS5qYXZh) | `60.00% <ø> (ø)` | |
   | [...oker/routing/timeboundary/TimeBoundaryManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/5324/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy90aW1lYm91bmRhcnkvVGltZUJvdW5kYXJ5TWFuYWdlci5qYXZh) | `89.74% <ø> (+2.24%)` | :arrow_up: |
   | [...mmon/assignment/InstanceAssignmentConfigUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/5324/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZUFzc2lnbm1lbnRDb25maWdVdGlscy5qYXZh) | `67.50% <ø> (-5.00%)` | :arrow_down: |
   | ... and [526 more](https://codecov.io/gh/apache/incubator-pinot/pull/5324/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5324?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5324?src=pr&el=footer). Last update [6a1565c...c0fb8a2](https://codecov.io/gh/apache/incubator-pinot/pull/5324?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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.

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