Skip to content

Improve query modification loading memory control#17788

Open
shuwenwei wants to merge 3 commits into
masterfrom
loadModificationFileWithMemControl
Open

Improve query modification loading memory control#17788
shuwenwei wants to merge 3 commits into
masterfrom
loadModificationFileWithMemControl

Conversation

@shuwenwei
Copy link
Copy Markdown
Member

Summary

  • Add quota pre-checking before loading modification files into the query mods cache.
  • Reserve memory incrementally while building modification pattern trees.
  • Fall back to path-matched scanning when cache quota or memory reservation is insufficient.
  • Release partial tree quota and memory before returning fallback matched modifications.
  • Extend unit coverage for cache loading, fallback scanning, final reservation failure, and sort/merge reservation adjustment.

@sonarqubecloud
Copy link
Copy Markdown

@codecov
Copy link
Copy Markdown

codecov Bot commented May 29, 2026

Codecov Report

❌ Patch coverage is 78.45304% with 39 lines in your changes missing coverage. Please review.
✅ Project coverage is 40.80%. Comparing base (0c25e53) to head (55606d4).
⚠️ Report is 14 commits behind head on master.

Files with missing lines Patch % Lines
...ne/execution/fragment/FragmentInstanceContext.java 18.18% 18 Missing ⚠️
...ne/execution/fragment/QueryModificationLoader.java 93.23% 9 Missing ⚠️
.../memory/NotThreadSafeMemoryReservationManager.java 0.00% 5 Missing ⚠️
...b/queryengine/execution/fragment/QueryContext.java 77.77% 2 Missing ⚠️
...ner/memory/ThreadSafeMemoryReservationManager.java 0.00% 2 Missing ⚠️
...torageengine/dataregion/tsfile/TsFileResource.java 77.77% 2 Missing ⚠️
.../planner/memory/FakedMemoryReservationManager.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #17788      +/-   ##
============================================
+ Coverage     40.58%   40.80%   +0.21%     
- Complexity     2575     2610      +35     
============================================
  Files          5181     5185       +4     
  Lines        350404   351013     +609     
  Branches      44801    44906     +105     
============================================
+ Hits         142225   143243    +1018     
+ Misses       208179   207770     -409     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Caideyipi
Copy link
Copy Markdown
Collaborator

Review notes:

  1. QueryModificationLoader.java:115 uses TsFileResource.getTotalModSizeInByte() as the quota pre-check before trying to cache mods. However, TsFileResource.getTotalModSizeInByte() currently counts the full shared mods file length (TsFileResource.java:453-455), while the actual iterator only reads shared mods from sharedModFileOffset (TsFileResource.java:1515-1516). For TsFiles using shared mods, this can make the pre-check fail just because the shared file has grown globally, even when the unread portion for this TsFile is small. That means cache loading will be bypassed and the query will repeatedly fall back to scan. Please consider using the actual readable mods size here, e.g. exclusiveModFileLength + max(0, sharedModFileLength - sharedModFileOffset).

  2. FragmentInstanceContext.java:413 uses modification -> modification.affects(deviceID) as the fallback matcher for the device-only overload. This is broader than the cache path (mods -> getPathModifications(mods, deviceID)), which matches deviceID + AlignedPath.VECTOR_PLACEHOLDER. For example, a deletion on root.sg.d1.s1 affects device root.sg.d1, but it does not match the time-column/vector-placeholder path. So cache success and fallback scan can return different results for the same API. Even if this overload is not on the hot path today, the fallback predicate should be equivalent to the pattern-tree matcher.

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