Skip to content

Conversation

@dai-chen
Copy link
Collaborator

Description

!!!NOTE!!! Please ignore most file changes as they're all changing instance method to static method call on DSL. The only change matters is in FunctionRepository which becomes a singleton after this.

Problem Statement

The performance issue is improved in #944, this PR is focused on the functionality and our own DSL language adoption problem.

Currently, FunctionRepository is managed by Spring container and inject to class who needs it. DSL depends on the function repository to resolve function signature as well. This causes the function repository or DSL instance passed around and leads to "ugly" code as below:

  private final DSL dsl = new ExpressionConfig().dsl(new ExpressionConfig().functionRepository());

  dsl.equal(
    DSL.ref("integer_value", INTEGER),
    DSL.literal(integerValue(1)))),

Besides, the bigger problem is it blocks high level physical operator from building custom logic on top of the expression system. For example, the new WindowAssigner needs to calculate the lower bound of a window as below. The current coupling with Spring container make it hard to implement:

  // Assign a numeric or datetime window to the value according to its type
  Window assignWindow(ExprValue value) {
    ...
    ExprValue lowerBound = rounding.round(value);
    ExprValue windowSize = ...
    ExprValue upperBound = 
      lowerBound.isNumber()
        ? DSL.add(lowerBound, windowNumberSize)
        : DSL.adddate(lowerBound, windowIntervalSize)
  }

Solution

After double check, make FunctionRepository singleton as well as DSL. This makes both can be reused anywhere.

TODO

  1. May need to move storage engine function register code elsewhere, because it is supposed to be a static list for each storage engine. Added TODO comment for now.

Further Thoughts

  1. Will this impact storage engine register its own function in future? <= Probably not.
  2. Should basic arithmetic operation built-in ExprValue? ex. val1.add(val2) rather than DSL.add(val1, val2).valueOf(null) <= Not sure. Maybe we should.

Issues Resolved

#944

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@codecov-commenter
Copy link

codecov-commenter commented Nov 17, 2022

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.73%. Comparing base (cdb30ee) to head (91d4d5b).
⚠️ Report is 478 commits behind head on 2.x.

❌ Your project status has failed because the head coverage (95.73%) is below the target coverage (99.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files
@@             Coverage Diff              @@
##                2.x    #1085      +/-   ##
============================================
- Coverage     98.28%   95.73%   -2.55%     
+ Complexity     3435     3434       -1     
============================================
  Files           344      353       +9     
  Lines          8560     9198     +638     
  Branches        544      663     +119     
============================================
+ Hits           8413     8806     +393     
- Misses          142      334     +192     
- Partials          5       58      +53     
Flag Coverage Δ
query-workbench 62.76% <ø> (?)
sql-engine 98.27% <100.00%> (-0.01%) ⬇️

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

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: Chen Dai <[email protected]>
@dai-chen dai-chen marked this pull request as ready for review November 17, 2022 18:46
@dai-chen dai-chen requested a review from a team as a code owner November 17, 2022 18:46
acarbonetto
acarbonetto previously approved these changes Nov 17, 2022
Copy link
Collaborator

@acarbonetto acarbonetto left a comment

Choose a reason for hiding this comment

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

LGTM

penghuo
penghuo previously approved these changes Nov 17, 2022
Copy link
Collaborator

@penghuo penghuo left a comment

Choose a reason for hiding this comment

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

Thanks

@dai-chen
Copy link
Collaborator Author

@Yury-Fridlyand Please confirm if the previous issues you mentioned in the old PR are gone. Will try to merge this today to unblock others. Thanks!

Yury-Fridlyand
Yury-Fridlyand previously approved these changes Nov 17, 2022
Signed-off-by: Chen Dai <[email protected]>
Copy link
Collaborator

@derek-ho derek-ho left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@penghuo penghuo left a comment

Choose a reason for hiding this comment

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

Thanks.

@dai-chen dai-chen merged commit 81c9285 into opensearch-project:2.x Nov 17, 2022
@dai-chen dai-chen deleted the refactor-function-repository-and-dsl branch December 16, 2022 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintenance Improves code quality, but not the product

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants