-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refine IndexHandler methods a bit to make them reentrant #9440
Conversation
@@ -74,22 +74,23 @@ public class FSTIndexHandler implements IndexHandler { | |||
public FSTIndexHandler(SegmentMetadata segmentMetadata, IndexLoadingConfig indexLoadingConfig) { | |||
_segmentMetadata = segmentMetadata; | |||
_fstType = indexLoadingConfig.getFSTIndexType(); | |||
_columnsToAddIdx = new HashSet<>(indexLoadingConfig.getFSTIndexColumns()); | |||
_columnsToAddIdx = indexLoadingConfig.getFSTIndexColumns(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nit) Isn't it probably cleaner and safer (from future changes) to not have _columnsToAddIdx
as a private member? Instead just have _indexLoadingConfig
as a private member and compute columnsToAddIdx
in the respective methods?
This is the pattern followed in other handlers like BloomFilterHandler,H3IndexHandler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see BF/H3 hander doesn't hold indexLoadingConfig reference either, but configs for creating BF/H3.
The handlers in this PR don't have specific configs from indexLoadingConfig either, so not hold its reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor comment. LGTM.
lgtm overall |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Use a defensive copy to make the IndexHandler methods reentrant and less confusing to use. This is not an issue today, as IndexHandler object is not reused.