Repository navigation
[ML] Transforms: Support for terms agg in pivot configurations. - #123634
Conversation
|
Pinging @elastic/ml-ui (:ml) |
| } | ||
| isInvalid={!validSize} | ||
| > | ||
| <EuiFieldText defaultValue={sizeText} onChange={(e) => updateSize(e.target.value)} /> |
There was a problem hiding this comment.
size is a number type, shall we use https://elastic.github.io/eui/#/forms/form-controls#number-field control instead?
There was a problem hiding this comment.
I remember we intentionally went with the text field in other cases too because we had problems in the past with custom validations. So for consistency I went with the text based one here again. See the input for 'Maximum page search size' for example.
There was a problem hiding this comment.
Found EuiFieldNumber in
There was a problem hiding this comment.
Might be worth another look at EuiFieldNumber as currently it silently accepts fractional values (rounding down to nearest integer):
There was a problem hiding this comment.
Thanks for the additional feedback, I switched to EuiFieldNumber in 4878e18. Looking into this, I noticed the form validation approach I copied over from the percentiles variant was a bit flawed. I fixed that in the same commit. For both percentiles and size it's now validating the input string and keeping that as separate state. Previously, similar like the screenshot above with a float not triggering an error, for percentiles it also wouldn't show an error but just fall back to the default value when applied.
| const sampleDoc = sample(docs) ?? {}; | ||
| const missingMappings = difference(Object.keys(sampleDoc), Object.keys(populatedProperties)); | ||
| // Identify missing mappings | ||
| const missingMappings = difference( |
There was a problem hiding this comment.
The grid isn't inferring the type of the value correctly - it thinks it's a string, so left aligns the cell contents, and gives the alphabetical sort options (note sort isn't working on nested fields but I will raise a separate issue for that).
There was a problem hiding this comment.
It makes sense to address this as part of #123796 as even if the type for the terms agg columns is correctly set to numeric it still won't be sorted correctly as the column name will contain a dot.
peteharverson
left a comment
There was a problem hiding this comment.
Tested latest changes and LGTM.
We should look at the issues with the type and sorting of the preview columns in #123796.
|
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: cc @walterra |
…tic#123634) Adds support for the terms agg for transform pivot configurations.
Summary
Part of #123459.
Adds support for the
termsagg for transform pivot configurations.Checklist