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

Add FunctionManager and Pluggable Functions Implementations #1168

Merged
merged 11 commits into from
Aug 15, 2023

Conversation

yuxtang-amazon
Copy link
Contributor

Relevant Issues

Description

  • Develops a FunctionManager for function overloading and redefines internal Function structures
  • Defines PartiQLFunction and Plugin interfaces, converts PartiQLFunction to ExprFunctions and adds ServiceLoader to
    PartiQL CLI to load pluggable functions
  • Adds --plugin command line operation and completes unit tests & integration tests of Java Service Loader
  • Moves PartiQLFunction parameters and signatures to partiql-types
  • Adds @deprecated notice and give a replacement API to use in place of the deprecated methods

Other Information

  • Updated Unreleased Section in CHANGELOG: [YES/NO]
    YES

  • Any backward-incompatible changes? [YES/NO]
    YES

    • Deprecates Map<String, ExprFunction> representation of functions in the CompilerPipeline
      and experimental PartiQLCompilerPipeline. Please use List to represent functions instead.
    • Deprecates Arguments class, callWithOptional() and callWithVariadic() methods in the ExprFunction
      with a Deprecation Level of ERROR. Please invoke callWithRequired() instead.
    • Deprecates optionalParameter and variadicParameter in the FunctionSignature with a Deprecation
      Level of ERROR. Please use multiple implementations of ExprFunction and use the LIST ExprValue to
      represent variadic parameters instead.
  • Any new external dependencies? [YES/NO]
    NO

  • Do your changes comply with the Contributing Guidelines
    and Code Style Guidelines? [YES/NO]
    YES

License Information

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions
Copy link

github-actions bot commented Jul 31, 2023

Conformance comparison report

Base (090cc67) eb5a209 +/-
% Passing 92.40% 92.33% -0.07%
✅ Passing 5376 5372 -4
❌ Failing 442 446 4
🔶 Ignored 0 0 0
Total Tests 5818 5818 0

Number passing in both: 5372

Number failing in both: 442

Number passing in Base (090cc67) but now fail: 4

Number failing in Base (090cc67) but now pass: 0

⁉️ CONFORMANCE REPORT REGRESSION DETECTED ⁉️. The following test(s) were previously passing but now fail:

Click here to see
  • MOD(NULL, 'some string'), compileOption: PERMISSIVE
  • MOD(NULL, 'some string'), compileOption: LEGACY
  • MOD('some string', NULL), compileOption: PERMISSIVE
  • MOD('some string', NULL), compileOption: LEGACY

Copy link
Member

@johnedquinn johnedquinn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work here! Seems like Vladimir's GPML PR was stripped with this PR, so you'll need to manually revert those files. Still have a couple files to review, but left some comments in the meantime.

@yuxtang-amazon yuxtang-amazon force-pushed the pluggable-functions-rebased branch 3 times, most recently from 5275b24 to 1467673 Compare August 1, 2023 18:07
@sahays sahays added the bug Something isn't working label Aug 8, 2023
@johnedquinn johnedquinn marked this pull request as ready for review August 10, 2023 17:03
@johnedquinn johnedquinn added enhancement New feature or request and removed bug Something isn't working labels Aug 10, 2023
@johnedquinn
Copy link
Member

All looks great to me! Great job! Let's wait for another team member to review/approve before merging.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
docs/wiki/tutorials/Pluggable Functions Tutorial.md Outdated Show resolved Hide resolved
docs/wiki/tutorials/Pluggable Functions Tutorial.md Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Aug 10, 2023

Codecov Report

Patch coverage: 38.28% and project coverage change: -1.22% ⚠️

Comparison is base (c580925) 73.18% compared to head (de3064a) 71.97%.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1168      +/-   ##
============================================
- Coverage     73.18%   71.97%   -1.22%     
+ Complexity     2358     2333      -25     
============================================
  Files           224      228       +4     
  Lines         17398    17699     +301     
  Branches       3202     3243      +41     
============================================
+ Hits          12733    12738       +5     
- Misses         3680     3969     +289     
- Partials        985      992       +7     
Flag Coverage Δ
CLI 13.53% <15.58%> (-0.76%) ⬇️
EXAMPLES 80.28% <ø> (ø)
LANG 79.29% <82.06%> (+0.26%) ⬆️

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

Files Changed Coverage Δ
.../main/kotlin/org/partiql/cli/functions/QueryDDB.kt 88.88% <ø> (-0.40%) ⬇️
...ain/kotlin/org/partiql/cli/pico/PipelineOptions.kt 0.00% <0.00%> (ø)
...otlin/org/partiql/cli/pipeline/AbstractPipeline.kt 35.86% <0.00%> (-1.21%) ⬇️
...rc/main/kotlin/org/partiql/cli/utils/Exceptions.kt 0.00% <0.00%> (ø)
.../kotlin/org/partiql/cli/utils/ServiceLoaderUtil.kt 7.77% <7.77%> (ø)
...iql/lang/eval/physical/PhysicalPlanCompilerImpl.kt 81.46% <35.71%> (-2.41%) ⬇️
...kotlin/org/partiql/lang/types/FunctionSignature.kt 50.00% <40.00%> (-36.67%) ⬇️
.../partiql/lang/planner/transforms/plan/PlanTyper.kt 52.12% <47.82%> (+4.12%) ⬆️
...rg/partiql/cli/utils/EvaluationSessionConnector.kt 50.00% <50.00%> (ø)
...rg/partiql/lang/compiler/PartiQLCompilerDefault.kt 84.41% <50.00%> (-1.12%) ⬇️
... and 13 more

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

RCHowell
RCHowell previously approved these changes Aug 11, 2023
@johnedquinn johnedquinn merged commit eb0b72e into main Aug 15, 2023
10 checks passed
@johnedquinn johnedquinn deleted the pluggable-functions-rebased branch March 7, 2024 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants