Uh oh!
There was an error while loading. Please reload this page.
Optimized the overall performance of IoTDB & Fixed the NPE in LimitOperatorTest - #17664
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@## master #17664 +/- ##
============================================
+ Coverage 40.26% 40.48% +0.21%
Complexity 2574 2574 ============================================
Files 5179 5179 Lines 349659 350152 +493 Branches 44688 44780 +92 ============================================
+ Hits 140798 141756 +958 + Misses 208861 208396 -465 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
JackieTien97
left a comment
There was a problem hiding this comment.
Thanks for the performance work — the changes are mostly solid and several are clear wins: the zero-copy wrapBuffer rewrite in QueryDataSetUtils, the bytesToHex lookup-table rewrite, factoring the device/time filter out of the per-measurement loop in ModificationUtils, the removeAll write-lock leak fix in TsFileManager, and the new matchFullPath(IDeviceID) / matchPrefixPath(IDeviceID) overloads in PartialPath (no more throwaway PartialPath allocations on every cache check).
A few items below are worth confirming before merge, grouped by priority:
Should confirm (potential correctness)
SessionDataSet(Java + C++): the loop switched to 1-based index access (valueColumnStartIndex + 1 .. columnSize). Please confirm the index-based getters are 1-based relative to the column-name list — an off-by-one here silently reads adjacent columns with no error.Session.java:nullMapis nownullwhen info logging is disabled. Every subsequent dereference in those methods must be guarded.UnclosedFileScanHandleImpl: the new lazyHashMapcaches are not thread-safe; please confirm single-threaded access or switch toConcurrentHashMap.TableDeviceSchemaCache.invalidateLastCache: thematchFullPathdirection was swapped — the new direction looks correct (and likely fixes a latent wildcard bug), worth calling out and covering with a test.ModificationUtils: confirm the newoverlap(...)helper has the same inclusive/exclusive boundary semantics asTimeRange.overlaps.
Minor / style / cleanup
- Inline fully-qualified
java.util.Collection/java.util.HashSetinSchemaEngineandStorageEngine— please use imports. QueryDataSetUtils:rowCountandvalueOccupationparameters are unused after the refactor — please drop them.IoTDBJDBCDataSet:getDeduplicatedColumnSizenow returnsmax(index)+1instead of distinct-count — equivalent for contiguous indices, just confirm no downstream code depends on the old size.
Details inline.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
luoluoyuyu
left a comment
There was a problem hiding this comment.
Review summary
Performance improvements across client, schema cache, storage scan, and TsFileManager. The removeAll finally block now calls writeUnlock instead of acquiring the write lock again, fixing a serious lock leak.
Please fix CI failures in this PR. See inline notes.
| private boolean filterNullValueAndMeasurementWithStringType( | ||
| List<String> valuesList, String deviceId, List<String> measurementsList) { | ||
| Map<String, Object> nullMap = new HashMap<>(); | ||
| Map<String, Object> nullMap = logger.isInfoEnabled() ? new HashMap<>() : null; |
There was a problem hiding this comment.
nullMap is null when info logging is disabled; all uses in this method are guarded. Please verify every overload of filterNullValueAndMeasurement changed in this PR uses the same pattern.
There was a problem hiding this comment.
Verified. The two filterNullValueAndMeasurement overloads changed in this PR guard every nullMap access with nullMap != null, and the wrapper overloads only delegate to them without dereferencing the map.
| } | ||
| } finally { | ||
| writeLock("removeAll"); | ||
| writeUnlock(); |
There was a problem hiding this comment.
Critical fix: finally now calls writeUnlock() instead of writeLock again in removeAll, which previously leaked the write lock on every call.
There was a problem hiding this comment.
Confirmed, this fix is included and has been called out in the PR description as a correctness fix, not just a performance optimization.
| List<TimeRange> timeRangeList = | ||
| modifications.stream().map(ModEntry::getTimeRange).collect(Collectors.toList()); | ||
| return ModificationUtils.isPointDeletedWithoutOrderedRange(timestamp, timeRangeList); | ||
| List<TimeRange> timeRangeList = deviceToDeletionRanges.get(deviceID); |
There was a problem hiding this comment.
deviceToDeletionRanges caches merged deletion ranges per device via putIfAbsent, avoiding repeated sortAndMerge in isDeviceTimeDeleted. If this handle can be used from multiple threads, document single-thread use or use a concurrent map.
There was a problem hiding this comment.
Verified. ClosedFileScanHandleImpl now uses ConcurrentHashMap for deviceToDeletionRanges and deviceToModifications, publishes device-level ranges with putIfAbsent, and creates per-device time-series maps as ConcurrentHashMap instances as well.
Caideyipi
commented
Jun 1, 2026
Thanks for the review. I rechecked the latest head and replied inline. The Windows and IT failures mentioned earlier are now green. The remaining C++ macOS failure was a Maven assembly OOM (Java heap space) while building iotdb-cli, so I reran the failed Multi-Language Client workflow; the macOS rerun is pending now. |
Uh oh!
There was an error while loading. Please reload this page.
…eratorTest (apache#17664) * Opt * Update UnclosedFileScanHandleImpl.java * Update StorageEngine.java * Update ClosedFileScanHandleImpl.java * column index * spt * Address performance review comments * fix (cherry picked from commit 89730b1)
…eratorTest (apache#17664) * Opt * Update UnclosedFileScanHandleImpl.java * Update StorageEngine.java * Update ClosedFileScanHandleImpl.java * column index * spt * Address performance review comments * fix



Description
This PR has:
for an unfamiliar reader.
for code coverage.
Key changed/added classes (or packages if there are too many classes) in this PR