Skip to content

Conversation

@jrodewig
Copy link
Contributor

Reformats the condition token filter docs as part of #44726.

Changes

  • Adds a title abbreviation
  • Updates the description and adds a Lucene link
  • Separates the analyze and custom analyzer example snippets
  • Expands on the configurable parameters

@jrodewig jrodewig added >docs General docs changes :Search Relevance/Analysis How text is split into tokens v8.0.0 v7.5.0 v7.6.0 v7.4.3 labels Oct 31, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (:Search/Analysis)

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-docs (>docs)

Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jrodewig I left a few thoughts for clarifications, nothing major though. Would like to hear your opinion about them.

token.

Only inline scripts are supported. For valid parameters, see
<<_script_parameters>>.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure where this links to, did a quick grep in the es repo and couldn't dig it up but I suppose its a general painless script page. When looking at the code about which kind of scripts are allowed here I was suprised to see that this is a specific kind of script (in the code called AnalysisPredicateScript) that seems to be limited to work on certain token attributes (like length, position etc...) only. Maybe its worth mentioning that this only works on token properties (so looks like "token.*" would always be the first part in the chain). @jdconrad should be able to validate this assumption.

Copy link
Contributor Author

@jrodewig jrodewig Nov 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @cbuescher. For clarity, this currently links here:
https://www.elastic.co/guide/en/elasticsearch/reference/master/modules-scripting-using.html#_script_parameters

That content covers generic parameters for the script object. They aren't specific to Painless.

I think that link is still probably the best place to get those generic parameters, but I can also link to this page for Painless scripts used in this parameter:
https://www.elastic.co/guide/en/elasticsearch/painless/current/painless-analysis-predicate-context.html

Would that work for you @cbuescher @jdconrad? If only Painless scripts are allowed here, I can try another approach.

Copy link
Contributor Author

@jrodewig jrodewig Nov 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made these proposed changes in 4d127da and 77a5dd9 to make review a bit easier.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Script contexts are supposed to be generic, so I would prefer to try to document them as such. I don't have a strong opinion one way or another on a link to Painless docs.

"type": "condition",
"filter": [ "reverse" ],
"script": {
"source": "token.getPosition() === 0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a short comment after this example explaining what this analyzer does, its pretty obvious from the analyzer name but for some folks it might be worth spelling it out.

Copy link
Contributor Author

@jrodewig jrodewig Nov 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the comment! I was debating whether that was needed.

I added a description to the preceding paragraph with 3653233.

@jrodewig jrodewig requested a review from jdconrad November 7, 2019 19:04
@jrodewig jrodewig requested a review from cbuescher November 7, 2019 19:16
Copy link
Contributor

@jdconrad jdconrad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look good to me. Thanks for making them. Moving forward, I would like to talk about the best way to document script contexts in general. @rjernst and I had some thoughts on how we might be able to automate some of this.

@jrodewig
Copy link
Contributor Author

jrodewig commented Nov 7, 2019

Thanks for your review @jdconrad.

I agree that the documentation for script contexts could be improved, at least in how it's surfaced to users. I'd love to hear more about the ideas for automation.

Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification and the short explanation of the example, great read as always!

@jrodewig jrodewig merged commit 547f300 into elastic:master Nov 11, 2019
@jrodewig jrodewig deleted the reformat.condition-tokenfilter branch November 11, 2019 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants