Add memory optimized dimension table#9802
Merged
KKcorps merged 9 commits intoapache:masterfrom Dec 2, 2022
Merged
Conversation
KKcorps
changed the title
Codecov Report
@@ Coverage Diff @@
## master #9802 +/- ##
=============================================
- Coverage 64.11% 15.80% -48.32%
+ Complexity 5385 175 -5210
=============================================
Files 1903 1923 +20
Lines 102554 103482 +928
Branches 15604 15758 +154
=============================================
- Hits 65753 16354 -49399
- Misses 32017 85936 +53919
+ Partials 4784 1192 -3592
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Contributor
Jackie-Jiang
left a comment
Jackie-Jiang
left a comment
There was a problem hiding this comment.
Shall we add a test for it?
pinot-core/src/main/java/org/apache/pinot/core/data/manager/offline/DimensionTable.java
Outdated
Show resolved
Hide resolved
...core/src/main/java/org/apache/pinot/core/data/manager/offline/DimensionTableDataManager.java
Outdated
Show resolved
Hide resolved
pinot-core/src/main/java/org/apache/pinot/core/data/manager/offline/LookupRecordLocation.java
Outdated
Show resolved
Hide resolved
Jackie-Jiang
added
feature
Configuration
.../src/main/java/org/apache/pinot/core/data/manager/offline/MemoryOptimizedDimensionTable.java
Outdated
Show resolved
Hide resolved
...core/src/main/java/org/apache/pinot/core/data/manager/offline/DimensionTableDataManager.java
Outdated
Show resolved
Hide resolved
...core/src/main/java/org/apache/pinot/core/data/manager/offline/DimensionTableDataManager.java
Outdated
Show resolved
Hide resolved
...core/src/main/java/org/apache/pinot/core/data/manager/offline/DimensionTableDataManager.java
Outdated
Show resolved
Hide resolved
...core/src/main/java/org/apache/pinot/core/data/manager/offline/DimensionTableDataManager.java
Outdated
Show resolved
Hide resolved
...core/src/main/java/org/apache/pinot/core/data/manager/offline/DimensionTableDataManager.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/apache/pinot/core/data/manager/offline/MemoryOptimizedDimensionTable.java
Outdated
Show resolved
Hide resolved
KKcorps
force-pushed
the
dim_table_memlookup_patch
branch
from
af6d8df to
d157124
Compare
...core/src/main/java/org/apache/pinot/core/data/manager/offline/DimensionTableDataManager.java
Outdated
Show resolved
Hide resolved
...core/src/main/java/org/apache/pinot/core/data/manager/offline/DimensionTableDataManager.java
Outdated
Show resolved
Hide resolved
Jackie-Jiang
approved these changes
Dec 1, 2022
Contributor
Jackie-Jiang
left a comment
Jackie-Jiang
left a comment
There was a problem hiding this comment.
LGTM. Please reformat the changes
.../src/main/java/org/apache/pinot/core/data/manager/offline/MemoryOptimizedDimensionTable.java
Outdated
Show resolved
Hide resolved
...core/src/main/java/org/apache/pinot/core/data/manager/offline/DimensionTableDataManager.java
Outdated
Show resolved
Hide resolved
...core/src/main/java/org/apache/pinot/core/data/manager/offline/DimensionTableDataManager.java
Outdated
Show resolved
Hide resolved
saurabhd336
reviewed
Mar 29, 2023
| if (lookupRecordLocation == null) { | ||
| return null; | ||
| } | ||
| return lookupRecordLocation.getRecord(_reuse); |
Contributor
There was a problem hiding this comment.
Is this thread safe? _reuse being shared across .get calls?
Contributor
Author
There was a problem hiding this comment.
I think we need to clear the _reuse before passing it to .getRecord.
That's what we do in other places in the code.
Contributor
There was a problem hiding this comment.
get can be called concurrently. Think this should be ThreadLocal<GenericRow> _reuse instead
Contributor
There was a problem hiding this comment.
Good catch! I think this is a bug
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently, we store the whole row in the Dimension table hash map for fast lookups.
However, in some cases, we can trade off the speed for memory efficiency.
In this PR, I am adding a new implementation that only stores the segment reference and docID, and the actual value is read at the time of lookup.
I have also created another design where I am using different DataManager implementations instead of DataTable implementations. It touches a lot of code paths though hence I didn't raise PR for that approach. The code can be seen here
https://github.com/KKcorps/incubator-pinot/pull/23/files
Release Notes
Disable preloading while using Dimension tables to save memory
Documentation
add the following config in tableConfig json to disable preloading
{ "dimensionTableConfig": { "disablePreload": true } }