-
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
Support Upsert deletion for TTL: construct queryableDocIds when adding segments out of TTL #11791
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11791 +/- ##
=============================================
+ Coverage 34.84% 61.42% +26.57%
- Complexity 946 1147 +201
=============================================
Files 2299 2378 +79
Lines 124763 128880 +4117
Branches 19289 19927 +638
=============================================
+ Hits 43479 79166 +35687
+ Misses 78243 43997 -34246
- Partials 3041 5717 +2676
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 853 files with indirect coverage changes 📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
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.
This is incorrect. Please add a test to ensure we can get the correct behavior
.../src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.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.
Good in general. We can further optimize it
.../src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java
Outdated
Show resolved
Hide resolved
queryableDocIds = new MutableRoaringBitmap(); | ||
// we allow passing null values for primaryKey columns and comparisonColumns to read deleteColumn to avoid paying | ||
// the overhead of reading primary key, comparison column etc. They will never be null in other scenarios. | ||
UpsertUtils.RecordInfoReader recordInfoReader = |
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.
Suggest not modifying the RecordInfoReader
because it is not designed for this purpose. We can simply create a PinotSegmentColumnReader
for _deleteRecordColumn
, then iterate over the validDocIds
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.
when iterate the value of _deleteRecordColumn, can deletedRecord be null? can this be further optimize?
if (deleteRecordColumnReader.isNull(doc_id)) {
// defaultNullValue for deleteColumn is "false"
queryableDocIds.add(doc_id);
} else if (!(Boolean) deleteRecordColumnReader.getValue(doc_id)) {
queryableDocIds.add(doc_id);
}
b604596
to
7c59d4a
Compare
8a86e84
to
44c3e6d
Compare
44c3e6d
to
d02bf67
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 in general. Applied a clean up commit
thanks @Jackie-Jiang! |
bugfix
,upsert
: construct queryableDocIds when adding segments out of TTLPreviously we don't handle cases that a upsert table with both upsert deletion and upsert ttl.
In this PR, fixed todo items, construct queryableDocIds when adding segments out of TTL.