Skip to content

Conversation

romseygeek
Copy link
Contributor

No description provided.

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-storage-engine (Team:StorageEngine)

@romseygeek
Copy link
Contributor Author

Note that we can't make this a record and keep the private constructor. I'm semi-wondering if I should make this an enum again, but keeping the six boolean fields.

@thecoop
Copy link
Member

thecoop commented Oct 9, 2025

Its definitely worth a comment on why it shouldn't be a record, to preempt some enthusiastic refactoring. Only certain combinations of IndexType being valid does point towards an enum (which would solve the equals/toString automatically)

Copy link
Member

@felixbarny felixbarny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think the way it is right now (not a boolean and not a record) fits this use case well. Especially since there can be a couple of valid permutations for terms and points.

Reading the static factory methods for terms and points again, I noticed they can be simplified by passing in the booleans from the arguments to the constructor directly and return NONE if both arguments are false.

@romseygeek romseygeek merged commit fc7ba8d into elastic:main Oct 10, 2025
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants