You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "xiangfu0 (via GitHub)" <gi...@apache.org> on 2023/07/13 03:38:58 UTC
[GitHub] [pinot] xiangfu0 opened a new pull request, #11097: Register dateTimeConverter,timeConvert,dateTrunc to v2 functions
xiangfu0 opened a new pull request, #11097:
URL: https://github.com/apache/pinot/pull/11097
Register dateTimeConverter,timeConvert,dateTrunc to v2 functions
--
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] walterddr commented on a diff in pull request #11097: Register dateTimeConverter,timeConvert,dateTrunc, regexpReplace to v2 functions
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #11097:
URL: https://github.com/apache/pinot/pull/11097#discussion_r1263762196
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/MultiStageEngineIntegrationTest.java:
##########
@@ -116,6 +127,364 @@ public void testMultiValueColumnSelectionQuery()
testQueryWithMatchingRowCount(pinotQuery, h2Query);
}
+ @Test
+ public void testTimeFunc()
+ throws Exception {
+ String sqlQuery = "SELECT toDateTime(now(), 'yyyy-MM-dd z'), toDateTime(ago('PT1H'), 'yyyy-MM-dd z') FROM mytable";
+ JsonNode response = postQuery(sqlQuery);
+ System.out.println("response = " + response);
+ String todayStr = response.get("resultTable").get("rows").get(0).get(0).asText();
+ String expectedTodayStr =
+ Instant.now().atZone(ZoneId.of("UTC")).format(DateTimeFormatter.ofPattern("yyyy-MM-dd z"));
+ assertEquals(todayStr, expectedTodayStr);
+
+ String oneHourAgoTodayStr = response.get("resultTable").get("rows").get(0).get(1).asText();
+ String expectedOneHourAgoTodayStr = Instant.now().minus(Duration.parse("PT1H")).atZone(ZoneId.of("UTC"))
+ .format(DateTimeFormatter.ofPattern("yyyy-MM-dd z"));
+ assertEquals(oneHourAgoTodayStr, expectedOneHourAgoTodayStr);
+ }
+
+ @Test
+ public void testRegexpReplace()
Review Comment:
1. is there a better way to reuse these tests on v1 and v2?
2. we should also run these function after a joined (if they are supported)
--
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] xiangfu0 commented on a diff in pull request #11097: Register dateTimeConverter,timeConvert,dateTrunc, regexpReplace to v2 functions
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #11097:
URL: https://github.com/apache/pinot/pull/11097#discussion_r1264363993
##########
pinot-common/src/main/java/org/apache/pinot/common/function/TransformFunctionType.java:
##########
@@ -134,7 +152,14 @@ public enum TransformFunctionType {
CLPDECODE("clpDecode"),
// Regexp functions
- REGEXP_EXTRACT("regexpExtract"),
+ REGEXP_EXTRACT("regexpExtract", "regexp_extract"),
+ REGEXPREPLACE("regexpReplace",
+ ReturnTypes.VARCHAR_2000_NULLABLE,
Review Comment:
there is no `ReturnTypes.VARCHAR`, only `VARCHAR_4`or `VARCHAR_2000`
<img width="344" alt="image" src="https://github.com/apache/pinot/assets/1202120/afc15901-bcfa-4e1d-9186-9a7363362742">
--
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] xiangfu0 commented on a diff in pull request #11097: Register dateTimeConverter,timeConvert,dateTrunc, regexpReplace to v2 functions
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #11097:
URL: https://github.com/apache/pinot/pull/11097#discussion_r1264364074
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/MultiStageEngineIntegrationTest.java:
##########
@@ -116,6 +127,364 @@ public void testMultiValueColumnSelectionQuery()
testQueryWithMatchingRowCount(pinotQuery, h2Query);
}
+ @Test
+ public void testTimeFunc()
+ throws Exception {
+ String sqlQuery = "SELECT toDateTime(now(), 'yyyy-MM-dd z'), toDateTime(ago('PT1H'), 'yyyy-MM-dd z') FROM mytable";
+ JsonNode response = postQuery(sqlQuery);
+ System.out.println("response = " + response);
+ String todayStr = response.get("resultTable").get("rows").get(0).get(0).asText();
+ String expectedTodayStr =
+ Instant.now().atZone(ZoneId.of("UTC")).format(DateTimeFormatter.ofPattern("yyyy-MM-dd z"));
+ assertEquals(todayStr, expectedTodayStr);
+
+ String oneHourAgoTodayStr = response.get("resultTable").get("rows").get(0).get(1).asText();
+ String expectedOneHourAgoTodayStr = Instant.now().minus(Duration.parse("PT1H")).atZone(ZoneId.of("UTC"))
+ .format(DateTimeFormatter.ofPattern("yyyy-MM-dd z"));
+ assertEquals(oneHourAgoTodayStr, expectedOneHourAgoTodayStr);
+ }
+
+ @Test
+ public void testRegexpReplace()
Review Comment:
will add more 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] xiangfu0 merged pull request #11097: Register dateTimeConverter,timeConvert,dateTrunc, regexpReplace to v2 functions
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #11097:
URL: https://github.com/apache/pinot/pull/11097
--
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] walterddr commented on a diff in pull request #11097: Register dateTimeConverter,timeConvert,dateTrunc, regexpReplace to v2 functions
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #11097:
URL: https://github.com/apache/pinot/pull/11097#discussion_r1263761378
##########
pinot-common/src/main/java/org/apache/pinot/common/function/TransformFunctionType.java:
##########
@@ -134,7 +152,14 @@ public enum TransformFunctionType {
CLPDECODE("clpDecode"),
// Regexp functions
- REGEXP_EXTRACT("regexpExtract"),
+ REGEXP_EXTRACT("regexpExtract", "regexp_extract"),
+ REGEXPREPLACE("regexpReplace",
+ ReturnTypes.VARCHAR_2000_NULLABLE,
Review Comment:
we dont have 2000 limit on VARCHAR so there's no need to add the cap in varchar (or do we?)
--
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] walterddr commented on a diff in pull request #11097: Register dateTimeConverter,timeConvert,dateTrunc, regexpReplace to v2 functions
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #11097:
URL: https://github.com/apache/pinot/pull/11097#discussion_r1263762196
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/MultiStageEngineIntegrationTest.java:
##########
@@ -116,6 +127,364 @@ public void testMultiValueColumnSelectionQuery()
testQueryWithMatchingRowCount(pinotQuery, h2Query);
}
+ @Test
+ public void testTimeFunc()
+ throws Exception {
+ String sqlQuery = "SELECT toDateTime(now(), 'yyyy-MM-dd z'), toDateTime(ago('PT1H'), 'yyyy-MM-dd z') FROM mytable";
+ JsonNode response = postQuery(sqlQuery);
+ System.out.println("response = " + response);
+ String todayStr = response.get("resultTable").get("rows").get(0).get(0).asText();
+ String expectedTodayStr =
+ Instant.now().atZone(ZoneId.of("UTC")).format(DateTimeFormatter.ofPattern("yyyy-MM-dd z"));
+ assertEquals(todayStr, expectedTodayStr);
+
+ String oneHourAgoTodayStr = response.get("resultTable").get("rows").get(0).get(1).asText();
+ String expectedOneHourAgoTodayStr = Instant.now().minus(Duration.parse("PT1H")).atZone(ZoneId.of("UTC"))
+ .format(DateTimeFormatter.ofPattern("yyyy-MM-dd z"));
+ assertEquals(oneHourAgoTodayStr, expectedOneHourAgoTodayStr);
+ }
+
+ @Test
+ public void testRegexpReplace()
Review Comment:
1. is there a better way to reuse these tests on v1 and v2?
--
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] walterddr commented on a diff in pull request #11097: Register dateTimeConverter,timeConvert,dateTrunc, regexpReplace to v2 functions
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #11097:
URL: https://github.com/apache/pinot/pull/11097#discussion_r1263760742
##########
pinot-common/src/main/java/org/apache/pinot/common/function/TransformFunctionType.java:
##########
@@ -98,9 +100,25 @@ public enum TransformFunctionType {
OperandTypes.family(ImmutableList.of(SqlTypeFamily.ANY, SqlTypeFamily.CHARACTER)), "json_extract_key"),
// date time functions
- TIMECONVERT("timeConvert", "time_convert"),
- DATETIMECONVERT("dateTimeConvert", "date_time_convert"),
- DATETRUNC("dateTrunc", "datetrunc"),
+ TIMECONVERT("timeConvert",
+ ReturnTypes.BIGINT_FORCE_NULLABLE,
+ OperandTypes.family(ImmutableList.of(SqlTypeFamily.ANY, SqlTypeFamily.CHARACTER, SqlTypeFamily.CHARACTER)),
+ "time_convert"),
+
+ DATETIMECONVERT("dateTimeConvert",
+ ReturnTypes.cascade(
+ opBinding -> dateTimeConverterReturnTypeInference(opBinding),
+ SqlTypeTransforms.FORCE_NULLABLE),
Review Comment:
this one is not force nulllable. dateTime convert is only null when input is null (unlike json extract)
--
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 #11097: Register dateTimeConverter,timeConvert,dateTrunc to v2 functions
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #11097:
URL: https://github.com/apache/pinot/pull/11097#issuecomment-1633518930
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/11097?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#11097](https://app.codecov.io/gh/apache/pinot/pull/11097?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (0f9a261) into [master](https://app.codecov.io/gh/apache/pinot/commit/b39329966f8aff7bf3d7e7b05fad5f78e07ebc6b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (b393299) will **increase** coverage by `0.00%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #11097 +/- ##
=========================================
Coverage 0.11% 0.11%
=========================================
Files 2203 2148 -55
Lines 118144 115637 -2507
Branches 17877 17575 -302
=========================================
Hits 137 137
+ Misses 117987 115480 -2507
Partials 20 20
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1temurin11 | `?` | |
| integration1temurin17 | `?` | |
| integration1temurin20 | `?` | |
| integration2temurin11 | `?` | |
| integration2temurin17 | `?` | |
| integration2temurin20 | `?` | |
| unittests1temurin11 | `?` | |
| unittests1temurin17 | `?` | |
| unittests1temurin20 | `?` | |
| unittests2temurin11 | `?` | |
| unittests2temurin17 | `0.11% <0.00%> (-0.01%)` | :arrow_down: |
| unittests2temurin20 | `?` | |
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/11097?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...e/pinot/common/function/TransformFunctionType.java](https://app.codecov.io/gh/apache/pinot/pull/11097?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vVHJhbnNmb3JtRnVuY3Rpb25UeXBlLmphdmE=) | `0.00% <0.00%> (ø)` | |
... and [57 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/11097/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] xiangfu0 commented on a diff in pull request #11097: Register dateTimeConverter,timeConvert,dateTrunc, regexpReplace to v2 functions
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #11097:
URL: https://github.com/apache/pinot/pull/11097#discussion_r1267117537
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/MultiStageEngineIntegrationTest.java:
##########
@@ -116,6 +127,364 @@ public void testMultiValueColumnSelectionQuery()
testQueryWithMatchingRowCount(pinotQuery, h2Query);
}
+ @Test
+ public void testTimeFunc()
+ throws Exception {
+ String sqlQuery = "SELECT toDateTime(now(), 'yyyy-MM-dd z'), toDateTime(ago('PT1H'), 'yyyy-MM-dd z') FROM mytable";
+ JsonNode response = postQuery(sqlQuery);
+ System.out.println("response = " + response);
+ String todayStr = response.get("resultTable").get("rows").get(0).get(0).asText();
+ String expectedTodayStr =
+ Instant.now().atZone(ZoneId.of("UTC")).format(DateTimeFormatter.ofPattern("yyyy-MM-dd z"));
+ assertEquals(todayStr, expectedTodayStr);
+
+ String oneHourAgoTodayStr = response.get("resultTable").get("rows").get(0).get(1).asText();
+ String expectedOneHourAgoTodayStr = Instant.now().minus(Duration.parse("PT1H")).atZone(ZoneId.of("UTC"))
+ .format(DateTimeFormatter.ofPattern("yyyy-MM-dd z"));
+ assertEquals(oneHourAgoTodayStr, expectedOneHourAgoTodayStr);
+ }
+
+ @Test
+ public void testRegexpReplace()
Review Comment:
For 1, since eventually we will deprecate v1, then I think it's ok to copy over tests gradually.
--
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