Add unit tests for FSTIndexHandler legacy-FST detection and index lifecycle#18960
Add unit tests for FSTIndexHandler legacy-FST detection and index lifecycle#18960Akanksha-kedia wants to merge 4 commits into
Conversation
FSTIndexHandler had no unit test coverage. Adds FSTIndexHandlerTest with 6 tests covering: - Legacy native FST index detected via magic bytes triggers rebuild - updateIndices removes legacy native FST index - Column dropped from config triggers index removal - updateIndices removes index for column dropped from config - New column in config triggers index creation - Index already up-to-date (Lucene FST, in config) requires no update The legacy-native check is tested by mocking PinotDataBuffer.getInt(0) to return the native magic constant, mirroring FstIndexUtils.isLegacyNativeFst. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #18960 +/- ##
============================================
+ Coverage 64.86% 65.25% +0.39%
- Complexity 1347 1405 +58
============================================
Files 3392 3418 +26
Lines 211663 214642 +2979
Branches 33306 33919 +613
============================================
+ Hits 137290 140069 +2779
+ Misses 63296 63272 -24
- Partials 11077 11301 +224
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
The newly added test is failing. Please take a look |
… on ColumnMetadata mock checkUnsupportedOperationsForFSTIndex() validates that the column is STRING, dictionary-encoded, and single-value before creating the index. The mock returned null for getDataType(), causing null != STRING to throw UnsupportedOperationException instead of returning true.
…ler preconditions Stub getTotalDocs()→1 and getAllColumns()→TreeSet in all SegmentMetadataImpl mocks so the BaseIndexHandler constructor precondition check does not throw.
|
@xiangfu0 please take a look when you get a chance |
|
@xiangfu0 @shounakmk219 would appreciate a review when you get a chance. This PR adds unit tests for |
|
Thanks for adding coverage here! A few notes: PR description — the description currently just contains an error string ("error connecting to userproxy.visa.com…"), which looks like a stray paste rather than an actual description. Could you update it to describe what this adds and why (e.g. legacy-FST detection + config-driven add/remove lifecycle for Style nit — the Test coverage — this covers
Happy to help think through mocking the creation path if that's tricky. |
…on path coverage - Convert class Javadoc to JEP 467 Markdown (///) style - Drop // ---- section-banner comments - Replace bare Map.of() with explicit Map.of(COLUMN, NO_FST) using FstIndexConfig.DISABLED so removed-column intent is unambiguous - Add three checkUnsupportedOperationsForFSTIndex tests: non-STRING data type, no dictionary, and multi-value column each throw UnsupportedOperationException from needUpdateIndices - Add testUpdateIndicesCreatesNewFstIndexForNewColumn: builds a real v1 segment, verifies needUpdateIndices returns true, calls updateIndices, and asserts the .lucene.v912.fst file is written
4 participants
error connecting to userproxy.visa.com
check your internet connection or https://githubstatus.com