gortiz commented on PR #18963: URL: https://github.com/apache/pinot/pull/18963#issuecomment-4959100579
Thanks for closing this coverage gap — `IFSTIndexHandler` sharing FST's lifecycle logic but having zero tests was an easy thing to miss, and these 5 cases (drop/add/no-op/metadata-absent) map cleanly onto the actual branches in `needUpdateIndices`/`updateIndices`. Nice work. A couple of style nits before merging, mostly so this matches conventions we'd like to see more broadly in the codebase: 1. **Doc comment style** — could you convert the class-level Javadoc to the newer [JEP 467](https://openjdk.org/jeps/467) Markdown style (contiguous `///` lines) instead of the classic `/** ... */` block with HTML `<p>`/`<ul>`/`<li>` tags? E.g.: ```java /// Unit tests for [IFSTIndexHandler]. /// /// IFST (case-insensitive FST) indexes share the same index lifecycle as FST indexes but /// use a separate index type ([StandardIndexes#ifst()]) and file extension. /// /// Covers: /// - Index removal when a column is dropped from the IFST index config /// - New index creation when a column is added to the config /// - No update required when the index is already present and matches config /// - No update when column metadata is absent (column not in segment) ``` This reads more cleanly as plain Markdown and is the direction we'd like new test/doc comments to move in. 2. **`// ---- section divider ----` comments** — could you drop these? They're not a pattern used elsewhere in this test package (or the module generally), and the test method names are already descriptive enough to self-group without banner comments. Blank lines between tests are sufficient. 3. Minor: the class doc's bullet list could call out the "index removed via `updateIndices`" case explicitly (currently implied under the first bullet) since it's its own test. Non-blocking, but as a possible follow-up: `checkUnsupportedOperationsForIFSTIndex`'s three `UnsupportedOperationException` branches (non-STRING type, no dictionary, multi-value column) aren't exercised here — might be worth a couple more cases, though I see `FSTIndexHandler` doesn't test those either, so happy to leave that for a separate pass. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
