Skip to content

Comments

Lookup join without boxing#8208

Closed
richardstartin wants to merge 2 commits intoapache:masterfrom
richardstartin:lookup-without-boxing
Closed

Lookup join without boxing#8208
richardstartin wants to merge 2 commits intoapache:masterfrom
richardstartin:lookup-without-boxing

Conversation

@richardstartin
Copy link
Member

We still see a large number of boxed integers when joining on an INT column, this replaces PrimaryKey with JoinKey which has a specialised implementation when all the keys are fixed width.

Screenshot 2022-02-16 at 14 44 28

@richardstartin richardstartin force-pushed the lookup-without-boxing branch 3 times, most recently from ac77b6b to 70a7d6b Compare February 16, 2022 15:10
@richardstartin richardstartin marked this pull request as draft February 16, 2022 15:14
@richardstartin richardstartin force-pushed the lookup-without-boxing branch 2 times, most recently from 5e3d908 to 8566f21 Compare February 16, 2022 16:00
@codecov-commenter
Copy link

codecov-commenter commented Feb 16, 2022

Codecov Report

Merging #8208 (a2614bc) into master (916d807) will decrease coverage by 6.67%.
The diff coverage is 64.22%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #8208      +/-   ##
============================================
- Coverage     71.02%   64.35%   -6.68%     
- Complexity     4314     4315       +1     
============================================
  Files          1626     1584      -42     
  Lines         84929    83153    -1776     
  Branches      12783    12596     -187     
============================================
- Hits          60325    53517    -6808     
- Misses        20462    25786    +5324     
+ Partials       4142     3850     -292     
Flag Coverage Δ
integration1 ?
integration2 ?
unittests1 67.37% <64.22%> (-0.02%) ⬇️
unittests2 14.12% <0.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
...pache/pinot/core/data/manager/offline/JoinKey.java 20.00% <20.00%> (ø)
...ore/data/manager/offline/VariableWidthJoinKey.java 58.33% <58.33%> (ø)
...t/core/data/manager/offline/FixedWidthJoinKey.java 71.42% <71.42%> (ø)
...or/transform/function/LookupTransformFunction.java 74.64% <72.97%> (-4.41%) ⬇️
...ata/manager/offline/DimensionTableDataManager.java 83.58% <78.94%> (-2.70%) ⬇️
...a/org/apache/pinot/common/metrics/MinionMeter.java 0.00% <0.00%> (-100.00%) ⬇️
...g/apache/pinot/common/metrics/ControllerMeter.java 0.00% <0.00%> (-100.00%) ⬇️
.../apache/pinot/common/metrics/BrokerQueryPhase.java 0.00% <0.00%> (-100.00%) ⬇️
.../apache/pinot/common/metrics/MinionQueryPhase.java 0.00% <0.00%> (-100.00%) ⬇️
...ache/pinot/server/access/AccessControlFactory.java 0.00% <0.00%> (-100.00%) ⬇️
... and 380 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 916d807...a2614bc. Read the comment docs.

@richardstartin richardstartin marked this pull request as ready for review February 16, 2022 22:43
@richardstartin
Copy link
Member Author

I can't establish whether this actually improves performance or not, though it definitely reduces allocation rate. I'm closing this in favour of delaying joins until after group by evaluation. It can be reopened later if necessary.

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.

2 participants