Add multi-valued text support to the analyzer API #10847

johtani · 2015-04-28T05:55:16Z

Add support array text as a multi-valued for AnalyzeRequestBuilder and REST API

Add support array text as a multi-valued for AnalyzeRequestBuilder Add support array text as a multi-valued for Analyze REST API Add docs Closes elastic#3023

dakrone · 2015-04-29T14:35:41Z

docs/reference/indices/analyze.asciidoc

@@ -18,6 +18,19 @@ curl -XGET 'localhost:9200/_analyze' -d '

 coming[2.0.0, body based parameters were added in 2.0.0]

+If text parameter is provided as array of string, it analyze as a multi-valued.


I would change this:

If text parameter is provided as array of string, it analyze as a multi-valued.

to If text parameter is provided as array of strings, it is analyzed as a multi-valued field.

dakrone · 2015-04-29T15:03:24Z

Left some comments but generally looks good, thanks @johtani!

Fix some comments Closes elastic#3023

johtani · 2015-04-30T02:18:27Z

Thanks for your comments. Fixed. @dakrone

dakrone · 2015-05-01T15:44:57Z

LGTM

javanna · 2015-05-01T15:55:50Z

src/main/java/org/elasticsearch/client/IndicesAdminClient.java

+     * @param index The index name
+     * @param text  The array of text to analyze
+     */
+    AnalyzeRequestBuilder prepareAnalyze(@Nullable String index, String... text);


I am wondering if using varargs here makes sense, I think it makes these different methods ambigous?
what's the difference between client.prepareAnalyze("index", "text1"); and client.prepareAnalyze("text1", "text2"); and client.prepareAnalyze("text1");?

Maybe we should got for new methods that accept an array instead?

Good point

I think we should drop the prepareAnalyze(String... text) arity, and force specifying the index if you want to use the varargs analysis. What do you think?

I have another option.
Drop prepareAnalyze() with varargs which added this PR.
And add addText(String...) to AnalyzeRequestBuilder.java

Hmm, is it hard to understand?

@johtani but in that case users would have to specify the first value via prepareAnalyze and the following ones using addText ?

Along same lines as Lee's comment, how about having a single AnalyzeRequestBuilder prepareAnalyze(@Nullable String index, String... text); and drop all the others? It would be a breaking change though for the java api. Otherwise we should have the following without varargs which doesnt look great:

AnalyzeRequestBuilder prepareAnalyze(@Nullable String index, String text); AnalyzeRequestBuilder prepareAnalyze(@Nullable String index, String[] text); AnalyzeRequestBuilder prepareAnalyze(String text); AnalyzeRequestBuilder prepareAnalyze(String[] text);

I think compromises between the above two solutions won't work well.

@javanna Yeah, my option is not good...

I think Lee's comment is the following:

AnalyzeRequestBuilder prepareAnalyze(@Nullable String index, String text); AnalyzeRequestBuilder prepareAnalyze(@Nullable String index, String... text); AnalyzeRequestBuilder prepareAnalyze(String text);

or

AnalyzeRequestBuilder prepareAnalyze(@Nullable String index, String... text); AnalyzeRequestBuilder prepareAnalyze(String text);

however, I think it is easy to understand that we have the other name of method, like
prepareAnalyze and prepareAnalyzeForMultiValued

Change names of IndicesAdminClient methods Closes elastic#3023

johtani · 2015-05-02T05:11:41Z

I push changing names of methods with varargs.

javanna · 2015-05-02T07:37:18Z

src/main/java/org/elasticsearch/client/IndicesAdminClient.java

+     *
+     * @param text The array of text to analyze
+     */
+    AnalyzeRequestBuilder prepareAnalyzeWithMultiValued(String... text);


I think these two methods go a bit against the naming convention that we have here for prepare* methods. I really think we should go for one of the options I mentioned here. What do you think @dakrone ?

Oh, sorry, and I completely agree about the naming convention.
Other question.
If we can change I/F about prepareAnalyze, what do you think about dropping all text parameters for I/F simplification and adding the setter of text/texts to AnalyzeRequestBuilder?

Of course, we are hard to find the difference between prepareAnalyze(String text), that is old version, and prepareAnalyze(String index), that is new one.

what do you think about dropping all text parameters for I/F simplification and adding the setter of text/texts to AnalyzeRequestBuilder?

that is an option too. For now I'd just expose a prepareAnalyze() then with no parameters, leave the existing ones for bw comp, and add the needed setters where needed?

Now, AnalyzeRequestBuilder doesn't have methods related text parameters.
If we have prepareAnalyze(), we should setText(String)/setText(String...) to AnalyzeRequestBuilder.java

Remove bad naming method Add prepareAnalyze() with no params Add setText to AnalyzeRequestBuilder Closes elastic#3023

dakrone · 2015-05-14T17:02:23Z

LGTM, thanks @johtani!

javanna · 2015-05-15T07:37:15Z

src/main/java/org/elasticsearch/rest/action/admin/indices/analyze/RestAnalyzeAction.java

-                    analyzeRequest.text(text);
+                if (texts == null || texts.length == 0) {
+                    texts = new String[]{ RestActions.getRestContent(request).toUtf8() };
+                    analyzeRequest.text(texts);


question: we seem to support a single value only here. What is the difference between this case and the below case where we check the presence of arrays? It is just me not knowing tha analyze api I guess.

Good question.

Analyze API has three way to receive text.

request parameter

request body, raw text

request body, JSON parameter

This case is 2.
In this case, I think it is difficult which letter we assume a delimiter.
And we leave this case for backward compatibility.

Then, I think it's OK we don't support multi valued text for this case.

thanks for the explanation. case 2) is same as case 1) I guess. The only way we could make it work with multiple values would be by allowing for comma separated values, but then the comma coyuldn't be part of the text anymore. Let's leave it as-is.

javanna · 2015-05-15T08:08:49Z

left a couple more comments, mainly around bw comp but this is targeted for 2.0 so it shouldn't be a problem. LGTM!

remove setSource JSON Closes elastic#3023

johtani · 2015-05-15T11:21:26Z

Pushed

johtani · 2015-05-15T11:52:34Z

This PR breaks the java API.
This PR changes the return type of the text getter String[] instead of String in AnalyzeRequest.

johtani added >enhancement v2.0.0-beta1 review :Search/Analysis How text is split into tokens and removed review labels Apr 28, 2015

Analysis: Add multi-valued text support

55c0f5b

Add support array text as a multi-valued for AnalyzeRequestBuilder Add support array text as a multi-valued for Analyze REST API Add docs Closes elastic#3023

clintongormley added the review label Apr 28, 2015

johtani force-pushed the fix/add_support_text_array_analyze_api branch from 1d1f894 to 55c0f5b Compare April 28, 2015 10:56

johtani removed the in progress label Apr 28, 2015

dakrone reviewed Apr 29, 2015
View reviewed changes

Analysis: Add multi-valued text support

b471d19

Fix some comments Closes elastic#3023

javanna reviewed May 1, 2015
View reviewed changes

Analysis: Add multi-valued text support

91c7abd

Change names of IndicesAdminClient methods Closes elastic#3023

javanna reviewed May 2, 2015
View reviewed changes

Analysis: Add multi-valued text support

598f94c

Remove bad naming method Add prepareAnalyze() with no params Add setText to AnalyzeRequestBuilder Closes elastic#3023

javanna reviewed May 15, 2015
View reviewed changes

johtani added the >breaking label May 15, 2015

Analysis: Add multi-valued text support

139bd34

remove setSource JSON Closes elastic#3023

johtani closed this May 15, 2015

kevinkluge removed the review label May 15, 2015

clintongormley changed the title ~~Analysis: Add multi-valued text support~~ Add multi-valued text support Jun 6, 2015

clintongormley changed the title ~~Add multi-valued text support~~ Add multi-valued text support to the analyzer API Aug 13, 2015

johtani deleted the fix/add_support_text_array_analyze_api branch April 25, 2016 00:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add multi-valued text support to the analyzer API #10847

Add multi-valued text support to the analyzer API #10847

johtani commented Apr 28, 2015

dakrone Apr 29, 2015

dakrone commented Apr 29, 2015

johtani commented Apr 30, 2015

dakrone commented May 1, 2015

javanna May 1, 2015

dakrone May 1, 2015

johtani May 1, 2015

javanna May 1, 2015

johtani May 1, 2015

johtani commented May 2, 2015

javanna May 2, 2015

johtani May 4, 2015

javanna May 5, 2015

johtani May 8, 2015

dakrone commented May 14, 2015

javanna May 15, 2015

johtani May 15, 2015

javanna May 15, 2015

javanna commented May 15, 2015

johtani commented May 15, 2015

johtani commented May 15, 2015

		@@ -18,6 +18,19 @@ curl -XGET 'localhost:9200/_analyze' -d '

		coming[2.0.0, body based parameters were added in 2.0.0]

		If text parameter is provided as array of string, it analyze as a multi-valued.

Add multi-valued text support to the analyzer API #10847

Add multi-valued text support to the analyzer API #10847

Conversation

johtani commented Apr 28, 2015

Choose a reason for hiding this comment

dakrone commented Apr 29, 2015

johtani commented Apr 30, 2015

dakrone commented May 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johtani commented May 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dakrone commented May 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna commented May 15, 2015

johtani commented May 15, 2015

johtani commented May 15, 2015