-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
add LazyRow abstraction for previously indexed record #11826
Conversation
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/readers/LazyRow.java
Outdated
Show resolved
Hide resolved
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/readers/LazyRow.java
Outdated
Show resolved
Hide resolved
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/readers/LazyRow.java
Show resolved
Hide resolved
fb393fd
to
49a7a98
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with minor comments
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/readers/LazyRow.java
Outdated
Show resolved
Hide resolved
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/readers/LazyRow.java
Outdated
Show resolved
Hide resolved
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/readers/LazyRow.java
Outdated
Show resolved
Hide resolved
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/readers/LazyRow.java
Outdated
Show resolved
Hide resolved
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/readers/LazyRow.java
Outdated
Show resolved
Hide resolved
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/readers/LazyRow.java
Show resolved
Hide resolved
.../src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java
Outdated
Show resolved
Hide resolved
...-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/PartialUpsertHandler.java
Outdated
Show resolved
Hide resolved
...-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/PartialUpsertHandler.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please take a look at the test failures
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/readers/LazyRow.java
Outdated
Show resolved
Hide resolved
.../java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManager.java
Outdated
Show resolved
Hide resolved
...-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/PartialUpsertHandler.java
Outdated
Show resolved
Hide resolved
...-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/PartialUpsertHandler.java
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #11826 +/- ##
============================================
- Coverage 61.35% 61.27% -0.09%
+ Complexity 1146 1145 -1
============================================
Files 2373 2374 +1
Lines 128276 128297 +21
Branches 19803 19805 +2
============================================
- Hits 78706 78608 -98
- Misses 43872 44005 +133
+ Partials 5698 5684 -14
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 31 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
refactor
Based on offline discussion for issue: #11174 and previous draft PR: #11584
Design doc: https://docs.google.com/document/d/1bBTCYZFP2stvzc6xZUOEh-XweVgC9WfD7uiSPbKtaZY/edit
There is a need for abstracting previously presisted row as an object to enable mergers between previous and current row which can return multiple merged column values in a single method.
This is a pre-requisite refactoring is needed PartialUpsertMerger contract to updated as
void merge(LazyRow previousRow, GenericRow currentRow, Map<String, Object> mergedValues)
The current change includes:
LazyRow _reusePreviousRow
in BasePartitionUpsertMetadataManager to be reused per consuming partition._reusePreviousRow
with previous row's indexSegment and docId.Local test setup:
Result:
Pinot_Data Explorer (2).csv
The rows as per the upsert config get merged, result is sorted in ascending order of insertion.