JingsongLi commented on code in PR #8206:
URL: https://github.com/apache/paimon/pull/8206#discussion_r3401605389
##########
paimon-core/src/main/java/org/apache/paimon/mergetree/LookupLevels.java:
##########
@@ -129,7 +135,36 @@ public void notifyDropFile(String file) {
@Nullable
public T lookup(InternalRow key, int startLevel) throws IOException {
- return LookupUtils.lookup(levels, key, startLevel, this::lookup,
this::lookupLevel0);
+ T result = LookupUtils.lookup(levels, key, startLevel, this::lookup,
this::lookupLevel0);
Review Comment:
This check only covers the situation where 'deleted rows are hit in the
cache lookup file', but when the cold cache/cache is retired, it may still
retrieve older versions at a deeper level. Create SstFileFromDataMile will use
a reader with dvFactory to build a lookup file, and the reader will first
filter deleted rows using DV; So the latest record at the current level is
filtered and returns null, and LookupUtils.lookup continues to search the next
layer, possibly returning an old version. This is semantically inconsistent
with the comment 'deleted hit means newest version is gone; must not continue
deeper'. Suggest not applying DV first when building lookup SST. Keep the
position and have LookupLevels verify and short-circuit it, or explicitly
distinguish between "hit but deleted by DV" and "current file has no key"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
