You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/05/18 19:56:28 UTC
[GitHub] [pinot] PrachiKhobragade opened a new pull request, #8732: Groovy transform function should evaluate the script when arguments are null
PrachiKhobragade opened a new pull request, #8732:
URL: https://github.com/apache/pinot/pull/8732
### Description
When the arguments for groovy transform are null, the evaluate function returns null without running the script. This fix removes that null check in order for the function to evaluate.
Details in isuue #8730
### Testing Done
- Tested the transformation works when reloading a segment and when creating a new segment using generic rows.
- Modified unit test to evaluate the supported groovy function.
--
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] npawar commented on a diff in pull request #8732: Groovy transform function should evaluate the script when arguments are null
Posted by GitBox <gi...@apache.org>.
npawar commented on code in PR #8732:
URL: https://github.com/apache/pinot/pull/8732#discussion_r876514057
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/GroovyFunctionEvaluator.java:
##########
@@ -86,13 +86,13 @@ public List<String> getArguments() {
public Object evaluate(GenericRow genericRow) {
for (String argument : _arguments) {
Object value = genericRow.getValue(argument);
- if (value == null) {
- // FIXME: if any param is null a) exit OR b) assume function handles it ?
- return null;
- }
_binding.setVariable(argument, value);
}
- return _script.run();
+ try {
+ return _script.run();
+ } catch (Exception e) {
Review Comment:
wont errors we were previously catching and throwing during record transformation just get hidden now?
--
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] PrachiKhobragade commented on a diff in pull request #8732: Groovy transform function should evaluate the script when arguments are null
Posted by GitBox <gi...@apache.org>.
PrachiKhobragade commented on code in PR #8732:
URL: https://github.com/apache/pinot/pull/8732#discussion_r876477739
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/GroovyFunctionEvaluator.java:
##########
@@ -86,10 +86,6 @@ public List<String> getArguments() {
public Object evaluate(GenericRow genericRow) {
for (String argument : _arguments) {
Object value = genericRow.getValue(argument);
- if (value == null) {
Review Comment:
Added the check
--
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] PrachiKhobragade commented on a diff in pull request #8732: Groovy transform function should evaluate the script when arguments are null
Posted by GitBox <gi...@apache.org>.
PrachiKhobragade commented on code in PR #8732:
URL: https://github.com/apache/pinot/pull/8732#discussion_r876337073
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/GroovyFunctionEvaluator.java:
##########
@@ -86,10 +86,6 @@ public List<String> getArguments() {
public Object evaluate(GenericRow genericRow) {
for (String argument : _arguments) {
Object value = genericRow.getValue(argument);
- if (value == null) {
Review Comment:
after the groovy transform is run, the null value transform will be run, and if any value still remains null, NullValueTransformer will set the default value based on datatype. We run all these transformations in sequence.
```
return new CompositeTransformer(Arrays
.asList(new ExpressionTransformer(tableConfig, schema), new FilterTransformer(tableConfig),
new DataTypeTransformer(schema), new NullValueTransformer(tableConfig, schema),
new SanitizationTransformer(schema)));
```
--
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] navina commented on a diff in pull request #8732: Groovy transform function should evaluate the script when arguments are null
Posted by GitBox <gi...@apache.org>.
navina commented on code in PR #8732:
URL: https://github.com/apache/pinot/pull/8732#discussion_r876321094
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/GroovyFunctionEvaluator.java:
##########
@@ -86,10 +86,6 @@ public List<String> getArguments() {
public Object evaluate(GenericRow genericRow) {
for (String argument : _arguments) {
Object value = genericRow.getValue(argument);
- if (value == null) {
Review Comment:
What is the behavior if the provided function doesn't handle `null` values? it might throw exception and is that handled?
--
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] PrachiKhobragade commented on a diff in pull request #8732: Groovy transform function should evaluate the script when arguments are null
Posted by GitBox <gi...@apache.org>.
PrachiKhobragade commented on code in PR #8732:
URL: https://github.com/apache/pinot/pull/8732#discussion_r876492343
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/GroovyFunctionEvaluator.java:
##########
@@ -86,13 +86,15 @@ public List<String> getArguments() {
public Object evaluate(GenericRow genericRow) {
for (String argument : _arguments) {
Object value = genericRow.getValue(argument);
- if (value == null) {
- // FIXME: if any param is null a) exit OR b) assume function handles it ?
- return null;
- }
_binding.setVariable(argument, value);
}
- return _script.run();
+ Object evaluated = null;
Review Comment:
modified
--
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] npawar commented on a diff in pull request #8732: Groovy transform function should evaluate the script when arguments are null
Posted by GitBox <gi...@apache.org>.
npawar commented on code in PR #8732:
URL: https://github.com/apache/pinot/pull/8732#discussion_r876515186
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/GroovyFunctionEvaluator.java:
##########
@@ -86,13 +86,13 @@ public List<String> getArguments() {
public Object evaluate(GenericRow genericRow) {
for (String argument : _arguments) {
Object value = genericRow.getValue(argument);
- if (value == null) {
- // FIXME: if any param is null a) exit OR b) assume function handles it ?
- return null;
- }
_binding.setVariable(argument, value);
}
- return _script.run();
+ try {
+ return _script.run();
Review Comment:
wont errors we were previously catching and throwing during record transformation just get hidden now?
--
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 #8732: Groovy transform function should evaluate the script when arguments are null
Posted by GitBox <gi...@apache.org>.
snleee commented on code in PR #8732:
URL: https://github.com/apache/pinot/pull/8732#discussion_r876489231
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/GroovyFunctionEvaluator.java:
##########
@@ -86,13 +86,15 @@ public List<String> getArguments() {
public Object evaluate(GenericRow genericRow) {
for (String argument : _arguments) {
Object value = genericRow.getValue(argument);
- if (value == null) {
- // FIXME: if any param is null a) exit OR b) assume function handles it ?
- return null;
- }
_binding.setVariable(argument, value);
}
- return _script.run();
+ Object evaluated = null;
Review Comment:
```
try {
return _script.run();
} catch (Exception e) {
return null;
}
```
the above is probably slightly better?
--
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 #8732: Groovy transform function should evaluate the script when arguments are null
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #8732:
URL: https://github.com/apache/pinot/pull/8732#discussion_r876448664
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/GroovyFunctionEvaluator.java:
##########
@@ -86,10 +86,6 @@ public List<String> getArguments() {
public Object evaluate(GenericRow genericRow) {
for (String argument : _arguments) {
Object value = genericRow.getValue(argument);
- if (value == null) {
Review Comment:
I think the suggestion here is to do a try-catch around `_script.run()` and return `null` if it throws exception
--
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 #8732: Groovy transform function should evaluate the script when arguments are null
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged PR #8732:
URL: https://github.com/apache/pinot/pull/8732
--
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] PrachiKhobragade commented on a diff in pull request #8732: Groovy transform function should evaluate the script when arguments are null
Posted by GitBox <gi...@apache.org>.
PrachiKhobragade commented on code in PR #8732:
URL: https://github.com/apache/pinot/pull/8732#discussion_r877539522
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/GroovyFunctionEvaluator.java:
##########
@@ -86,13 +86,13 @@ public List<String> getArguments() {
public Object evaluate(GenericRow genericRow) {
for (String argument : _arguments) {
Object value = genericRow.getValue(argument);
- if (value == null) {
- // FIXME: if any param is null a) exit OR b) assume function handles it ?
- return null;
- }
_binding.setVariable(argument, value);
}
- return _script.run();
+ try {
+ return _script.run();
Review Comment:
thanks, modifying!
--
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] sajjad-moradi commented on a diff in pull request #8732: Groovy transform function should evaluate the script when arguments are null
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on code in PR #8732:
URL: https://github.com/apache/pinot/pull/8732#discussion_r877524439
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/GroovyFunctionEvaluator.java:
##########
@@ -86,13 +86,13 @@ public List<String> getArguments() {
public Object evaluate(GenericRow genericRow) {
for (String argument : _arguments) {
Object value = genericRow.getValue(argument);
- if (value == null) {
- // FIXME: if any param is null a) exit OR b) assume function handles it ?
- return null;
- }
_binding.setVariable(argument, value);
}
- return _script.run();
+ try {
+ return _script.run();
Review Comment:
+1 on this approach. I think it's useful to have the ability to throw exception when transformation is problematic.
--
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 #8732: Groovy transform function should evaluate the script when arguments are null
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #8732:
URL: https://github.com/apache/pinot/pull/8732#discussion_r877331922
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/GroovyFunctionEvaluator.java:
##########
@@ -86,13 +86,13 @@ public List<String> getArguments() {
public Object evaluate(GenericRow genericRow) {
for (String argument : _arguments) {
Object value = genericRow.getValue(argument);
- if (value == null) {
- // FIXME: if any param is null a) exit OR b) assume function handles it ?
- return null;
- }
_binding.setVariable(argument, value);
}
- return _script.run();
+ try {
+ return _script.run();
Review Comment:
@npawar Good call. Yes, it is essentially a decision between fail fast or keep the ingestion going but fill the default value.
Since we are throwing the exception right now, we might want to keep the same behavior. One way to get both worlds is to add a flag to track if any argument is `null`, and return `null` if the flag is `true`, throw exception if the flag is `false`
--
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 #8732: Groovy transform function should evaluate the script when arguments are null
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #8732:
URL: https://github.com/apache/pinot/pull/8732#issuecomment-1130547471
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8732?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 [#8732](https://codecov.io/gh/apache/pinot/pull/8732?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6fefe68) into [master](https://codecov.io/gh/apache/pinot/commit/1b3819e7879947f1337045df81f2ae411778e774?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1b3819e) will **decrease** coverage by `55.41%`.
> The diff coverage is `0.00%`.
> :exclamation: Current head 6fefe68 differs from pull request most recent head 12d13c8. Consider uploading reports for the commit 12d13c8 to get more accurate results
```diff
@@ Coverage Diff @@
## master #8732 +/- ##
=============================================
- Coverage 69.66% 14.24% -55.42%
+ Complexity 4616 168 -4448
=============================================
Files 1730 1684 -46
Lines 90341 88332 -2009
Branches 13442 13202 -240
=============================================
- Hits 62932 12579 -50353
- Misses 23045 74819 +51774
+ Partials 4364 934 -3430
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `14.24% <0.00%> (-0.04%)` | :arrow_down: |
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/8732?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...operator/filter/RangeIndexBasedFilterOperator.java](https://codecov.io/gh/apache/pinot/pull/8732/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvUmFuZ2VJbmRleEJhc2VkRmlsdGVyT3BlcmF0b3IuamF2YQ==) | `0.00% <0.00%> (-70.94%)` | :arrow_down: |
| [...lter/predicate/RangePredicateEvaluatorFactory.java](https://codecov.io/gh/apache/pinot/pull/8732/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvcHJlZGljYXRlL1JhbmdlUHJlZGljYXRlRXZhbHVhdG9yRmFjdG9yeS5qYXZh) | `0.00% <0.00%> (-90.05%)` | :arrow_down: |
| [...egment/local/function/GroovyFunctionEvaluator.java](https://codecov.io/gh/apache/pinot/pull/8732/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9mdW5jdGlvbi9Hcm9vdnlGdW5jdGlvbkV2YWx1YXRvci5qYXZh) | `0.00% <ø> (-100.00%)` | :arrow_down: |
| [...src/main/java/org/apache/pinot/sql/FilterKind.java](https://codecov.io/gh/apache/pinot/pull/8732/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvRmlsdGVyS2luZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ain/java/org/apache/pinot/core/data/table/Key.java](https://codecov.io/gh/apache/pinot/pull/8732/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL0tleS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...in/java/org/apache/pinot/spi/utils/StringUtil.java](https://codecov.io/gh/apache/pinot/pull/8732/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvU3RyaW5nVXRpbC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/spi/utils/BooleanUtils.java](https://codecov.io/gh/apache/pinot/pull/8732/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQm9vbGVhblV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/core/data/table/Record.java](https://codecov.io/gh/apache/pinot/pull/8732/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1JlY29yZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/core/util/GroupByUtils.java](https://codecov.io/gh/apache/pinot/pull/8732/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91dGlsL0dyb3VwQnlVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/BaseRecording.java](https://codecov.io/gh/apache/pinot/pull/8732/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvQmFzZVJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1354 more](https://codecov.io/gh/apache/pinot/pull/8732/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8732?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8732?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [1b3819e...12d13c8](https://codecov.io/gh/apache/pinot/pull/8732?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] snleee commented on a diff in pull request #8732: Groovy transform function should evaluate the script when arguments are null
Posted by GitBox <gi...@apache.org>.
snleee commented on code in PR #8732:
URL: https://github.com/apache/pinot/pull/8732#discussion_r876332531
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/GroovyFunctionEvaluator.java:
##########
@@ -86,10 +86,6 @@ public List<String> getArguments() {
public Object evaluate(GenericRow genericRow) {
for (String argument : _arguments) {
Object value = genericRow.getValue(argument);
- if (value == null) {
Review Comment:
+1. We need to double check on what `_script.run()` would return for NullPointerException in the groovy side and we should handle it properly (e.g. on exception, we put `null` value. Or, we can even throw the exception and fail the segment creation.)
--
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