gortiz commented on PR #18960:
URL: https://github.com/apache/pinot/pull/18960#issuecomment-4959243390
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 `FSTIndexHandler`)?
**Style nit** — the `//
---------------------------------------------------------------------------`
section banners aren't a pattern we typically use in Pinot to group tests
within a class (see `VectorIndexHandlerTest`/`InvertedIndexHandlerTest` for the
existing convention). Would suggest dropping them and letting descriptive test
method names carry that grouping instead.
**Test coverage** — this covers `needUpdateIndices`/removal well, but a
couple of gaps relative to the PR title ("index lifecycle"):
- `checkUnsupportedOperationsForFSTIndex` (non-`STRING` type,
non-dictionary-encoded, multi-value columns → `UnsupportedOperationException`)
isn't exercised at all. The "new column added" test already builds a
`ColumnMetadata` mock that could be parameterized to hit these branches.
- The actual index-creation path (`createFSTIndexForColumn` — marker file,
`IndexCreationContext`, `FSTIndexCreator`, dictionary iteration, v3 write) is
never invoked/verified. Right now `updateIndices` is only tested for the
removal branches; it'd be good to add a case confirming a new FST index
actually gets created for a newly-added column, not just that
`needUpdateIndices` detects the need for one.
Happy to help think through mocking the creation path if that's tricky.
--
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]
