You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "61yao (via GitHub)" <gi...@apache.org> on 2023/04/06 03:20:20 UTC
[GitHub] [pinot] 61yao opened a new pull request, #10562: [feature] [null support # 5] Support null in scalar transform function wrapper
61yao opened a new pull request, #10562:
URL: https://github.com/apache/pinot/pull/10562
Support null in scalar transform.
Scalar function invocation will determine whether the result is null or not.
--
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] 61yao commented on pull request #10562: [feature] [null support # 5] Support null in scalar transform function wrapper
Posted by "61yao (via GitHub)" <gi...@apache.org>.
61yao commented on PR #10562:
URL: https://github.com/apache/pinot/pull/10562#issuecomment-1505759545
> nullableParameters
nullableParameters is still needed because when it is set to false, we return null if any of the argument is null:
You can check this file for details: https://github.com/apache/pinot/pull/10376/files#diff-688b9166dc63a0dbc1f1726deb4847284cc4f2ebbf9ccbab64c4631e68c33c23
--
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 #10562: [feature] [null support # 5] Support null in scalar transform function wrapper
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #10562:
URL: https://github.com/apache/pinot/pull/10562
--
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] 61yao commented on a diff in pull request #10562: [feature] [null support # 5] Support null in scalar transform function wrapper
Posted by "61yao (via GitHub)" <gi...@apache.org>.
61yao commented on code in PR #10562:
URL: https://github.com/apache/pinot/pull/10562#discussion_r1166231507
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/ScalarTransformFunctionWrapper.java:
##########
@@ -85,14 +89,45 @@ public void init(List<TransformFunction> arguments, Map<String, ColumnContext> c
"Wrong number of arguments for method: %s, expected: %s, actual: %s", _functionInvoker.getMethod(),
parameterTypes.length, numArguments);
- _arguments = new Object[numArguments];
+ _scalarArguments = new Object[numArguments];
_nonLiteralIndices = new int[numArguments];
_nonLiteralFunctions = new TransformFunction[numArguments];
for (int i = 0; i < numArguments; i++) {
TransformFunction transformFunction = arguments.get(i);
if (transformFunction instanceof LiteralTransformFunction) {
- String literal = ((LiteralTransformFunction) transformFunction).getStringLiteral();
- _arguments[i] = parameterTypes[i].convert(literal, PinotDataType.STRING);
+ LiteralTransformFunction literalTransformFunction = (LiteralTransformFunction) transformFunction;
+ DataType dataType = literalTransformFunction.getResultMetadata().getDataType();
Review Comment:
Will fix in another PR.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/LiteralTransformFunction.java:
##########
@@ -68,24 +68,28 @@ public LiteralTransformFunction(LiteralContext literalContext) {
_doubleLiteral = _bigDecimalLiteral.doubleValue();
}
- public BigDecimal getBigDecimalLiteral() {
- return _bigDecimalLiteral;
+ public boolean getBooleanLiteral() {
+ return BooleanUtils.toBoolean(_literal);
}
public int getIntLiteral() {
return _intLiteral;
}
+ public long getLongLiteral() {
+ return _longLiteral;
+ }
+
Review Comment:
ditto.
--
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 #10562: [feature] [null support # 5] Support null in scalar transform function wrapper
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10562:
URL: https://github.com/apache/pinot/pull/10562#discussion_r1164765767
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/LiteralTransformFunction.java:
##########
@@ -68,24 +68,28 @@ public LiteralTransformFunction(LiteralContext literalContext) {
_doubleLiteral = _bigDecimalLiteral.doubleValue();
}
- public BigDecimal getBigDecimalLiteral() {
- return _bigDecimalLiteral;
+ public boolean getBooleanLiteral() {
+ return BooleanUtils.toBoolean(_literal);
}
public int getIntLiteral() {
return _intLiteral;
}
+ public long getLongLiteral() {
+ return _longLiteral;
+ }
+
Review Comment:
How about float?
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/ScalarTransformFunctionWrapper.java:
##########
@@ -85,14 +89,45 @@ public void init(List<TransformFunction> arguments, Map<String, ColumnContext> c
"Wrong number of arguments for method: %s, expected: %s, actual: %s", _functionInvoker.getMethod(),
parameterTypes.length, numArguments);
- _arguments = new Object[numArguments];
+ _scalarArguments = new Object[numArguments];
_nonLiteralIndices = new int[numArguments];
_nonLiteralFunctions = new TransformFunction[numArguments];
for (int i = 0; i < numArguments; i++) {
TransformFunction transformFunction = arguments.get(i);
if (transformFunction instanceof LiteralTransformFunction) {
- String literal = ((LiteralTransformFunction) transformFunction).getStringLiteral();
- _arguments[i] = parameterTypes[i].convert(literal, PinotDataType.STRING);
+ LiteralTransformFunction literalTransformFunction = (LiteralTransformFunction) transformFunction;
+ DataType dataType = literalTransformFunction.getResultMetadata().getDataType();
Review Comment:
Not introduced in this PR, but can literal be `INT` or `FLOAT` type? `NumberUtils.createNumber(literal)` will parse to the string to the smallest type, and we should probably keep this convention
--
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] KKcorps commented on a diff in pull request #10562: [feature] [null support # 5] Support null in scalar transform function wrapper
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on code in PR #10562:
URL: https://github.com/apache/pinot/pull/10562#discussion_r1165928304
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/ScalarTransformFunctionWrapper.java:
##########
@@ -85,14 +89,45 @@ public void init(List<TransformFunction> arguments, Map<String, ColumnContext> c
"Wrong number of arguments for method: %s, expected: %s, actual: %s", _functionInvoker.getMethod(),
parameterTypes.length, numArguments);
- _arguments = new Object[numArguments];
+ _scalarArguments = new Object[numArguments];
_nonLiteralIndices = new int[numArguments];
_nonLiteralFunctions = new TransformFunction[numArguments];
for (int i = 0; i < numArguments; i++) {
TransformFunction transformFunction = arguments.get(i);
if (transformFunction instanceof LiteralTransformFunction) {
- String literal = ((LiteralTransformFunction) transformFunction).getStringLiteral();
- _arguments[i] = parameterTypes[i].convert(literal, PinotDataType.STRING);
+ LiteralTransformFunction literalTransformFunction = (LiteralTransformFunction) transformFunction;
+ DataType dataType = literalTransformFunction.getResultMetadata().getDataType();
Review Comment:
Yes +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] codecov-commenter commented on pull request #10562: [feature] [null support # 5] Support null in scalar transform function wrapper
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10562:
URL: https://github.com/apache/pinot/pull/10562#issuecomment-1498463737
## [Codecov](https://codecov.io/gh/apache/pinot/pull/10562?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#10562](https://codecov.io/gh/apache/pinot/pull/10562?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c7da2a4) into [master](https://codecov.io/gh/apache/pinot/commit/114a81ef022722eabae66be6c685f768c6888ece?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (114a81e) will **decrease** coverage by `14.08%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10562 +/- ##
=============================================
- Coverage 27.90% 13.83% -14.08%
- Complexity 58 439 +381
=============================================
Files 2087 2049 -38
Lines 112306 110591 -1715
Branches 16918 16743 -175
=============================================
- Hits 31340 15300 -16040
- Misses 77869 94044 +16175
+ Partials 3097 1247 -1850
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests2 | `13.83% <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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/10562?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...form/function/BinaryOperatorTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/10562?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vQmluYXJ5T3BlcmF0b3JUcmFuc2Zvcm1GdW5jdGlvbi5qYXZh) | `0.00% <0.00%> (-22.79%)` | :arrow_down: |
| [...ator/transform/function/CastTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/10562?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vQ2FzdFRyYW5zZm9ybUZ1bmN0aW9uLmphdmE=) | `0.00% <0.00%> (-18.14%)` | :arrow_down: |
| [.../transform/function/DivisionTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/10562?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vRGl2aXNpb25UcmFuc2Zvcm1GdW5jdGlvbi5qYXZh) | `0.00% <0.00%> (-32.82%)` | :arrow_down: |
| [...orm/function/LogicalOperatorTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/10562?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vTG9naWNhbE9wZXJhdG9yVHJhbnNmb3JtRnVuY3Rpb24uamF2YQ==) | `0.00% <0.00%> (-73.92%)` | :arrow_down: |
| [...or/transform/function/ModuloTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/10562?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vTW9kdWxvVHJhbnNmb3JtRnVuY3Rpb24uamF2YQ==) | `0.00% <0.00%> (-45.17%)` | :arrow_down: |
| [...form/function/MultiplicationTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/10562?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vTXVsdGlwbGljYXRpb25UcmFuc2Zvcm1GdW5jdGlvbi5qYXZh) | `0.00% <0.00%> (-42.86%)` | :arrow_down: |
| [...ansform/function/NotOperatorTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/10562?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vTm90T3BlcmF0b3JUcmFuc2Zvcm1GdW5jdGlvbi5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...tor/transform/function/PowerTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/10562?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vUG93ZXJUcmFuc2Zvcm1GdW5jdGlvbi5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...nsform/function/RoundDecimalTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/10562?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vUm91bmREZWNpbWFsVHJhbnNmb3JtRnVuY3Rpb24uamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...sform/function/ScalarTransformFunctionWrapper.java](https://codecov.io/gh/apache/pinot/pull/10562?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vU2NhbGFyVHJhbnNmb3JtRnVuY3Rpb25XcmFwcGVyLmphdmE=) | `0.00% <0.00%> (-33.01%)` | :arrow_down: |
| ... and [5 more](https://codecov.io/gh/apache/pinot/pull/10562?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
... and [973 files with indirect coverage changes](https://codecov.io/gh/apache/pinot/pull/10562/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
: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=The+Apache+Software+Foundation)
--
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] KKcorps commented on pull request #10562: [feature] [null support # 5] Support null in scalar transform function wrapper
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on PR #10562:
URL: https://github.com/apache/pinot/pull/10562#issuecomment-1504680000
We earlier added `nullableParameters = true` in ScalarFunction annotation. IMO, we should also remove that since all functions can now support null. This is only used internally in the codebase so shouldn't be an issue.
We do need to make sure the methods which uses this flag though work correctly.
--
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] KKcorps commented on pull request #10562: [feature] [null support # 5] Support null in scalar transform function wrapper
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on PR #10562:
URL: https://github.com/apache/pinot/pull/10562#issuecomment-1507945592
https://github.com/apache/pinot/pull/10562#discussion_r1164780161
This will be taken care in another PR
--
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