Convert more classes to record classes #13328

shubhamvishu · 2024-04-30T02:25:43Z

Description

This PR addresses Find more classes in main branch that can be converted to record classes #13207 to convert more classes on main branch to record classes on main (Lucene 10 only).
It moves a lot of data classes(120 to be precise) to use record classes that were flagged suitable in my IDE and seemed good candidate in general.
I left a couple of them like TotalHits and SynonymMap as 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) but renderJavadoc task 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.

lucene/core/src/java/org/apache/lucene/codecs/TermStats.java

uschindler · 2024-04-30T16:02:03Z

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 @param.

lucene/core/src/java/org/apache/lucene/util/TermAndVector.java

lucene/core/src/java/org/apache/lucene/util/packed/PackedInts.java

lucene/core/src/java/org/apache/lucene/search/CollectionStatistics.java

shubhamvishu · 2024-05-02T00:12:05Z

I addressed your comments in the new revision and all the checks are passing now after your fix. Thanks!

uschindler · 2024-05-02T21:51:27Z

Thanks, looks fine. I have no time to do another closer review, please give me some time to proceed.

uschindler

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.

uschindler · 2024-05-02T21:52:31Z

lucene/analysis/common/src/java/org/apache/lucene/analysis/hunspell/GeneratingSuggester.java

+      implements Comparable<Weighted<T>> {

    @Override
    public boolean equals(Object o) {


needs to be removed, too

uschindler · 2024-05-02T21:53:18Z

lucene/analysis/common/src/java/org/apache/lucene/analysis/hunspell/GeneratingSuggester.java

-      return Objects.hash(word, score);
-    }
-
    @Override


let's keep that one, as the toString output is different from default

uschindler · 2024-05-02T21:55:31Z

lucene/analysis/common/src/java/org/apache/lucene/analysis/synonym/word2vec/TermAndBoost.java

+ */
+public record TermAndBoost(BytesRef term, float boost) {
  /** Creates a new TermAndBoost */
  public TermAndBoost(BytesRef term, float boost) {


this can be converted to the generic ctor without parameters:

public TermAndBoost { term = BytesRef.deepCopyOf(term); }

Done!.....I changed others such occurrences that I could find as well

lucene/codecs/src/java/org/apache/lucene/codecs/blocktreeords/FSTOrdsOutputs.java

lucene/codecs/src/java/org/apache/lucene/codecs/blocktreeords/OrdsBlockTreeTermsWriter.java

uschindler · 2024-05-02T22:04:59Z

lucene/core/src/java/org/apache/lucene/document/NearestNeighbor.java

+  record Cell(
+      PointTree index, int readerIndex, byte[] minPacked, byte[] maxPacked, double distanceSortKey)
+      implements Comparable<Cell> {
+    Cell(


remove that ctor and replace by default ctor and only copy minPacked and maxPacked (see above).

github-actions · 2024-05-17T00:18:22Z

This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution!

uschindler · 2024-05-18T17:15:15Z

Hi @shubhamvishu,
can you check my comments? The changes here look fine, please fix the remaining issues.
Uwe

shubhamvishu · 2024-05-20T17:06:46Z

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.

Update : Got stuck with some other work. Will take a look at this sometime this week.

…syntax

shubhamvishu · 2024-06-03T00:53:51Z

Hi @uschindler , thanks for the review. I took another pass at the changes and pushed some commits addressing your comments. Please have a look when you get a chance. Thanks!

github-actions · 2024-06-26T00:18:57Z

This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution!

shubhamvishu · 2024-07-04T16:52:11Z

Hi @uschindler, Could you please review the current code changes once you get some time and if it looks good maybe we can move this forward?

github-actions · 2024-07-19T01:11:01Z

This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution!

stefanvodita · 2024-09-09T15:49:23Z

Added the blocker label to draw attention to this PR for the Lucene 10 release.

shubhamvishu · 2024-09-09T16:54:01Z

Hi @uschindler @mikemccand @jpountz , I fixed the recent merge conflicts and looking for reviews as 10.0 release is blocked on this change. This PR has been attracting merge conflicts frequently since its touching 200+ files, so hoping we could close this sooner. Let me know if there is any concerns here(I'm up for quick followups). Thanks!

jpountz

I left some minor comments but the change looks good to me in general. Can you add an entry to lucene/MIGRATE.txt that explains that TopDocs, CollectionStatistics, TermStatistics and LeafMetadata were converted to records? I'm only considering mentioning these because these are the most user-facing APIs that are migrated to a record from what I can tell (please let me know if you think I'm missing an important one).

lucene/core/src/java/org/apache/lucene/search/CollectionStatistics.java

jpountz · 2024-09-09T17:16:49Z

lucene/misc/src/java/org/apache/lucene/misc/util/fst/UpToTwoPositiveIntOutputs.java

-    @Override
-    public String toString() {
-      return "TwoLongs:" + first + "," + second;
-    }


we'll get a different toString() but I think it's fine in this case

Yes, its fine in this case.

lucene/monitor/src/java/org/apache/lucene/monitor/HighlightsMatch.java

jpountz · 2024-09-09T17:22:29Z

lucene/sandbox/src/java/org/apache/lucene/sandbox/search/CombinedFieldQuery.java

      if (o == null || getClass() != o.getClass()) return false;
      FieldAndWeight that = (FieldAndWeight) o;
      return Float.compare(that.weight, weight) == 0 && Objects.equals(field, that.field);
    }


Did you note remove this equals override because the record would behave differently? (I tried to quickly look up what record does wrt equality for floats and doubles but couldn't find it, will look later)

Nope, I think its a leftover.I'll change it. Found similar one TermAndBoost in SynonymQuery also.

(I tried to quickly look up what record does wrt equality for floats and doubles but couldn't find it, will look later)

I found one mention of floating point numbers in the JEP for records:

In addition, for all record classes the implicitly declared equals method is implemented so that it is reflexive and that it behaves consistently with hashCode for record classes that have floating point components. Again, explicitly declared equals and hashCode methods should behave similarly.

I'm not sure I understand that paragraph :)

Hi,
The equals methods for floats is generated by invokedynamic at runtime using a bootstrapping:

https://github.com/openjdk/jdk/blob/9785e19f3f87306cabc26a862d35b89d41cfef93/src/java.base/share/classes/java/lang/runtime/ObjectMethods.java#L382

The class file of a record only contains a stub equals/hashcode/to string which delegates to an invokedynamic call passing the class which allows the bootstrapped to get all record components and construct a method handle for the whole method implementation.

When a record component is a float, the autogenerated impl using this impl to handle floats. It compares them using this code:

https://github.com/openjdk/jdk/blob/9785e19f3f87306cabc26a862d35b89d41cfef93/src/java.base/share/classes/java/lang/runtime/ObjectMethods.java#L157

The method handles used by the code generator are saved in a map:

https://github.com/openjdk/jdk/blob/9785e19f3f87306cabc26a862d35b89d41cfef93/src/java.base/share/classes/java/lang/runtime/ObjectMethods.java#L101

This is equivalent to the already existing code. So please remove all custom equals methods.

Unfortunately in the merged PR there are some more. Please remove them in a followup.

Nice! I'll check and followup with removing others I could find.

...est-framework/src/java/org/apache/lucene/tests/codecs/asserting/AssertingLiveDocsFormat.java

lucene/test-framework/src/java/org/apache/lucene/tests/index/AssertingLeafReader.java

jpountz · 2024-09-09T17:29:09Z

lucene/core/src/java/org/apache/lucene/util/FixedBits.java

-    this.bits = bits;
-    this.length = length;
-  }
+record FixedBits(long[] bits, int length) implements Bits {


it doesn't really fit my mental model for a record, maybe undo the refactoring on this class?

shubhamvishu · 2024-09-10T09:40:35Z

@jpountz I addressed your comments and added a CHANGES, MIGRATE entry now. Thanks for reviewing!

mikemccand

Thank you for persisting here @shubhamvishu -- this is a massive rote refactoring change. We should rather bias towards quickly merging such changes since they pick up conflicts so quickly. I just left some minor feedback about wording in CHANGES and MIGRATE, else +1 to merge.

mikemccand · 2024-09-10T13:35:11Z

lucene/CHANGES.txt

  argument with a `FacetsCollectorManager` and update the return type to include both `TopDocs` results as well as
  facets results. (Luca Cavanna)

+* GITHUB#13328: Convert CollectionStatistics, TermStatistics and LeafMetadata etc. to record classes. (Shubham Chaudhary)


Maybe reword to Convert many basic Lucene classes to record classes, including X, Y, and Z? It's not just these three classes, if I'm reading the PR correctly, and the etc. should not have so much power ;)

This looks better indeed. I'll reword it.

mikemccand · 2024-09-10T13:37:02Z

lucene/MIGRATE.md


 - `IOContext`, `MergeInfo`, and `FlushInfo` (GITHUB#13205)
 - `BooleanClause` (GITHUB#13261)
+- `CollectionStatistics`, `TermStatistics` and `LeafMetadata` (GITHUB#13328)


And maybe change this wording too (same as CHANGES.txt)? I think @jpountz asked for TopDocs to also be mentioned?

Sure, I'll reword. I haven't changed TopDocs in this PR and hence I didn't include it. Maybe some confusion(prolly because there are just too many classes changed here)

mikemccand · 2024-09-10T13:42:43Z

lucene/sandbox/src/java/org/apache/lucene/sandbox/search/CombinedFieldQuery.java

      if (o == null || getClass() != o.getClass()) return false;
      FieldAndWeight that = (FieldAndWeight) o;
      return Float.compare(that.weight, weight) == 0 && Objects.equals(field, that.field);
    }


(I tried to quickly look up what record does wrt equality for floats and doubles but couldn't find it, will look later)

I found one mention of floating point numbers in the JEP for records:

In addition, for all record classes the implicitly declared equals method is implemented so that it is reflexive and that it behaves consistently with hashCode for record classes that have floating point components. Again, explicitly declared equals and hashCode methods should behave similarly.

I'm not sure I understand that paragraph :)

stefanvodita · 2024-09-10T21:24:56Z

I think we've run into a conflict with #13689. @jpountz - should I fix that or are you already doing it?

jpountz · 2024-09-10T21:41:35Z

@javanna just fixed it, thanks @javanna !

uschindler

There is one as symmetry in removal of equals/hashcode.

Please fix this in a new PR.

uschindler · 2024-09-10T22:28:58Z

lucene/core/src/java/org/apache/lucene/search/MaxScoreAccumulator.java

      return docBase == result.docBase && Float.compare(result.score, score) == 0;
    }

-    @Override


Please also remove the equals, see this comment: #13328 (comment)

To string is also useless here. Please remove, too

Sure, I'll fix it in a followup PR. I could only find this instance in the classes changed to records where we have removable custom equals/toString but I'll take another pass to be sure.

I opened a followup PR #13769. Thanks!

Convert more classes to record classes

299d344

shubhamvishu commented Apr 30, 2024

View reviewed changes

lucene/core/src/java/org/apache/lucene/codecs/TermStats.java Show resolved Hide resolved

shubhamvishu mentioned this pull request Apr 30, 2024

Temporarily exclude record ctors and methods from javadoc checks #13329

Closed

uschindler mentioned this pull request Apr 30, 2024

Improve MissingDoclet linter to check records correctly #13332

Merged

uschindler added this to the 10.0.0 milestone Apr 30, 2024

uschindler self-assigned this Apr 30, 2024

uschindler mentioned this pull request Apr 30, 2024

Find more classes in main branch that can be converted to record classes #13207

Closed

6 tasks

uschindler reviewed Apr 30, 2024

View reviewed changes

lucene/core/src/java/org/apache/lucene/util/TermAndVector.java Outdated Show resolved Hide resolved

uschindler reviewed Apr 30, 2024

View reviewed changes

lucene/core/src/java/org/apache/lucene/util/packed/PackedInts.java Outdated Show resolved Hide resolved

uschindler reviewed Apr 30, 2024

View reviewed changes

lucene/core/src/java/org/apache/lucene/search/CollectionStatistics.java Show resolved Hide resolved

shubhamvishu added 3 commits May 1, 2024 15:48

Merge branch 'main' into use-record-classes

399c9f9

Addressed comments and got all checks passed

71bf91b

Made records leaner

42680f1

uschindler requested changes May 2, 2024

View reviewed changes

github-actions bot added the Stale label May 17, 2024

github-actions bot removed the Stale label May 19, 2024

shubhamvishu added 4 commits June 3, 2024 02:39

Merge branch 'main' into use-record-classes

dd8f467

Retain LabelAndValue as class after conflict from apache#13414

7e4aacb

Remove extra equals,hashCode etc methods and use default constructor …

5addff7

…syntax

Some more changes

0c91d18

Merge branch 'main' into use-record-classes

31c8850

shubhamvishu requested a review from uschindler June 11, 2024 20:42

github-actions bot added the Stale label Jun 26, 2024

github-actions bot removed the Stale label Jul 5, 2024

github-actions bot added the Stale label Jul 19, 2024

stefanvodita added the blocker A severe issue that should be resolved before the released specified in its Milestone. label Sep 9, 2024

Merge branch 'main' into use-record-classes

f8e9ff1

shubhamvishu force-pushed the use-record-classes branch from 0b39341 to f8e9ff1 Compare September 9, 2024 16:48

jpountz approved these changes Sep 9, 2024

View reviewed changes

github-actions bot removed the Stale label Sep 10, 2024

shubhamvishu added 3 commits September 10, 2024 14:31

Address comments

f1fd960

Merge branch 'main' into use-record-classes

29b34e0

Add MIGRATE and CHANGES entry

4db2f23

mikemccand approved these changes Sep 10, 2024

View reviewed changes

shubhamvishu added 2 commits September 10, 2024 21:43

Rephrase entries

6c76a8b

Merge branch 'main' into use-record-classes

6c35fee

jpountz merged commit 942065c into apache:main Sep 10, 2024

uschindler reviewed Sep 10, 2024

View reviewed changes

shubhamvishu mentioned this pull request Sep 11, 2024

Remove unnecessary methods from DocAndScore #13769

Merged

Convert more classes to record classes #13328

Convert more classes to record classes #13328

Uh oh!

Conversation

shubhamvishu commented Apr 30, 2024

Description

Uh oh!

Uh oh!

uschindler commented Apr 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shubhamvishu commented May 2, 2024

Uh oh!

uschindler commented May 2, 2024

Uh oh!

uschindler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented May 17, 2024

Uh oh!

uschindler commented May 18, 2024

Uh oh!

shubhamvishu commented May 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shubhamvishu commented Jun 3, 2024

Uh oh!

github-actions bot commented Jun 26, 2024

Uh oh!

shubhamvishu commented Jul 4, 2024

Uh oh!

github-actions bot commented Jul 19, 2024

Uh oh!

stefanvodita commented Sep 9, 2024

Uh oh!

shubhamvishu commented Sep 9, 2024

Uh oh!

jpountz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uschindler Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

uschindler commented Apr 30, 2024 •

edited

Loading

shubhamvishu commented May 20, 2024 •

edited

Loading

uschindler Sep 10, 2024 •

edited

Loading