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