You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "tibrewalpratik17 (via GitHub)" <gi...@apache.org> on 2023/07/04 20:58:29 UTC
[GitHub] [pinot] tibrewalpratik17 commented on a diff in pull request #11030: Improve `DateTimeGranularitySpec` to follow the convention of new dateTimeField format
tibrewalpratik17 commented on code in PR #11030:
URL: https://github.com/apache/pinot/pull/11030#discussion_r1252354689
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/DateTimeGranularitySpec.java:
##########
@@ -43,21 +49,41 @@ public class DateTimeGranularitySpec {
*/
public DateTimeGranularitySpec(String granularity) {
Preconditions.checkArgument(StringUtils.isNotEmpty(granularity), "Must provide granularity");
- String[] granularityTokens = StringUtil.split(granularity, SEPARATOR, 2);
- Preconditions.checkArgument(granularityTokens.length >= NUM_TOKENS,
- "Invalid granularity: %s, must be of format 'size:timeUnit", granularity);
+
+ int sizePosition;
+ int timeUnitPosition;
+ String[] granularityTokens;
+
+ if (Character.isDigit(granularity.charAt(0))) {
+ // Colon format
+ granularityTokens = StringUtil.split(granularity, COLON_SEPARATOR, 2);
+ Preconditions.checkArgument(granularityTokens.length >= NUM_TOKENS,
+ "Invalid granularity: %s, must be of format 'size:timeUnit", granularity);
Review Comment:
We can merge both the `Preconditions.checkArgument` behind a boolean flag and throw an error something like:
```suggestion
Preconditions.checkArgument(granularityTokens.length >= NUM_TOKENS,
"Invalid granularity: %s, must be of format 'size:timeUnit or timeUnit|size", granularity);
```
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/DateTimeGranularitySpec.java:
##########
@@ -83,9 +109,9 @@ public TimeUnit getTimeUnit() {
/**
* Converts a granularity to millis.
* <ul>
- * <li>1) granularityToMillis(1:HOURS) = 3600000 (60*60*1000)</li>
- * <li>2) granularityToMillis(1:MILLISECONDS) = 1</li>
- * <li>3) granularityToMillis(15:MINUTES) = 900000 (15*60*1000)</li>
Review Comment:
let's not remove this but mark as old format?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/DateTimeGranularitySpec.java:
##########
@@ -29,10 +29,16 @@
* Class to represent granularity from {@link DateTimeFieldSpec}
*/
public class DateTimeGranularitySpec {
- // 'size:timeUnit'
- private static final char SEPARATOR = ':';
- private static final int SIZE_POSITION = 0;
- private static final int TIME_UNIT_POSITION = 1;
+ // Colon format:
Review Comment:
nit: let's add the format in the comment as it was originally present
--
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