-
Notifications
You must be signed in to change notification settings - Fork 3.8k
17735 trunk #1737
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
17735 trunk #1737
Conversation
…ity - Settings Virtual Table was not updated on changes after startup
adelapena
left a comment
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.
Looks good, I have just left a couple of suggestions.
| MINUTES_DURATION(Integer.class, DurationSpec.IntMinutesBound.class, | ||
| DurationSpec.IntMinutesBound::new, | ||
| DurationSpec.IntMinutesBound::toMinutes), | ||
| o -> o == -1 ? null : new DurationSpec.IntMinutesBound(o), |
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.
Maybe we should rename this converter to MINUTES_CUSTOM_DURATION and add a comment, since this is a special way of converting due to the acceptance of -1 as disabled value. That's what was done with MILLIS_CUSTOM_DURATION, although here we wouldn't have a regular/custom pair of converters.
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.
That and I actually got reminded that I need to update this converter unit test to include the "-1" part
d145352 to
d234872
Compare
… to `EC` so we support BM25 by default. (apache#1737) We do not need to update JVECTOR_VERSION post CNDB-14301
… to `EC` so we support BM25 by default. (apache#1737) We do not need to update JVECTOR_VERSION post CNDB-14301
No description provided.