You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "s0nskar (via GitHub)" <gi...@apache.org> on 2023/07/07 12:50:27 UTC
[GitHub] [pinot] s0nskar opened a new pull request, #11057: Support for new dataTime format in `DateTimeGranularitySpec` without explicitly setting size
s0nskar opened a new pull request, #11057:
URL: https://github.com/apache/pinot/pull/11057
This is a follow-up PR for issue [#11028](https://github.com/apache/pinot/issues/11028)
Adding support for new dataTime format in `DateTimeGranularitySpec` without explicitly setting the size ( ref comment by @Jackie-Jiang : https://github.com/apache/pinot/pull/11030#pullrequestreview-1515740305)
Test Plan:
- Current UT passed
- Added new UT's
--
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] Jackie-Jiang merged pull request #11057: Support for new dataTime format in `DateTimeGranularitySpec` without explicitly setting size
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #11057:
URL: https://github.com/apache/pinot/pull/11057
--
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] Jackie-Jiang commented on pull request #11057: Support for new dataTime format in `DateTimeGranularitySpec` without explicitly setting size
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #11057:
URL: https://github.com/apache/pinot/pull/11057#issuecomment-1631656800
Great job! Can you also help update the [Pinot doc](https://docs.pinot.apache.org/configuration-reference/schema#datetimefieldspec) reflecting this new 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
[GitHub] [pinot] s0nskar commented on pull request #11057: Support for new dataTime format in `DateTimeGranularitySpec` without explicitly setting size
Posted by "s0nskar (via GitHub)" <gi...@apache.org>.
s0nskar commented on PR #11057:
URL: https://github.com/apache/pinot/pull/11057#issuecomment-1625371811
cc: @Jackie-Jiang @snleee PTAL
--
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] s0nskar commented on a diff in pull request #11057: Support for new dataTime format in `DateTimeGranularitySpec` without explicitly setting size
Posted by "s0nskar (via GitHub)" <gi...@apache.org>.
s0nskar commented on code in PR #11057:
URL: https://github.com/apache/pinot/pull/11057#discussion_r1256986968
##########
pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/DateTimeConversionTransformFunctionTest.java:
##########
@@ -66,7 +66,7 @@ public Object[][] testIllegalArguments() {
}, new Object[]{"dateTimeConvert(5,'1:MILLISECONDS:EPOCH','1:MINUTES:EPOCH','1:MINUTES')"}, new Object[]{
String.format("dateTimeConvert(%s,'1:MILLISECONDS:EPOCH','1:MINUTES:EPOCH','1:MINUTES')", INT_MV_COLUMN)
}, new Object[]{
- String.format("dateTimeConvert(%s,'1:MILLISECONDS:EPOCH','1:MINUTES:EPOCH','MINUTES')", TIME_COLUMN)
+ String.format("dateTimeConvert(%s,'1:MILLISECONDS:EPOCH','1:MINUTES:EPOCH','MINUTES:1')", TIME_COLUMN)
Review Comment:
Fixing failing test, as `MINUTES` was getting considered as a valid granularity after this diff.
--
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] s0nskar commented on a diff in pull request #11057: Support for new dataTime format in `DateTimeGranularitySpec` without explicitly setting size
Posted by "s0nskar (via GitHub)" <gi...@apache.org>.
s0nskar commented on code in PR #11057:
URL: https://github.com/apache/pinot/pull/11057#discussion_r1256871324
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/DateTimeGranularitySpec.java:
##########
@@ -53,38 +55,49 @@ public DateTimeGranularitySpec(String granularity) {
char separator;
int sizePosition;
int timeUnitPosition;
+ int minTokens = NUM_TOKENS;
+ int maxTokens = NUM_TOKENS;
Review Comment:
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.
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] s0nskar commented on pull request #11057: Support for new dataTime format in `DateTimeGranularitySpec` without explicitly setting size
Posted by "s0nskar (via GitHub)" <gi...@apache.org>.
s0nskar commented on PR #11057:
URL: https://github.com/apache/pinot/pull/11057#issuecomment-1626869186
Thanks, @Jackie-Jiang for the review. Addressed 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.
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] s0nskar commented on a diff in pull request #11057: Support for new dataTime format in `DateTimeGranularitySpec` without explicitly setting size
Posted by "s0nskar (via GitHub)" <gi...@apache.org>.
s0nskar commented on code in PR #11057:
URL: https://github.com/apache/pinot/pull/11057#discussion_r1256872370
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/DateTimeGranularitySpec.java:
##########
@@ -53,38 +55,49 @@ public DateTimeGranularitySpec(String granularity) {
char separator;
int sizePosition;
int timeUnitPosition;
+ int minTokens = NUM_TOKENS;
+ int maxTokens = NUM_TOKENS;
if (Character.isDigit(granularity.charAt(0))) {
// Colon format: 'size:timeUnit'
separator = COLON_SEPARATOR;
sizePosition = COLON_FORMAT_SIZE_POSITION;
timeUnitPosition = COLON_FORMAT_TIME_UNIT_POSITION;
} else {
- // Pipe format: 'timeUnit|size'
+ // Pipe format: 'timeUnit(|size)'
separator = PIPE_SEPARATOR;
sizePosition = PIPE_FORMAT_SIZE_POSITION;
timeUnitPosition = PIPE_FORMAT_TIME_UNIT_POSITION;
+
+ minTokens = PIPE_FORMAT_MIN_TOKENS;
+ maxTokens = PIPE_FORMAT_MAX_TOKENS;
}
String[] granularityTokens = StringUtil.split(granularity, separator, 2);
- Preconditions.checkArgument(granularityTokens.length >= NUM_TOKENS,
- "Invalid granularity: %s, must be of format 'timeUnit|size' or 'size:timeUnit'", granularity);
+ Preconditions.checkArgument(granularityTokens.length >= minTokens && granularityTokens.length <= maxTokens,
+ "Invalid granularity: %s, must be of format 'timeUnit(|size)' or 'size:timeUnit'", granularity);
try {
- _size = Integer.parseInt(granularityTokens[sizePosition]);
+ _timeUnit = TimeUnit.valueOf(granularityTokens[timeUnitPosition]);
} catch (Exception e) {
throw new IllegalArgumentException(
- String.format("Invalid size: %s in granularity: %s", granularityTokens[sizePosition], granularity));
+ String.format("Invalid time unit: %s in granularity: %s", granularityTokens[timeUnitPosition],
+ granularity));
+ }
+
+ // New format without explicitly setting size - use default size = 1
+ if (granularityTokens.length == 1) {
Review Comment:
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.
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] Jackie-Jiang commented on a diff in pull request #11057: Support for new dataTime format in `DateTimeGranularitySpec` without explicitly setting size
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #11057:
URL: https://github.com/apache/pinot/pull/11057#discussion_r1256777533
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/DateTimeGranularitySpec.java:
##########
@@ -53,38 +55,49 @@ public DateTimeGranularitySpec(String granularity) {
char separator;
int sizePosition;
int timeUnitPosition;
+ int minTokens = NUM_TOKENS;
+ int maxTokens = NUM_TOKENS;
Review Comment:
(minor) Move the assignment into the if check for readability
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/DateTimeGranularitySpec.java:
##########
@@ -53,38 +55,49 @@ public DateTimeGranularitySpec(String granularity) {
char separator;
int sizePosition;
int timeUnitPosition;
+ int minTokens = NUM_TOKENS;
+ int maxTokens = NUM_TOKENS;
if (Character.isDigit(granularity.charAt(0))) {
// Colon format: 'size:timeUnit'
separator = COLON_SEPARATOR;
sizePosition = COLON_FORMAT_SIZE_POSITION;
timeUnitPosition = COLON_FORMAT_TIME_UNIT_POSITION;
} else {
- // Pipe format: 'timeUnit|size'
+ // Pipe format: 'timeUnit(|size)'
separator = PIPE_SEPARATOR;
sizePosition = PIPE_FORMAT_SIZE_POSITION;
timeUnitPosition = PIPE_FORMAT_TIME_UNIT_POSITION;
+
+ minTokens = PIPE_FORMAT_MIN_TOKENS;
+ maxTokens = PIPE_FORMAT_MAX_TOKENS;
}
String[] granularityTokens = StringUtil.split(granularity, separator, 2);
- Preconditions.checkArgument(granularityTokens.length >= NUM_TOKENS,
- "Invalid granularity: %s, must be of format 'timeUnit|size' or 'size:timeUnit'", granularity);
+ Preconditions.checkArgument(granularityTokens.length >= minTokens && granularityTokens.length <= maxTokens,
+ "Invalid granularity: %s, must be of format 'timeUnit(|size)' or 'size:timeUnit'", granularity);
try {
- _size = Integer.parseInt(granularityTokens[sizePosition]);
+ _timeUnit = TimeUnit.valueOf(granularityTokens[timeUnitPosition]);
} catch (Exception e) {
throw new IllegalArgumentException(
- String.format("Invalid size: %s in granularity: %s", granularityTokens[sizePosition], granularity));
+ String.format("Invalid time unit: %s in granularity: %s", granularityTokens[timeUnitPosition],
+ granularity));
+ }
+
+ // New format without explicitly setting size - use default size = 1
+ if (granularityTokens.length == 1) {
Review Comment:
(minor)
```suggestion
if (granularityTokens.length == PIPE_FORMAT_MIN_TOKENS) {
```
--
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] s0nskar commented on pull request #11057: Support for new dataTime format in `DateTimeGranularitySpec` without explicitly setting size
Posted by "s0nskar (via GitHub)" <gi...@apache.org>.
s0nskar commented on PR #11057:
URL: https://github.com/apache/pinot/pull/11057#issuecomment-1631865842
@Jackie-Jiang Will do 👍
--
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