Convert more classes to record classes #13328
Conversation
|
Hi, you can merge main into your branch and hopefully checks pass. For some packages like the "codecs" one, the new code requires you to also document all record components with |
| public float[] getVector() { | ||
| return this.vector; | ||
| } | ||
| public record TermAndVector(BytesRef term, float[] vector) { |
There was a problem hiding this comment.
this class has the problem that it supplies a method to normalize the inner vector. This is against immutability of records. It is possible to do this, but feels bad.
There was a problem hiding this comment.
Makes sense! I changed it back to class in the new revision
| this.format = format; | ||
| this.bitsPerValue = bitsPerValue; | ||
| } | ||
| public record FormatAndBits(Format format, int bitsPerValue) { | ||
|
|
||
| @Override | ||
| public String toString() { |
There was a problem hiding this comment.
do we need toString(), this one can be autogenerated, just the output is different.
There was a problem hiding this comment.
Removed this now
| @@ -116,7 +105,8 @@ public CollectionStatistics( | |||
| * | |||
| * @return field's name, not {@code null} | |||
| */ | |||
| public final String field() { | |||
There was a problem hiding this comment.
we can remove all accessor methods and add the documentation into @param on the record declaration.
There was a problem hiding this comment.
Done! I also changed some more records I could find to be as leaner as possible.
|
I addressed your comments in the new revision and all the checks are passing now after your fix. Thanks! |
|
Thanks, looks fine. I have no time to do another closer review, please give me some time to proceed. |
There was a problem hiding this comment.
I went through the first few files and have some changes:
- there were some hashCodes revoed but equals survied, please remove both
- some toString may be obsolete, too (unless the exact formatting is important)
- some ctors with exact same parameers like default can be rewritten to use the default syntax and remove the field assignments
I will proceed later on the weekend with other classes.
| this.score = score; | ||
| } | ||
| private record Weighted<T extends Comparable<T>>(T word, int score) | ||
| implements Comparable<Weighted<T>> { | ||
|
|
||
| @Override | ||
| public boolean equals(Object o) { |
There was a problem hiding this comment.
needs to be removed, too
| public int hashCode() { | ||
| return Objects.hash(word, score); | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
let's keep that one, as the toString output is different from default
| * @param term the term | ||
| * @param boost the boost | ||
| */ | ||
| public record TermAndBoost(BytesRef term, float boost) { | ||
| /** Creates a new TermAndBoost */ | ||
| public TermAndBoost(BytesRef term, float boost) { |
There was a problem hiding this comment.
this can be converted to the generic ctor without parameters:
public TermAndBoost {
term = BytesRef.deepCopyOf(term);
}| @@ -68,16 +63,6 @@ public int hashCode() { | |||
| hash = (int) (hash ^ endOrd); | |||
| return hash; | |||
| } | |||
|
|
|||
| public final BytesRef maxTerm; | ||
|
|
||
| public FieldMetaData( | ||
| private record FieldMetaData( |
There was a problem hiding this comment.
make this the default ctor (see above and remove the field assignments), just keep asserts or other checks
| record Cell( | ||
| PointTree index, int readerIndex, byte[] minPacked, byte[] maxPacked, double distanceSortKey) | ||
| implements Comparable<Cell> { | ||
| Cell( |
There was a problem hiding this comment.
remove that ctor and replace by default ctor and only copy minPacked and maxPacked (see above).
|
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the [email protected] list. Thank you for your contribution! |
|
Hi @shubhamvishu, |
|
Hi @uschindler , sorry I was on vacation until last week, so this PR stalled. I'll take a look at the comments today or tomorrow. |
Description
mainbranch to record classes on main (Lucene 10 only).TotalHitsandSynonymMapas the PR is already very big and including those were leading to a lot more changes. Maybe we could do those as a separate PR.Raising this PR since all the tests are passing(
./gradlew test) butrenderJavadoctask is complaining about missing java docs on some record classes(converted in this PR) which I see has the javadocs already eg:TermStats,ReaderSlice. I'm not sure why its flagging those incorrectly or if maybe I'm missing something.