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/02/08 22:23:19 UTC
[GitHub] [pinot] timsants opened a new pull request #8169: Adding controller config for disabling Groovy in ingestionConfig
timsants opened a new pull request #8169:
URL: https://github.com/apache/pinot/pull/8169
## Description
This change adds a broker config for disabling Groovy transform and filter functions in the table ingestion config as it is a security risk. See Github issue https://github.com/apache/pinot/issues/7966. By default, Groovy is allowed for backwards compatibility to not break existing use cases which currently use Groovy.
### Testing
Added unit tests and tested config with quick-start config override.
## Upgrade Notes
Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
* [ ] Yes (Please label as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
Does this PR fix a zero-downtime upgrade introduced earlier?
* [ ] Yes (Please label this as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
Does this PR otherwise need attention when creating release notes? Things to consider:
* [x] Yes (Please label this PR as **<code>release-notes</code>** and complete the section on Release Notes)
## Release Notes
<!-- If you have tagged this as either backward-incompat or release-notes,
you MUST add text here that you would like to see appear in release notes of the
next release. -->
Introduced new config for disabling Groovy in ingestionConfig: `controller.disable.groovy`. If not defined, defaults to `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 #8169: Adding controller config for disabling Groovy in ingestionConfig
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #8169:
URL: https://github.com/apache/pinot/pull/8169#issuecomment-1034430517
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8169?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 [#8169](https://codecov.io/gh/apache/pinot/pull/8169?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (01ba3ca) into [master](https://codecov.io/gh/apache/pinot/commit/e71c1b16b6059d70ce37813a8058a2c6d1e9d7b0?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e71c1b1) will **decrease** coverage by `42.51%`.
> The diff coverage is `26.15%`.
[](https://codecov.io/gh/apache/pinot/pull/8169?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #8169 +/- ##
=============================================
- Coverage 71.38% 28.86% -42.52%
=============================================
Files 1624 1612 -12
Lines 84142 83970 -172
Branches 12597 12604 +7
=============================================
- Hits 60061 24239 -35822
- Misses 19964 57522 +37558
+ Partials 4117 2209 -1908
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `28.86% <26.15%> (-0.03%)` | :arrow_down: |
| integration2 | `?` | |
| unittests1 | `?` | |
| 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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/8169?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ler/api/resources/TableConfigsRestletResource.java](https://codecov.io/gh/apache/pinot/pull/8169/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1RhYmxlQ29uZmlnc1Jlc3RsZXRSZXNvdXJjZS5qYXZh) | `0.00% <0.00%> (-73.13%)` | :arrow_down: |
| [...gment/local/function/FunctionEvaluatorFactory.java](https://codecov.io/gh/apache/pinot/pull/8169/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9mdW5jdGlvbi9GdW5jdGlvbkV2YWx1YXRvckZhY3RvcnkuamF2YQ==) | `0.00% <0.00%> (-96.88%)` | :arrow_down: |
| [...indexsegment/immutable/ImmutableSegmentLoader.java](https://codecov.io/gh/apache/pinot/pull/8169/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9pbmRleHNlZ21lbnQvaW1tdXRhYmxlL0ltbXV0YWJsZVNlZ21lbnRMb2FkZXIuamF2YQ==) | `0.00% <0.00%> (-84.16%)` | :arrow_down: |
| [...ndex/converter/SegmentV1V2ToV3FormatConverter.java](https://codecov.io/gh/apache/pinot/pull/8169/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2NvbnZlcnRlci9TZWdtZW50VjFWMlRvVjNGb3JtYXRDb252ZXJ0ZXIuamF2YQ==) | `0.00% <0.00%> (-77.78%)` | :arrow_down: |
| [...he/pinot/segment/local/utils/TableConfigUtils.java](https://codecov.io/gh/apache/pinot/pull/8169/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91dGlscy9UYWJsZUNvbmZpZ1V0aWxzLmphdmE=) | `0.00% <0.00%> (-65.84%)` | :arrow_down: |
| [...ment/spi/loader/SegmentDirectoryLoaderContext.java](https://codecov.io/gh/apache/pinot/pull/8169/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-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2xvYWRlci9TZWdtZW50RGlyZWN0b3J5TG9hZGVyQ29udGV4dC5qYXZh) | `0.00% <0.00%> (-70.00%)` | :arrow_down: |
| [...oller/api/resources/PinotTableRestletResource.java](https://codecov.io/gh/apache/pinot/pull/8169/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90VGFibGVSZXN0bGV0UmVzb3VyY2UuamF2YQ==) | `23.18% <66.66%> (-26.99%)` | :arrow_down: |
| [.../pinot/core/data/manager/BaseTableDataManager.java](https://codecov.io/gh/apache/pinot/pull/8169/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvQmFzZVRhYmxlRGF0YU1hbmFnZXIuamF2YQ==) | `69.78% <84.61%> (-16.80%)` | :arrow_down: |
| [...va/org/apache/pinot/controller/ControllerConf.java](https://codecov.io/gh/apache/pinot/pull/8169/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9Db250cm9sbGVyQ29uZi5qYXZh) | `50.20% <100.00%> (-8.34%)` | :arrow_down: |
| [.../pinot/controller/api/upload/SegmentValidator.java](https://codecov.io/gh/apache/pinot/pull/8169/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvdXBsb2FkL1NlZ21lbnRWYWxpZGF0b3IuamF2YQ==) | `62.50% <100.00%> (ø)` | |
| ... and [1187 more](https://codecov.io/gh/apache/pinot/pull/8169/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/8169?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/8169?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 [3972104...01ba3ca](https://codecov.io/gh/apache/pinot/pull/8169?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] timsants commented on a change in pull request #8169: Adding controller config for disabling Groovy in ingestionConfig
Posted by GitBox <gi...@apache.org>.
timsants commented on a change in pull request #8169:
URL: https://github.com/apache/pinot/pull/8169#discussion_r802181642
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/FunctionEvaluatorFactory.java
##########
@@ -96,6 +96,14 @@ public static FunctionEvaluator getExpressionEvaluator(FieldSpec fieldSpec) {
return functionEvaluator;
}
+ public static FunctionEvaluator getExpressionEvaluator(String transformExpression, boolean disableGroovy) {
Review comment:
So are you saying that we first call `isGroovyExpression(String transformExpression, boolean disableGroovy)` and pass the return value to the getter? i.e.
`public static FunctionEvaluator getExpressionEvaluator(String transformExpression, boolean isGroovyExpression)`.
--
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 change in pull request #8169: Adding controller config for disabling Groovy in ingestionConfig
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #8169:
URL: https://github.com/apache/pinot/pull/8169#discussion_r802125026
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -841,6 +842,13 @@ public String getControllerResourcePackages() {
return getProperty(CONTROLLER_RESOURCE_PACKAGES, DEFAULT_CONTROLLER_RESOURCE_PACKAGES);
}
+ /**
+ * @return true if Groovy functions are disabled in controller config, otherwise returns false.
+ */
+ public boolean getDisableGroovy() {
Review comment:
```suggestion
public boolean isDisableIngestionGroovy() {
```
--
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] timsants commented on a change in pull request #8169: Adding controller config for disabling Groovy in ingestionConfig
Posted by GitBox <gi...@apache.org>.
timsants commented on a change in pull request #8169:
URL: https://github.com/apache/pinot/pull/8169#discussion_r803230351
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/FunctionEvaluatorFactory.java
##########
@@ -96,6 +96,14 @@ public static FunctionEvaluator getExpressionEvaluator(FieldSpec fieldSpec) {
return functionEvaluator;
}
+ public static FunctionEvaluator getExpressionEvaluator(String transformExpression, boolean disableGroovy) {
Review comment:
Updated. Let me know if this was what you were thinking.
--
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 edited a comment on pull request #8169: Adding controller config for disabling Groovy in ingestionConfig
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #8169:
URL: https://github.com/apache/pinot/pull/8169#issuecomment-1034430517
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8169?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 [#8169](https://codecov.io/gh/apache/pinot/pull/8169?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (01ba3ca) into [master](https://codecov.io/gh/apache/pinot/commit/e71c1b16b6059d70ce37813a8058a2c6d1e9d7b0?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e71c1b1) will **decrease** coverage by `40.75%`.
> The diff coverage is `26.15%`.
[](https://codecov.io/gh/apache/pinot/pull/8169?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #8169 +/- ##
=============================================
- Coverage 71.38% 30.62% -40.76%
=============================================
Files 1624 1612 -12
Lines 84142 83970 -172
Branches 12597 12604 +7
=============================================
- Hits 60061 25713 -34348
- Misses 19964 55967 +36003
+ Partials 4117 2290 -1827
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `28.86% <26.15%> (-0.03%)` | :arrow_down: |
| integration2 | `27.61% <23.07%> (+0.02%)` | :arrow_up: |
| unittests1 | `?` | |
| 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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/8169?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ler/api/resources/TableConfigsRestletResource.java](https://codecov.io/gh/apache/pinot/pull/8169/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1RhYmxlQ29uZmlnc1Jlc3RsZXRSZXNvdXJjZS5qYXZh) | `0.00% <0.00%> (-73.13%)` | :arrow_down: |
| [...gment/local/function/FunctionEvaluatorFactory.java](https://codecov.io/gh/apache/pinot/pull/8169/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9mdW5jdGlvbi9GdW5jdGlvbkV2YWx1YXRvckZhY3RvcnkuamF2YQ==) | `0.00% <0.00%> (-96.88%)` | :arrow_down: |
| [...indexsegment/immutable/ImmutableSegmentLoader.java](https://codecov.io/gh/apache/pinot/pull/8169/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9pbmRleHNlZ21lbnQvaW1tdXRhYmxlL0ltbXV0YWJsZVNlZ21lbnRMb2FkZXIuamF2YQ==) | `0.00% <0.00%> (-84.16%)` | :arrow_down: |
| [...ndex/converter/SegmentV1V2ToV3FormatConverter.java](https://codecov.io/gh/apache/pinot/pull/8169/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2NvbnZlcnRlci9TZWdtZW50VjFWMlRvVjNGb3JtYXRDb252ZXJ0ZXIuamF2YQ==) | `0.00% <0.00%> (-77.78%)` | :arrow_down: |
| [...he/pinot/segment/local/utils/TableConfigUtils.java](https://codecov.io/gh/apache/pinot/pull/8169/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91dGlscy9UYWJsZUNvbmZpZ1V0aWxzLmphdmE=) | `0.00% <0.00%> (-65.84%)` | :arrow_down: |
| [...ment/spi/loader/SegmentDirectoryLoaderContext.java](https://codecov.io/gh/apache/pinot/pull/8169/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-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2xvYWRlci9TZWdtZW50RGlyZWN0b3J5TG9hZGVyQ29udGV4dC5qYXZh) | `0.00% <0.00%> (-70.00%)` | :arrow_down: |
| [...oller/api/resources/PinotTableRestletResource.java](https://codecov.io/gh/apache/pinot/pull/8169/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90VGFibGVSZXN0bGV0UmVzb3VyY2UuamF2YQ==) | `26.98% <66.66%> (-23.19%)` | :arrow_down: |
| [.../pinot/core/data/manager/BaseTableDataManager.java](https://codecov.io/gh/apache/pinot/pull/8169/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvQmFzZVRhYmxlRGF0YU1hbmFnZXIuamF2YQ==) | `70.21% <84.61%> (-16.37%)` | :arrow_down: |
| [...va/org/apache/pinot/controller/ControllerConf.java](https://codecov.io/gh/apache/pinot/pull/8169/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9Db250cm9sbGVyQ29uZi5qYXZh) | `51.82% <100.00%> (-6.72%)` | :arrow_down: |
| [.../pinot/controller/api/upload/SegmentValidator.java](https://codecov.io/gh/apache/pinot/pull/8169/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvdXBsb2FkL1NlZ21lbnRWYWxpZGF0b3IuamF2YQ==) | `62.50% <100.00%> (ø)` | |
| ... and [1131 more](https://codecov.io/gh/apache/pinot/pull/8169/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/8169?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/8169?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 [3972104...01ba3ca](https://codecov.io/gh/apache/pinot/pull/8169?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] Jackie-Jiang commented on a change in pull request #8169: Adding controller config for disabling Groovy in ingestionConfig
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #8169:
URL: https://github.com/apache/pinot/pull/8169#discussion_r803159352
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/FunctionEvaluatorFactory.java
##########
@@ -96,6 +96,14 @@ public static FunctionEvaluator getExpressionEvaluator(FieldSpec fieldSpec) {
return functionEvaluator;
}
+ public static FunctionEvaluator getExpressionEvaluator(String transformExpression, boolean disableGroovy) {
Review comment:
Basically adding a method `public static boolean isGroovyExpression(String transformExpression)`. The table config validator can call this method, and throw exception if it returns `true`
--
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 #8169: Adding controller config for disabling Groovy in ingestionConfig
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged pull request #8169:
URL: https://github.com/apache/pinot/pull/8169
--
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] timsants commented on a change in pull request #8169: Adding controller config for disabling Groovy in ingestionConfig
Posted by GitBox <gi...@apache.org>.
timsants commented on a change in pull request #8169:
URL: https://github.com/apache/pinot/pull/8169#discussion_r802175559
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -251,6 +251,7 @@ private static long getRandomInitialDelayInSeconds() {
private static final String DEFAULT_DIM_TABLE_MAX_SIZE = "200M";
private static final String DEFAULT_PINOT_FS_FACTORY_CLASS_LOCAL = LocalPinotFS.class.getName();
+ public static final String DISABLE_GROOVY = "controller.disable.groovy";
Review comment:
Shall we then change the broker property to `pinot.broker.disable.query.groovy`?
--
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 change in pull request #8169: Adding controller config for disabling Groovy in ingestionConfig
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #8169:
URL: https://github.com/apache/pinot/pull/8169#discussion_r803157408
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -251,6 +251,7 @@ private static long getRandomInitialDelayInSeconds() {
private static final String DEFAULT_DIM_TABLE_MAX_SIZE = "200M";
private static final String DEFAULT_PINOT_FS_FACTORY_CLASS_LOCAL = LocalPinotFS.class.getName();
+ public static final String DISABLE_GROOVY = "controller.disable.groovy";
Review comment:
Sure, we can make it explicit
--
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 change in pull request #8169: Adding controller config for disabling Groovy in ingestionConfig
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #8169:
URL: https://github.com/apache/pinot/pull/8169#discussion_r802124827
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -251,6 +251,7 @@ private static long getRandomInitialDelayInSeconds() {
private static final String DEFAULT_DIM_TABLE_MAX_SIZE = "200M";
private static final String DEFAULT_PINOT_FS_FACTORY_CLASS_LOCAL = LocalPinotFS.class.getName();
+ public static final String DISABLE_GROOVY = "controller.disable.groovy";
Review comment:
Let's make it explicit that this disables groovy for ingestion, e.g. `controller.disable.ingestion.groovy`
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -251,6 +251,7 @@ private static long getRandomInitialDelayInSeconds() {
private static final String DEFAULT_DIM_TABLE_MAX_SIZE = "200M";
private static final String DEFAULT_PINOT_FS_FACTORY_CLASS_LOCAL = LocalPinotFS.class.getName();
+ public static final String DISABLE_GROOVY = "controller.disable.groovy";
Review comment:
This is a legacy issue. Let's keep it the current way, and we can fix it all together in the future.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -841,6 +842,13 @@ public String getControllerResourcePackages() {
return getProperty(CONTROLLER_RESOURCE_PACKAGES, DEFAULT_CONTROLLER_RESOURCE_PACKAGES);
}
+ /**
+ * @return true if Groovy functions are disabled in controller config, otherwise returns false.
+ */
+ public boolean getDisableGroovy() {
Review comment:
```suggestion
public boolean isDisableGroovy() {
```
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/FunctionEvaluatorFactory.java
##########
@@ -96,6 +96,14 @@ public static FunctionEvaluator getExpressionEvaluator(FieldSpec fieldSpec) {
return functionEvaluator;
}
+ public static FunctionEvaluator getExpressionEvaluator(String transformExpression, boolean disableGroovy) {
Review comment:
Let's add a method `isGroovyExpression()` instead of passing `disableGroovy` to the getter
--
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] timsants commented on a change in pull request #8169: Adding controller config for disabling Groovy in ingestionConfig
Posted by GitBox <gi...@apache.org>.
timsants commented on a change in pull request #8169:
URL: https://github.com/apache/pinot/pull/8169#discussion_r802110350
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -251,6 +251,7 @@ private static long getRandomInitialDelayInSeconds() {
private static final String DEFAULT_DIM_TABLE_MAX_SIZE = "200M";
private static final String DEFAULT_PINOT_FS_FACTORY_CLASS_LOCAL = LocalPinotFS.class.getName();
+ public static final String DISABLE_GROOVY = "controller.disable.groovy";
Review comment:
I noticed that the configs defined in this file don't start with the `pinot.` prefix like how the properties are defined in CommonConstants. Should we make this consistent with the broker config i.e. `pinot.broker.disable.groovy`?
--
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 edited a comment on pull request #8169: Adding controller config for disabling Groovy in ingestionConfig
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #8169:
URL: https://github.com/apache/pinot/pull/8169#issuecomment-1034430517
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8169?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 [#8169](https://codecov.io/gh/apache/pinot/pull/8169?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (38e0d00) into [master](https://codecov.io/gh/apache/pinot/commit/6844cb32e2ed10b3e8640bac8ef10af09d0a6b76?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6844cb3) will **increase** coverage by `0.00%`.
> The diff coverage is `18.75%`.
[](https://codecov.io/gh/apache/pinot/pull/8169?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #8169 +/- ##
=======================================
Coverage 30.67% 30.68%
=======================================
Files 1612 1612
Lines 83962 83970 +8
Branches 12602 12604 +2
=======================================
+ Hits 25754 25764 +10
+ Misses 55919 55917 -2
Partials 2289 2289
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `28.85% <18.75%> (-0.07%)` | :arrow_down: |
| integration2 | `27.69% <18.75%> (+0.14%)` | :arrow_up: |
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/8169?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ler/api/resources/TableConfigsRestletResource.java](https://codecov.io/gh/apache/pinot/pull/8169/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1RhYmxlQ29uZmlnc1Jlc3RsZXRSZXNvdXJjZS5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...gment/local/function/FunctionEvaluatorFactory.java](https://codecov.io/gh/apache/pinot/pull/8169/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9mdW5jdGlvbi9GdW5jdGlvbkV2YWx1YXRvckZhY3RvcnkuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...he/pinot/segment/local/utils/TableConfigUtils.java](https://codecov.io/gh/apache/pinot/pull/8169/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91dGlscy9UYWJsZUNvbmZpZ1V0aWxzLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...oller/api/resources/PinotTableRestletResource.java](https://codecov.io/gh/apache/pinot/pull/8169/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90VGFibGVSZXN0bGV0UmVzb3VyY2UuamF2YQ==) | `26.98% <66.66%> (ø)` | |
| [...va/org/apache/pinot/controller/ControllerConf.java](https://codecov.io/gh/apache/pinot/pull/8169/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9Db250cm9sbGVyQ29uZi5qYXZh) | `51.82% <100.00%> (-0.22%)` | :arrow_down: |
| [...nction/DistinctCountBitmapAggregationFunction.java](https://codecov.io/gh/apache/pinot/pull/8169/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9mdW5jdGlvbi9EaXN0aW5jdENvdW50Qml0bWFwQWdncmVnYXRpb25GdW5jdGlvbi5qYXZh) | `45.59% <0.00%> (-6.74%)` | :arrow_down: |
| [.../common/request/context/predicate/EqPredicate.java](https://codecov.io/gh/apache/pinot/pull/8169/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vcmVxdWVzdC9jb250ZXh0L3ByZWRpY2F0ZS9FcVByZWRpY2F0ZS5qYXZh) | `53.33% <0.00%> (-6.67%)` | :arrow_down: |
| [...e/pinot/controller/helix/SegmentStatusChecker.java](https://codecov.io/gh/apache/pinot/pull/8169/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9TZWdtZW50U3RhdHVzQ2hlY2tlci5qYXZh) | `66.90% <0.00%> (-4.93%)` | :arrow_down: |
| [...core/query/pruner/SelectionQuerySegmentPruner.java](https://codecov.io/gh/apache/pinot/pull/8169/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9wcnVuZXIvU2VsZWN0aW9uUXVlcnlTZWdtZW50UHJ1bmVyLmphdmE=) | `82.95% <0.00%> (-2.28%)` | :arrow_down: |
| [...ava/org/apache/pinot/core/plan/FilterPlanNode.java](https://codecov.io/gh/apache/pinot/pull/8169/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL0ZpbHRlclBsYW5Ob2RlLmphdmE=) | `57.84% <0.00%> (-1.97%)` | :arrow_down: |
| ... and [24 more](https://codecov.io/gh/apache/pinot/pull/8169/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/8169?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/8169?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 [6844cb3...38e0d00](https://codecov.io/gh/apache/pinot/pull/8169?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