Text to vector refactoring by nicolo-rinaldi · Pull Request #4375 · apache/solr

nicolo-rinaldi · 2026-04-28T09:46:38Z

Description

This refactoring is needed to prepare the code for the addition of the new module "Document Enrichment with LLMs" already in PR #4229, to avoid duplicated code as much as possible.

Solution

Abstract classes have been created for both the model and the store/rest packages. Also, renamed the exception to generalize across models. Implemented actual instances of the new abstract classes for the Text-to-vector module.
Claude code has been used as an assistant for coding this PR.

Tests

No tests have been added, since only interfaces and abstract classes have been developed.

Checklist

Please review the following and check all that apply:

I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
I have created a Jira issue and added the issue ID to my pull request title.
I have given Solr maintainers access to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
I have developed this patch against the main branch.
I have run ./gradlew check.
I have added tests for my changes.
I have added documentation for the Reference Guide
I have added a changelog entry for my change

…ture "Document enrichment with LLMs"

…nt enrichment module

aruggero · 2026-04-29T09:57:37Z

Would it be worth also moving the common part of this logic to SolrLanguageModel and generalizing it?

Since each model might have different parameter, I wanted to keep this separated

aruggero · 2026-04-29T10:00:27Z

+ * limitations under the License.
+ */
+
+/** Contains model store related classes. */


Isn't this rest specific?

Yes, this is leftover from a copy-paste of the license. Changed

I replied to the wrong comment. I changed it now, adding "rest"

aruggero · 2026-04-29T10:01:50Z

@@ -39,12 +39,12 @@ public void cleanup() throws Exception {
  }

  @Test
-  public void testModelAreStoredCompact() throws Exception {
+  public void testModelAreStored() throws Exception {


What does this mean?

Before it was saved as a compact file, this was done due to a (possibly) copy-paste leftover of this module from the LTR module, where it makes sense to save the files related to the models as compact, due to the possible high number of features. In both these contributions (text-to-vector and following document enrichment), it doesn't make sense to apply the compression to reduce space. This is also a consequence of the fact that the method

@Override protected ManagedResourceStorage createStorage( ManagedResourceStorage.StorageIO storageIO, SolrResourceLoader loader) throws SolrException { return new ManagedResourceStorage.JsonStorage(storageIO, loader, -1); }

from ManagedTextToVectorStore is no longer overridden. "-1" means "save in a compact format".

So does it make sense to have this test?
Was this made just to check if compact?

Yes, it can be removed.

aruggero · 2026-04-29T10:18:07Z

+ * via the REST API. Concrete subclasses supply the REST endpoint and the model instantiation logic.
+ */
+@ThreadSafe
+public abstract class ManagedModelStore<M extends SolrLanguageModel> extends ManagedResource


Is there a better name variable instead of "M"?

Generics are usually uppercase single letters that can be followed by a number. Do you still want me to rename this variable?

Maybe LM? to discuss

aruggero · 2026-04-29T10:22:50Z

-/** Simple store to manage CRUD operations on the {@link SolrTextToVectorModel} */
-public class TextToVectorModelStore {
+/** Generic store to manage CRUD operations on models that extend {@link SolrLanguageModel} */
+public class LanguageModelStore<M extends SolrLanguageModel> {


Again, a better name instead of "M"?

Same as before

aruggero · 2026-04-29T10:54:43Z


-/** Simple store to manage CRUD operations on the {@link SolrTextToVectorModel} */
-public class TextToVectorModelStore {
+/** Generic store to manage CRUD operations on models that extend {@link SolrLanguageModel} */


Is this class necessary? Can't we just have ManagedModelStore with everything inside?

We have a different endpoint for each module, so I wanted to keep them different

nicolo-rinaldi · 2026-04-29T12:52:20Z

+ * via the REST API. Concrete subclasses supply the REST endpoint and the model instantiation logic.
+ */
+@ThreadSafe
+public abstract class ManagedModelStore<M extends SolrLanguageModel> extends ManagedResource


Generics are usually uppercase single letters that can be followed by a number. Do you still want me to rename this variable?

nicolo-rinaldi · 2026-04-29T13:01:37Z

+ * limitations under the License.
+ */
+
+/** Contains model store related classes. */


Yes, this is leftover from a copy-paste of the license. Changed

nicolo-rinaldi · 2026-04-29T13:02:34Z


-/** Simple store to manage CRUD operations on the {@link SolrTextToVectorModel} */
-public class TextToVectorModelStore {
+/** Generic store to manage CRUD operations on models that extend {@link SolrLanguageModel} */


We have a different endpoint for each module, so I wanted to keep them different

nicolo-rinaldi · 2026-04-29T13:03:15Z

-/** Simple store to manage CRUD operations on the {@link SolrTextToVectorModel} */
-public class TextToVectorModelStore {
+/** Generic store to manage CRUD operations on models that extend {@link SolrLanguageModel} */
+public class LanguageModelStore<M extends SolrLanguageModel> {


Same as before

nicolo-rinaldi · 2026-04-29T13:17:55Z

-  }
-
-  @Override
-  protected ManagedResourceStorage createStorage(


See the comment in TestModelManagerPersistence

nicolo-rinaldi · 2026-04-29T13:34:49Z

+@ThreadSafe
+public abstract class ManagedModelStore<M extends SolrLanguageModel> extends ManagedResource
+    implements ManagedResource.ChildResourceSupport {
+  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());


I kept it here due to the fact that most of the logic is in the function that are already in the abstract class

nicolo-rinaldi · 2026-04-29T13:44:44Z

+   * @param modelMap a map containing {@code "class"}, {@code "name"}, and {@code "params"} keys
+   * @return the instantiated model
+   */
+  protected abstract M fromModelMap(SolrResourceLoader loader, Map<String, Object> modelMap);


A static function cannot be declared as abstract

nicolo-rinaldi added 5 commits April 27, 2026 12:13

[text-to-vector-refactoring] Refactoring to accomodate future new fea…

2136650

…ture "Document enrichment with LLMs"

[text-to-vector-refactoring] Quick fixes ported from review of docume…

4c86f35

…nt enrichment module

[text-to-vector-refactoring] Quick fixes ported from review of docume…

86949b3

…nt enrichment module

Merge branch 'main' into text-to-vector-refactoring

54529af

[language-model-refactoring] Pre-check changes

11bb601

github-actions Bot added the tests label Apr 28, 2026

aruggero suggested changes Apr 29, 2026

View reviewed changes

nicolo-rinaldi commented Apr 29, 2026

View reviewed changes

nicolo-rinaldi added 2 commits April 29, 2026 15:45

[text-to-vector-refactoring] Addressing Anna's comments

56a4db4

[text-to-vector-refactoring] Addressing Anna's comments

de3328f

Conversation

nicolo-rinaldi commented Apr 28, 2026

Description

Solution

Tests

Checklist

Uh oh!

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!

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!

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!

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants