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/04 15:09:06 UTC
[GitHub] [pinot] s0nskar opened a new pull request, #11030: Improve `DateTimeGranularitySpec` to follow the convention of new dateTimeField
s0nskar opened a new pull request, #11030:
URL: https://github.com/apache/pinot/pull/11030
Adding support for new datetime format for DateTimeGranularitySpec as well.
Proposed change:
`
old format:
size:timeUnit -> 1:MILLISECONDS
new format:
timeUnit|size -> MILLISECONDS|1
`
--
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 #11030: Improve `DateTimeGranularitySpec` to follow the convention of new dateTimeField format
Posted by "s0nskar (via GitHub)" <gi...@apache.org>.
s0nskar commented on PR #11030:
URL: https://github.com/apache/pinot/pull/11030#issuecomment-1621926243
Added unit tests.
--
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 #11030: Improve `DateTimeGranularitySpec` to follow the convention of new dateTimeField format
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #11030:
URL: https://github.com/apache/pinot/pull/11030#discussion_r1253920050
##########
pinot-spi/src/test/java/org/apache/pinot/spi/data/DateTimeGranularitySpecTest.java:
##########
@@ -0,0 +1,70 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.spi.data;
+
+import java.util.concurrent.TimeUnit;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertThrows;
+
+public class DateTimeGranularitySpecTest {
+
+ @Test
Review Comment:
Please reformat the changes with [Pinot Style](https://docs.pinot.apache.org/developers/developers-and-contributors/code-setup#intellij)
--
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 #11030: Improve `DateTimeGranularitySpec` to follow the convention of new dateTimeField format
Posted by "s0nskar (via GitHub)" <gi...@apache.org>.
s0nskar commented on code in PR #11030:
URL: https://github.com/apache/pinot/pull/11030#discussion_r1253965322
##########
pinot-spi/src/test/java/org/apache/pinot/spi/data/DateTimeGranularitySpecTest.java:
##########
@@ -0,0 +1,70 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.spi.data;
+
+import java.util.concurrent.TimeUnit;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertThrows;
+
+public class DateTimeGranularitySpecTest {
+
+ @Test
Review Comment:
Thanks @Jackie-Jiang for pointing it out. Fixed it.
--
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] tibrewalpratik17 commented on a diff in pull request #11030: Improve `DateTimeGranularitySpec` to follow the convention of new dateTimeField format
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
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
[GitHub] [pinot] s0nskar commented on pull request #11030: Improve `DateTimeGranularitySpec` to follow the convention of new dateTimeField format
Posted by "s0nskar (via GitHub)" <gi...@apache.org>.
s0nskar commented on PR #11030:
URL: https://github.com/apache/pinot/pull/11030#issuecomment-1623033782
> Thank you for the quick action on this ticket!
>
> LGTM other than the formatting that @Jackie-Jiang mentioned.
@Jackie-Jiang I can take this in a follow-up PR and update the documentation for the same as well
--
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 #11030: Improve `DateTimeGranularitySpec` to follow the convention of new dateTimeField format
Posted by "s0nskar (via GitHub)" <gi...@apache.org>.
s0nskar commented on PR #11030:
URL: https://github.com/apache/pinot/pull/11030#issuecomment-1622290462
@Jackie-Jiang test failure look unrelated to the change, can you please retrigger the failing tests.
--
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 #11030: Improve `DateTimeGranularitySpec` to follow the convention of new dateTimeField format
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #11030:
URL: https://github.com/apache/pinot/pull/11030
--
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 #11030: Improve `DateTimeGranularitySpec` to follow the convention of new dateTimeField format
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #11030:
URL: https://github.com/apache/pinot/pull/11030#issuecomment-1622221875
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/11030?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#11030](https://app.codecov.io/gh/apache/pinot/pull/11030?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (5db7260) into [master](https://app.codecov.io/gh/apache/pinot/commit/f7a076496ae6e07a42207cb2c978dc74562cf0cf?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (f7a0764) will **increase** coverage by `0.00%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #11030 +/- ##
=========================================
Coverage 0.11% 0.11%
=========================================
Files 2200 2146 -54
Lines 118829 116331 -2498
Branches 18023 17722 -301
=========================================
Hits 137 137
+ Misses 118672 116174 -2498
Partials 20 20
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1temurin11 | `?` | |
| integration1temurin17 | `?` | |
| integration1temurin20 | `?` | |
| integration2temurin17 | `?` | |
| integration2temurin20 | `?` | |
| unittests1temurin11 | `?` | |
| unittests1temurin17 | `?` | |
| unittests1temurin20 | `?` | |
| unittests2temurin11 | `?` | |
| unittests2temurin17 | `0.11% <0.00%> (ø)` | |
| unittests2temurin20 | `0.11% <0.00%> (ø)` | |
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=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/pinot/pull/11030?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...apache/pinot/spi/data/DateTimeGranularitySpec.java](https://app.codecov.io/gh/apache/pinot/pull/11030?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9EYXRlVGltZUdyYW51bGFyaXR5U3BlYy5qYXZh) | `0.00% <0.00%> (ø)` | |
... and [56 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/11030/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
: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=apache)
--
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 #11030: Improve `DateTimeGranularitySpec` to follow the convention of new dateTimeField format
Posted by "s0nskar (via GitHub)" <gi...@apache.org>.
s0nskar commented on PR #11030:
URL: https://github.com/apache/pinot/pull/11030#issuecomment-1620979318
Thanks @tibrewalpratik17 for review. Addressed comments 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 pull request #11030: Improve `DateTimeGranularitySpec` to follow the convention of new dateTimeField format
Posted by "s0nskar (via GitHub)" <gi...@apache.org>.
s0nskar commented on PR #11030:
URL: https://github.com/apache/pinot/pull/11030#issuecomment-1623034383
> Can we also add support without explicitly setting the size? E.g. `MILLISECONDS`
@Jackie-Jiang I can take this in a follow-up PR and update the documentation for the same as well
--
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 commented on a diff in pull request #11030: Improve `DateTimeGranularitySpec` to follow the convention of new dateTimeField format
Posted by "snleee (via GitHub)" <gi...@apache.org>.
snleee commented on code in PR #11030:
URL: https://github.com/apache/pinot/pull/11030#discussion_r1253941126
##########
pinot-spi/src/test/java/org/apache/pinot/spi/data/DateTimeGranularitySpecTest.java:
##########
@@ -0,0 +1,70 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.spi.data;
+
+import java.util.concurrent.TimeUnit;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertThrows;
+
+public class DateTimeGranularitySpecTest {
+
+ @Test
Review Comment:
+1
--
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