-
Notifications
You must be signed in to change notification settings - Fork 127
314 minor fixes for guides #2371
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here for the exp-docs ✅ (which I guess happened because of the querydsl file? A couple quick suggestions from me, but I’ll leave the bulk of the review to the Core Docs team.
You can follow this guide using any {{es}} deployment. If you already have a deployment up and running, you can skip ahead to the [first step](#getting-started-index-creation). | ||
|
||
If not, refer to [choose your deployment type](/deploy-manage/deploy.md#choosing-your-deployment-type) for your options. To get started quickly, you can spin up a cluster [locally in Docker](get-started.md): | ||
You can follow this guide using any {{es}} deployment. If you have a deployment setup ready, skip ahead to the [first step](#getting-started-index-creation). If not, refer [choose your deployment type](/deploy-manage/deploy.md#choosing-your-deployment-type) to see all deployment options. To get started quickly, spin up a cluster [locally in Docker](run-elasticsearch-locally.md): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need setup here?
You can follow this guide using any {{es}} deployment. If you have a deployment setup ready, skip ahead to the [first step](#getting-started-index-creation). If not, refer [choose your deployment type](/deploy-manage/deploy.md#choosing-your-deployment-type) to see all deployment options. To get started quickly, spin up a cluster [locally in Docker](run-elasticsearch-locally.md): | |
You can follow this guide using any {{es}} deployment. If you have a deployment ready, skip ahead to the [first step](#getting-started-index-creation). If not, refer to [choose your deployment type](/deploy-manage/deploy.md#choosing-your-deployment-type) to see all deployment options. To get started quickly, spin up a cluster [locally in Docker](run-elasticsearch-locally.md): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bmorelli25 just wondering—how else would the user be able to run the first query if we don’t provide this setup guidance?
@@ -131,8 +125,6 @@ POST /_bulk | |||
{"name": "The Handmaids Tale", "author": "Margaret Atwood", "release_date": "1985-06-01", "page_count": 311} | |||
``` | |||
|
|||
You should receive a response indicating there were no errors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed you removed this response explanation but added explanations to responses below:
The following response displays the mappings that were created by Elasticsearch.
The following response indicates a successful operation.
Should we keep this explanation for consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're right @bmorelli25 , it was my mistake. Added it back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Thank you for these fixes. I left a few nits, please take or leave them. LGTM!
@@ -5,23 +5,21 @@ applies_to: | |||
navigation_title: Search and filter with ES|QL | |||
--- | |||
|
|||
# Tutorial: Search and filter with {{esql}} | |||
# Search and filter with {{esql}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider keeping "Tutorial" in the title to identify the purpose of the page more easily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@szabosteve Since these pages fall under Quickstarts, I removed the word "Tutorial" from the title to keep things consistent. Also, “tutorial” typically suggests guiding a user toward a specific end goal, whereas these pages are more about showing different ways to search. Let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
@@ -9,31 +9,31 @@ products: | |||
- id: elasticsearch | |||
--- | |||
|
|||
# Tutorial: Full-text search and filtering with Query DSL [full-text-filter-tutorial] | |||
# Full-text search and filtering with Query DSL [full-text-filter-tutorial] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above.
Co-authored-by: Brandon Morelli <[email protected]>
Co-authored-by: Brandon Morelli <[email protected]>
Co-authored-by: István Zoltán Szabó <[email protected]>
Co-authored-by: István Zoltán Szabó <[email protected]>
Co-authored-by: István Zoltán Szabó <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice cleanup, spotted couple minor things :)
1. The `standard` analyzer is used by default for `text` fields if an `analyzer` isn't specified. It's included here for demonstration purposes. | ||
2. [Multi-fields](elasticsearch://reference/elasticsearch/mapping-reference/multi-fields.md) are used here to index `text` fields as both `text` and `keyword` [data types](elasticsearch://reference/elasticsearch/mapping-reference/field-data-types.md). This enables both full-text search and exact matching/filtering on the same field. Note that if you used [dynamic mapping](../../manage-data/data-store/mapping/dynamic-field-mapping.md), these multi-fields would be created automatically. | ||
3. The [`ignore_above` parameter](elasticsearch://reference/elasticsearch/mapping-reference/ignore-above.md) prevents indexing values longer than 256 characters in the `keyword` field. Again this is the default value, but it's included here for demonstration purposes. It helps to save disk space and avoid potential issues with Lucene's term byte-length limit. | ||
1. `analyzer`: Used for text analysis. If you don't specify it, the `standard` analyzer is used by default for `text` fields. It’s included here for demonstration purposes. To know more about analyzers, refer [Anatomy of an analyzer](https://docs-v3-preview.elastic.dev/elastic/docs-content/tree/main/manage-data/data-store/text-analysis/anatomy-of-an-analyzer). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1. `analyzer`: Used for text analysis. If you don't specify it, the `standard` analyzer is used by default for `text` fields. It’s included here for demonstration purposes. To know more about analyzers, refer [Anatomy of an analyzer](https://docs-v3-preview.elastic.dev/elastic/docs-content/tree/main/manage-data/data-store/text-analysis/anatomy-of-an-analyzer). | |
1. `analyzer`: Used for text analysis. If you don't specify it, the `standard` analyzer is used by default for `text` fields. It’s included here for demonstration purposes. To know more about analyzers, refer to [Anatomy of an analyzer](https://docs-v3-preview.elastic.dev/elastic/docs-content/tree/main/manage-data/data-store/text-analysis/anatomy-of-an-analyzer). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I can commit this suggestion. However, could you help me understand something? In a few of my previous PRs, I was advised to change from 'Refer to' to just 'Refer'. How do we decide when to use one over the other? Is it based on how the sentence starts, or is there another guideline?
$$$filter-context$$$ | ||
|
||
$$$query-dsl-allow-expensive-queries$$$ | ||
|
||
$$$relevance-scores$$$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure we want to delete these, the syntax was used to maintain IDs in migration but if there isn't any CI errors, I guess it's OK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right. After I removed those, the broken links for those titles started working too. Let me know if we need to confirm about this from someone else too.
Co-authored-by: Liam Thompson <[email protected]>
Linked to https://github.com/elastic/developer-docs-team/issues/314 and includes fixes for API quickstarts:
Minor rephrasing of sentences
Shortened sentences
Fixes for broken links
Also, removed the redundant migrated content after the main header in
explore-analyze/query-filter/languages/querydsl.md