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

feat: add pinot function registry to translate functions #21

Merged
merged 2 commits into from
Sep 21, 2020

Conversation

aaron-steinfeld
Copy link
Contributor

@aaron-steinfeld aaron-steinfeld commented Sep 18, 2020

Change Motivation: Query service is an abstraction over one or more data stores, but currently the abstract query requires implementation-specific function names. This function converter relaxes that constraint, and converts abstract functions (which, as of this change, are mostly the same as pinot-specific functions, with the exception of percentile and count) into functions compatible with pinot. This will allow us to remove some pinot-bound logic in gateway service (which previously did this conversion - we include logic here to unwind that for backwards compatibility), as well as add an upcoming feature, attribute projection, in such a way that the projection logic is not coupled to a specific data store.

@codecov
Copy link

codecov bot commented Sep 18, 2020

Codecov Report

Merging #21 into main will increase coverage by 1.05%.
The diff coverage is 93.90%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main      #21      +/-   ##
============================================
+ Coverage     73.83%   74.89%   +1.05%     
- Complexity      261      283      +22     
============================================
  Files            27       29       +2     
  Lines          1093     1151      +58     
  Branches        141      140       -1     
============================================
+ Hits            807      862      +55     
- Misses          230      233       +3     
  Partials         56       56              
Flag Coverage Δ Complexity Δ
#unit 74.89% <93.90%> (+1.05%) 283.00 <25.00> (+22.00)

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

Impacted Files Coverage Δ Complexity Δ
...org/hypertrace/core/query/service/ConfigUtils.java 75.00% <75.00%> (ø) 1.00 <1.00> (?)
...rvice/pinot/converters/PinotFunctionConverter.java 92.98% <92.98%> (ø) 21.00 <21.00> (?)
.../query/service/pinot/PinotBasedRequestHandler.java 69.14% <100.00%> (+0.76%) 65.00 <2.00> (+2.00)
...service/pinot/QueryRequestToPinotSQLConverter.java 85.10% <100.00%> (+0.04%) 51.00 <1.00> (-2.00) ⬆️

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 2bafdd4...cc44763. Read the comment docs.

Base automatically changed from add-dependency-injection to main September 20, 2020 18:07
tim-mwangi
tim-mwangi previously approved these changes Sep 21, 2020
import org.hypertrace.core.query.service.api.Value;
import org.hypertrace.core.query.service.api.ValueType;

public class PinotFunctionConverter {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice.. Thanks for cleaning this up!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem. Once this is up and running, we can do some clean up on the gateway side to strip the conversion there since this handles it now.

@aaron-steinfeld aaron-steinfeld merged commit b3513b0 into main Sep 21, 2020
@aaron-steinfeld aaron-steinfeld deleted the pinot-function-registry branch September 21, 2020 19:18
@aaron-steinfeld
Copy link
Contributor Author

Change Motivation: Query service is an abstraction over one or more data stores, but currently the abstract query requires implementation-specific function names. This function converter relaxes that constraint, and converts abstract functions (which, as of this change, are mostly the same as pinot-specific functions, with the exception of percentile and count) into functions compatible with pinot. This will allow us to remove some pinot-bound logic in gateway service (which previously did this conversion - we include logic here to unwind that for backwards compatibility), as well as add an upcoming feature, attribute projection, in such a way that the projection logic is not coupled to a specific data store.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants