Skip to content

Pass context in operators instead of individual objects#10306

Closed
KKcorps wants to merge 1 commit intoapache:masterfrom
KKcorps:query_operator_refactor
Closed

Pass context in operators instead of individual objects#10306
KKcorps wants to merge 1 commit intoapache:masterfrom
KKcorps:query_operator_refactor

Conversation

@KKcorps
Copy link
Contributor

@KKcorps KKcorps commented Feb 20, 2023

This will allow us to easily add more params to operators constructors in the future without major refactoring.

@KKcorps KKcorps requested a review from walterddr February 20, 2023 08:24
@codecov-commenter
Copy link

Codecov Report

Merging #10306 (d25d4e5) into master (0927e94) will decrease coverage by 54.73%.
The diff coverage is 0.00%.

@@              Coverage Diff              @@
##             master   #10306       +/-   ##
=============================================
- Coverage     68.41%   13.69%   -54.73%     
+ Complexity     5109      182     -4927     
=============================================
  Files          2015     1961       -54     
  Lines        109656   107209     -2447     
  Branches      16686    16393      -293     
=============================================
- Hits          75024    14679    -60345     
- Misses        29283    91358    +62075     
+ Partials       5349     1172     -4177     
Flag Coverage Δ
integration1 ?
unittests1 ?
unittests2 13.69% <0.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
...inot/query/runtime/operator/AggregateOperator.java 0.00% <0.00%> (-92.47%) ⬇️
...e/pinot/query/runtime/operator/FilterOperator.java 0.00% <0.00%> (-92.60%) ⬇️
...pinot/query/runtime/operator/HashJoinOperator.java 0.00% <0.00%> (-95.00%) ⬇️
...e/operator/LeafStageTransferableBlockOperator.java 0.00% <0.00%> (-86.54%) ⬇️
...t/query/runtime/operator/LiteralValueOperator.java 0.00% <0.00%> (-100.00%) ⬇️
...query/runtime/operator/MailboxReceiveOperator.java 0.00% <0.00%> (-95.46%) ⬇️
...ot/query/runtime/operator/MailboxSendOperator.java 0.00% <0.00%> (-68.75%) ⬇️
...che/pinot/query/runtime/operator/SortOperator.java 0.00% <0.00%> (-95.35%) ⬇️
...inot/query/runtime/operator/TransformOperator.java 0.00% <0.00%> (-100.00%) ⬇️
.../pinot/query/runtime/plan/PhysicalPlanVisitor.java 0.00% <0.00%> (-92.31%) ⬇️
... and 1555 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@KKcorps
Copy link
Contributor Author

KKcorps commented Feb 20, 2023

After discussion with @walterddr it seems like not the right approach to go with. Reason being the PlanRequestContext object is destroyed after the visitor is called. Any operator if it caches this object, will not be able to use it at runtime. Better to not take this risk in the future.

@gortiz
Copy link
Contributor

gortiz commented Feb 21, 2023

Should we close this PR then?

Copy link
Contributor

@walterddr walterddr left a comment

Choose a reason for hiding this comment

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

yes this one can be closed in favor of #10299 ?

@KKcorps KKcorps closed this Feb 22, 2023
@gortiz
Copy link
Contributor

gortiz commented Feb 22, 2023

But #10299 isn't focused on passing context operators, right? I think we still need to think how to pass this contextual information in Pinot. I've found several places where contextual information is needed and in order to do not add tons of arguments to the constructor I had to delegate on static methods. For example when dealing with #10183 or sending metrics. I have the feeling that we should start thinking on using some dependency injector in order to try to decouple our objects from the contextual arguments

@KKcorps
Copy link
Contributor Author

KKcorps commented Feb 22, 2023

I agree with @gortiz here. Passing more than 3-4 arguments is simply not the correct way and also increases the time to make small changes to functions.

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.

4 participants