removing check in TextToVectorUpdateProcessorFactory which brakes update for new dynamic fields #3426

renatoh · 2025-07-09T19:17:37Z

Using TextToVectorUpdateProcessorFactory with dynamics fields, the removed check causes an exception if no document with the input-field has been processes yet. The schema does not know the field yet.

TextToVectorUpdateProcessor#isNullOrEmpty is already checking for null and if the input field does not exist it will be skipped anyway, hence I think it is safe to remove the check.

The same could happen on the output field check , but it might be more likely, that the output field is a field and not a dymaicField, and with that will already exist on the schema

alessandrobenedetti · 2025-07-10T09:20:04Z

hi @renatoh , can you add a test that covers the scenario? I think your proposed change makes sense, but with the additional test coverage we'll be more confident

renatoh · 2025-07-10T19:27:48Z

.../org/apache/solr/llm/textvectorisation/update/processor/TextToVectorUpdateProcessorTest.java

@@ -40,6 +41,13 @@ public static void cleanup() throws Exception {
    afterTest();
  }

+  @After


the deletion within the test method will not happen if a test fails, such a failing test was then impacting all following test since the deletion was not done and the setup is done only once for the test-class

It makes sense, have you checked that all is fine? I remember I struggled a bit with the after/afterClass

yes, now with the @after, even if I make one test fail on purpose, the result is consistent across 5 test runs. Without the @after one failing test has unwanted side effect and other test of the class are randomly failing due to the data which got not cleaned up.

renatoh · 2025-07-10T19:31:26Z

.../modules/llm/src/java/org/apache/solr/llm/textvectorisation/model/SolrTextToVectorModel.java

+                      .getMethod(paramName, String.class)
+                      .invoke(builder, params.get(paramName).toString());
+                } catch (NoSuchMethodException e) {
+                  log.error("Parameter {} not supported by model {}", paramName, className);


Implementing my own DimensionAwareEmbeddingModel, I notice that if the attribute is not supported by the model, the error was not logged at all and I had to debug to see what the issue was.
I also took the liberty tot clean up the duplicate code snippets.

renatoh · 2025-07-10T19:35:44Z

Hi @alessandrobenedetti,

Thanks for your feedback! One of your test was actually failing, I adjusted it so that it no longer check for the exception.
The case where we the input fields it not present or empty is already covered well by TextToVectorUpdateProcessorTest.
Please also have a look at my comments on my changes

alessandrobenedetti

I think we are nearly there, the contributions make sense but the tests should get some additional attention

.../modules/llm/src/java/org/apache/solr/llm/textvectorisation/model/SolrTextToVectorModel.java

alessandrobenedetti · 2025-07-11T08:47:48Z

.../org/apache/solr/llm/textvectorisation/update/processor/TextToVectorUpdateProcessorTest.java

@@ -40,6 +41,13 @@ public static void cleanup() throws Exception {
    afterTest();
  }

+  @After


It makes sense, have you checked that all is fine? I remember I struggled a bit with the after/afterClass

alessandrobenedetti · 2025-07-11T08:49:21Z

...ache/solr/llm/textvectorisation/update/processor/TextToVectorUpdateProcessorFactoryTest.java

@@ -132,7 +132,7 @@ public void init_notDenseVectorOutputField_shouldThrowExceptionWithDetailedMessa

  /* Following test depends on a real solr schema and depends on BeforeClass-AfterClass methods */
  @Test
-  public void init_notExistentInputField_shouldThrowExceptionWithDetailedMessage() {
+  public void init_notExistentInputField_shouldNotThrowException() {


mmmm I think this was ok, if the field is undefined I expect the exception.
You should keep this test and add a new one specific for a dynamic field (the scope of this issue).
Did I misunderstand anything?

But this test will fail with the changes I did to TextToVectorUpdateProcessorFactory.java, since the exception is not thrown anymore. If we want to keep this test we need to distinguish somehow between a dynamic field and a non-dynamic field within TextToVectorUpdateProcessorFactory.java, in order to throw the exception only for dynamic fields, not sure if this would make sense

mmm, so what would happen if the user specifies in the input a non-existent field? For example, because of a typo?
Will it just silently fail?
If that's the case, we need to differentiate between non-existent and 'non explicitly defined but compatible with a dynamic field' I suspect

ok, I am checking now it the input or output field is a dynamic field and the exception is now still throw if the non-dynamic-field does not exists

alessandrobenedetti

The changes look good to me, I'll wait for a green build and add the changes.txt, merging afterwards. Thanks Renato!

alessandrobenedetti · 2025-07-14T10:57:30Z

Hi Renato, wanted to merge, but a bit in a rush now, Can you solve the conflict and push it again? (the annoying CHANGES.txt).

I'll merge after lunch, so in case you can't do it in the meantime, I'll do it later.

Cheers

renatoh · 2025-07-14T11:53:39Z

Hi Renato, wanted to merge, but a bit in a rush now, Can you solve the conflict and push it again? (the annoying CHANGES.txt).

I'll merge after lunch, so in case you can't do it in the meantime, I'll do it later.

Cheers

Hi Alessandro, only back at my computer on friday.
Ah, always the annoying CHANGES. txt

…ts for it

…ve to exist

…ate for new dynamic fields (#3426) * distinguish between dynamic field and field, dynamic fields do not have to exist --------- Co-authored-by: Alessandro Benedetti <[email protected]> (cherry picked from commit 8a28b91)

renatoh changed the title ~~removing check which brakes update with new dynamic field~~ removing check in TextToVectorUpdateProcessorFactory which brakes update for new dynamic fields Jul 9, 2025

github-actions bot added the tests label Jul 10, 2025

renatoh commented Jul 10, 2025

View reviewed changes

alessandrobenedetti requested changes Jul 11, 2025

View reviewed changes

alessandrobenedetti approved these changes Jul 14, 2025

View reviewed changes

renatoh and others added 7 commits July 14, 2025 14:45

removing check which brakes update with new dynamic field

f91f287

removing check which brakes update with new dynamic field, adjust tes…

285a6ef

…ts for it

clean up duplicate code segment; add log statement

5cec14e

distinguish between dynamic field and field, dynamic fields do not ha…

20b1174

…ve to exist

wrapping exception into SolrException

94d60a5

wrapping exception into SolrException

997376c

Update CHANGES.txt

a6a45e4

alessandrobenedetti force-pushed the error-handling-in-texttovecupdateprocessorfactory branch from 69156c6 to a6a45e4 Compare July 14, 2025 13:48

alessandrobenedetti merged commit 8a28b91 into apache:main Jul 14, 2025
3 checks passed

removing check in TextToVectorUpdateProcessorFactory which brakes update for new dynamic fields #3426

removing check in TextToVectorUpdateProcessorFactory which brakes update for new dynamic fields #3426

Uh oh!

Conversation

renatoh commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alessandrobenedetti commented Jul 10, 2025

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!

renatoh Jul 10, 2025 • 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!

renatoh commented Jul 10, 2025

Uh oh!

alessandrobenedetti 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!

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!

alessandrobenedetti left a comment

Choose a reason for hiding this comment

Uh oh!

alessandrobenedetti commented Jul 14, 2025

Uh oh!

renatoh commented Jul 14, 2025

Uh oh!

Uh oh!

Uh oh!

renatoh commented Jul 9, 2025 •

edited

Loading

renatoh Jul 10, 2025 •

edited

Loading