You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "egalpin (via GitHub)" <gi...@apache.org> on 2023/06/03 22:18:35 UTC
[GitHub] [pinot] egalpin opened a new pull request, #10841: [feature] multi-value datetime transform variants
egalpin opened a new pull request, #10841:
URL: https://github.com/apache/pinot/pull/10841
Closes #10838
--
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 #10841: [feature] multi-value datetime transform variants
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10841:
URL: https://github.com/apache/pinot/pull/10841#discussion_r1218709548
##########
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/DateTimeFunctions.java:
##########
@@ -642,4 +1138,30 @@ public static long timestampDiff(String unit, long timestamp1, long timestamp2)
ISOChronology chronology = ISOChronology.getInstanceUTC();
return DateTimeUtils.getTimestampField(chronology, unit).getDifferenceAsLong(timestamp2, timestamp1);
}
+ @ScalarFunction(names = {"timestampDiffMVMV", "dateDiffMVMV"})
+ public static long[] timestampDiffMVMV(String unit, long[] timestamp1, long[] timestamp2) {
+ assert timestamp1.length == timestamp2.length;
Review Comment:
Don't use assert here as it won't be executed in production. We should explicitly check and throw exception if the length doesn't match
##########
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/DateTimeFunctions.java:
##########
@@ -642,4 +1138,30 @@ public static long timestampDiff(String unit, long timestamp1, long timestamp2)
ISOChronology chronology = ISOChronology.getInstanceUTC();
return DateTimeUtils.getTimestampField(chronology, unit).getDifferenceAsLong(timestamp2, timestamp1);
}
+ @ScalarFunction(names = {"timestampDiffMVMV", "dateDiffMVMV"})
+ public static long[] timestampDiffMVMV(String unit, long[] timestamp1, long[] timestamp2) {
Review Comment:
We should revisit these method names, they are kind of confusing now
##########
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/DateTimeFunctions.java:
##########
@@ -78,6 +78,14 @@ private DateTimeFunctions() {
public static long toEpochSeconds(long millis) {
return TimeUnit.MILLISECONDS.toSeconds(millis);
}
+ @ScalarFunction
Review Comment:
（minor) Please add an empty line between methods, same for other places
--
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] egalpin commented on a diff in pull request #10841: [feature] multi-value datetime transform variants
Posted by "egalpin (via GitHub)" <gi...@apache.org>.
egalpin commented on code in PR #10841:
URL: https://github.com/apache/pinot/pull/10841#discussion_r1220487877
##########
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/DateTimeFunctions.java:
##########
@@ -642,4 +1138,30 @@ public static long timestampDiff(String unit, long timestamp1, long timestamp2)
ISOChronology chronology = ISOChronology.getInstanceUTC();
return DateTimeUtils.getTimestampField(chronology, unit).getDifferenceAsLong(timestamp2, timestamp1);
}
+ @ScalarFunction(names = {"timestampDiffMVMV", "dateDiffMVMV"})
+ public static long[] timestampDiffMVMV(String unit, long[] timestamp1, long[] timestamp2) {
Review Comment:
Agreed haha. I removed the `MVMV` variant altogether in https://github.com/apache/pinot/pull/10841/commits/39a2fcdad5489ed5bdfdcd095c9c77ffd90d0380 because I think it would be prone to error (users relying on ordering of MV columns).
--
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 #10841: [feature] multi-value datetime transform variants
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #10841:
URL: https://github.com/apache/pinot/pull/10841
--
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 #10841: [feature] multi-value datetime transform variants
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10841:
URL: https://github.com/apache/pinot/pull/10841#issuecomment-1575255538
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/10841?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#10841](https://app.codecov.io/gh/apache/pinot/pull/10841?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (8a97ba3) into [master](https://app.codecov.io/gh/apache/pinot/commit/6dbd9e23fef8d326fa2f2467ae8a10e2b3fdb428?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (6dbd9e2) will **decrease** coverage by `10.28%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10841 +/- ##
=============================================
- Coverage 34.25% 23.97% -10.28%
+ Complexity 462 49 -413
=============================================
Files 2170 2154 -16
Lines 116650 116513 -137
Branches 17654 17670 +16
=============================================
- Hits 39959 27938 -12021
- Misses 73201 85622 +12421
+ Partials 3490 2953 -537
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `23.97% <0.00%> (+0.07%)` | :arrow_up: |
| integration2 | `?` | |
| unittests2 | `?` | |
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/10841?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...inot/common/function/scalar/DateTimeFunctions.java](https://app.codecov.io/gh/apache/pinot/pull/10841?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vc2NhbGFyL0RhdGVUaW1lRnVuY3Rpb25zLmphdmE=) | `2.73% <0.00%> (-9.95%)` | :arrow_down: |
... and [513 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/10841/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] egalpin commented on pull request #10841: [feature] multi-value datetime transform variants
Posted by "egalpin (via GitHub)" <gi...@apache.org>.
egalpin commented on PR #10841:
URL: https://github.com/apache/pinot/pull/10841#issuecomment-1577581308
@Jackie-Jiang should there be extra tests added given that these functions all call previously tested functions in a loop? I suppose I know that the answer is yes, but I'm trying to be lazy haha.
--
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