Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only discover public methods annotated with @ScalarFunction #8544

Merged
merged 2 commits into from
Apr 14, 2022

Conversation

richardstartin
Copy link
Member

@richardstartin richardstartin commented Apr 14, 2022

Stop discovering private scalar functions to avoid unpredictable illegal reflective access.

This is a breaking change; if a user has a non-public method annotated with @ScalarFunction then it will no longer be evaluated. The migration path is for the user to make the appropriate methods public, since they own the code.

@codecov-commenter
Copy link

Codecov Report

Merging #8544 (4596643) into master (900f01f) will decrease coverage by 41.30%.
The diff coverage is 0.00%.

@@              Coverage Diff              @@
##             master    #8544       +/-   ##
=============================================
- Coverage     70.65%   29.35%   -41.31%     
=============================================
  Files          1674     1669        -5     
  Lines         87743    87602      -141     
  Branches      13279    13282        +3     
=============================================
- Hits          61999    25713    -36286     
- Misses        21439    59508    +38069     
+ Partials       4305     2381     -1924     
Flag Coverage Δ
integration1 27.22% <0.00%> (+0.23%) ⬆️
integration2 25.89% <0.00%> (-0.22%) ⬇️
unittests1 ?
unittests2 ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...org/apache/pinot/common/function/FunctionInfo.java 87.50% <ø> (-12.50%) ⬇️
.../apache/pinot/common/function/FunctionInvoker.java 67.44% <ø> (-12.11%) ⬇️
...apache/pinot/common/function/FunctionRegistry.java 88.88% <0.00%> (-5.23%) ⬇️
.../java/org/apache/pinot/spi/utils/BooleanUtils.java 0.00% <0.00%> (-100.00%) ⬇️
...java/org/apache/pinot/spi/trace/BaseRecording.java 0.00% <0.00%> (-100.00%) ⬇️
...java/org/apache/pinot/spi/trace/NoOpRecording.java 0.00% <0.00%> (-100.00%) ⬇️
...ava/org/apache/pinot/spi/config/table/FSTType.java 0.00% <0.00%> (-100.00%) ⬇️
...ava/org/apache/pinot/spi/data/MetricFieldSpec.java 0.00% <0.00%> (-100.00%) ⬇️
...va/org/apache/pinot/spi/utils/BigDecimalUtils.java 0.00% <0.00%> (-100.00%) ⬇️
...java/org/apache/pinot/common/tier/TierFactory.java 0.00% <0.00%> (-100.00%) ⬇️
... and 1197 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 900f01f...4596643. Read the comment docs.

@richardstartin
Copy link
Member Author

It appears illegal reflective access is relied on even internally

@richardstartin richardstartin marked this pull request as ready for review April 14, 2022 18:52
@richardstartin richardstartin added release-notes Referenced by PRs that need attention when compiling the next release notes jdk17 breaking-change labels Apr 14, 2022
@richardstartin richardstartin changed the title only discover public functions Only discover public methods annotated with @ScalarFunction Apr 14, 2022
@siddharthteotia siddharthteotia merged commit d503d50 into apache:master Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change jdk17 release-notes Referenced by PRs that need attention when compiling the next release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants