Skip to content
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.

Sign up for GitHub

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

[Mappings editor] Add missing max_shingle_size parameter to search_as_you_type #55161

Merged
merged 4 commits into from
Jan 21, 2020
Merged

[Mappings editor] Add missing max_shingle_size parameter to search_as_you_type #55161

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b08e77a
Add "max_shingle_size" parameter so SearchAsYouType type
sebelga Jan 17, 2020
81e6580
Add parameter schema + update tests
sebelga Jan 20, 2020
25894c6
Merge branch 'master' into fix/add-missing-max_shingle_size-parameter
elasticmachine Jan 20, 2020
54cc9ca
Update "max_shingle_size" text description
sebelga Jan 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions ...ublic/app/components/mappings_editor/components/document_fields/field_parameters/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,3 +51,5 @@ export * from './fielddata_parameter';
export * from './split_queries_on_whitespace_parameter';

export * from './locale_parameter';

export * from './max_shingle_size_parameter';
45 changes: 45 additions & 0 deletions ...appings_editor/components/document_fields/field_parameters/max_shingle_size_parameter.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import React from 'react';

import { i18n } from '@kbn/i18n';

import { getFieldConfig } from '../../../lib';
import { EditFieldFormRow } from '../fields/edit_field';
import { UseField, Field } from '../../../shared_imports';

interface Props {
defaultToggleValue: boolean;
}

export const MaxShingleSizeParameter = ({ defaultToggleValue }: Props) => (
<EditFieldFormRow
title={i18n.translate('xpack.idxMgmt.mappingsEditor.maxShingleSizeFieldTitle', {
defaultMessage: 'Set max shingle size',
})}
description={i18n.translate('xpack.idxMgmt.mappingsEditor.maxShingleSizeFieldDescription', {
defaultMessage:
'The largest shingle size to index the input with and create subfields for, creating one subfield for each shingle size between 2 and the max value.',
Copy link
Contributor

Choose a reason for hiding this comment

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

I had a tricky time parsing this. What do you think of this copy Gail and I came up with?

The default is three shingle subfields. More subfields enables more specific queries, but increases index size.

Copy link
Contributor Author

@sebelga sebelga Jan 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think it is great to strive for improving the UI help texts, but we should align them with our docs. So one is not "better" (easier to understand) than the other. This is the current text for the parameter

Screen Shot 2020-01-20 at 12 18 55

Also, I am not sure we should add the default (hardcoded) value in the translation. That would be one more place to look for if it ever changes. If we think it adds value, then better to write it in parenthesis (Default: 3) and read the value from the parameter definition constant.

[EDIT] Just to be clear, I think it is great to improve the texts to explain a complex configuration parameter. But we should also make sure to port that effort to our docs if we've found an easier way to explain how a parameter behaves.

Copy link
Contributor

Choose a reason for hiding this comment

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

I love the idea of updating our docs. What do you propose? Would you like to incorporate the change we suggested, and we'll create an issue to update the docs accordingly?

I hear you in terms of keeping the text maintainable. We have hardcoded values in our help text in various places, so this isn't the only place we'll have this problem. How about we hardcode it for now, and create an issue for universally implementing a more robust solution like the one you suggest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I love the idea of updating our docs. What do you propose? Would you like to incorporate the change we suggested, and we'll create an issue to update the docs accordingly?

Yes let's do that 👍

Regarding the default value, I haven't looked in details where/when we started using default values in text description but I think we should try as much as possible to look how we did in previous places/forms and repeat the pattern (unless we decided that was a wrong idea).

This is how we did with follower indices and I think it was a great call

Screen Shot 2020-01-21 at 10 57 15

After we change the default:

Screen Shot 2020-01-21 at 10 57 35

})}
defaultToggleValue={defaultToggleValue}
>
<UseField
path="max_shingle_size"
component={Field}
config={getFieldConfig('max_shingle_size')}
componentProps={{
euiFieldProps: {
options: [
{ value: 2, text: '2' },
{ value: 3, text: '3' },
{ value: 4, text: '4' },
],
},
}}
/>
</EditFieldFormRow>
);
8 changes: 7 additions & 1 deletion ...ents/mappings_editor/components/document_fields/fields/field_types/search_as_you_type.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
NormsParameter,
SimilarityParameter,
TermVectorParameter,
MaxShingleSizeParameter,
} from '../../field_parameters';
import { BasicParametersSection, AdvancedParametersSection } from '../edit_field';

Expand All @@ -24,7 +25,8 @@ interface Props {
const getDefaultToggleValue = (param: string, field: FieldType) => {
switch (param) {
case 'similarity':
case 'term_vector': {
case 'term_vector':
case 'max_shingle_size': {
return field[param] !== undefined && field[param] !== getFieldConfig(param).defaultValue;
}
case 'analyzers': {
Expand All @@ -47,6 +49,10 @@ export const SearchAsYouType = React.memo(({ field }: Props) => {
<AdvancedParametersSection>
<AnalyzersParameter field={field} withSearchQuoteAnalyzer={true} />

<MaxShingleSizeParameter
defaultToggleValue={getDefaultToggleValue('max_shingle_size', field.source)}
/>

<NormsParameter />

<SimilarityParameter
Expand Down
10 changes: 10 additions & 0 deletions ...ndex_management/public/app/components/mappings_editor/constants/parameters_definition.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -894,4 +894,14 @@ export const PARAMETERS_DEFINITION = {
},
schema: t.string,
},
max_shingle_size: {
Copy link
Contributor

Choose a reason for hiding this comment

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

should we define a schema here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great catch, yes indeed we should 👍

fieldConfig: {
type: FIELD_TYPES.SELECT,
label: i18n.translate('xpack.idxMgmt.mappingsEditor.largestShingleSizeFieldLabel', {
defaultMessage: 'Max shingle size',
}),
defaultValue: 3,
formatters: [toInt],
},
},
};
3 changes: 2 additions & 1 deletion x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,8 @@ export type ParameterName =
| 'points_only'
| 'path'
| 'dims'
| 'depth_limit';
| 'depth_limit'
| 'max_shingle_size';

export interface Parameter {
fieldConfig: FieldConfig;
Expand Down