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

Jump to bottom

[data views] Remove regex for creating dot notation #193795

Merged
merged 1 commit into from
Sep 24, 2024
Merged

[data views] Remove regex for creating dot notation #193795

merged 1 commit into from
Sep 24, 2024

Conversation

mattkime
Copy link
Contributor

@mattkime mattkime commented Sep 23, 2024 •
edited by kibanamachine
Loading

Summary

Use basic string and array functions instead of regex to create short dot field names.

@mattkime
Copy link
Contributor Author

/ci

@mattkime mattkime self-assigned this Sep 23, 2024
@mattkime mattkime changed the title remove regex for creating dot notation [data views] Remove regex for creating dot notation Sep 23, 2024
@mattkime mattkime added Feature:Data Views Data Views code and UI - index patterns before 8.0 Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. backport:all-open Backport to all branches that could still receive a release release_note:skip Skip the PR/issue when compiling release notes labels Sep 23, 2024
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Jest Tests #1 / BadArgument component should display message and help output if command is not hidden from help
  • [job] [logs] Jest Tests #1 / BadArgument component should only display message (no help) if command is hidden from help
  • [job] [logs] Jest Tests #5 / Timeline rendering should trim kqlQueryExpression

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
dataViews 62.2KB 62.3KB +67.0B

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @mattkime

@mattkime mattkime marked this pull request as ready for review September 23, 2024 23:16
@mattkime mattkime requested a review from a team as a code owner September 23, 2024 23:16
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

lukasolson
lukasolson approved these changes Sep 23, 2024
View reviewed changes
Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

LGTM! Did a little benchmark and this is much more performant:

image

@mattkime
Copy link
Contributor Author

@lukasolson
3rVfBUa9f0RErtMZBH

@mattkime mattkime merged commit ab9459c into elastic:main Sep 24, 2024
31 checks passed
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Sep 24, 2024
@mattkime
[data views] Remove regex for creating dot notation (elastic#193795)
618cf67 
## Summary

Use basic string and array functions instead of regex to create short
dot field names.

(cherry picked from commit ab9459c)
@kibanamachine kibanamachine mentioned this pull request Sep 24, 2024
Merged
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Sep 24, 2024
@mattkime
[data views] Remove regex for creating dot notation (elastic#193795)
2a88a1f 
## Summary

Use basic string and array functions instead of regex to create short
dot field names.

(cherry picked from commit ab9459c)
@kibanamachine kibanamachine mentioned this pull request Sep 24, 2024
Merged
@kibanamachine
Copy link
Contributor

💔 Some backports could not be created

Status Branch Result
❌ 7.17 Backport failed because of merge conflicts
✅ 8.15
✅ 8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Manual backport

To create the backport manually run:

node scripts/backport --pr 193795

Questions ?

Please refer to the Backport tool documentation

@mattkime mattkime mentioned this pull request Sep 24, 2024
Merged
@mattkime
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
✅ 7.17

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

mattkime added a commit to mattkime/kibana that referenced this pull request Sep 24, 2024
@mattkime
[data views] Remove regex for creating dot notation (elastic#193795)
cbc3ece 
## Summary

Use basic string and array functions instead of regex to create short
dot field names.

(cherry picked from commit ab9459c)

# Conflicts:
#	src/plugins/data_views/common/fields/utils.ts
kibanamachine added a commit that referenced this pull request Sep 24, 2024
@kibanamachine @mattkime
[8.15] [data views] Remove regex for creating dot notation (#193795) (#…
e290e90 
…193808)

# Backport

This will backport the following commits from `main` to `8.15`:
- [[data views] Remove regex for creating dot notation
(#193795)](#193795)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Matthew
Kime","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-09-24T00:30:52Z","message":"[data
views] Remove regex for creating dot notation (#193795)\n\n##
Summary\r\n\r\nUse basic string and array functions instead of regex to
create short\r\ndot field
names.","sha":"ab9459ca4358451614c3b4fb09380af9841e7a66","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Data
Views","release_note:skip","v9.0.0","Team:DataDiscovery","backport:all-open"],"title":"[data
views] Remove regex for creating dot
notation","number":193795,"url":"https://github.com/elastic/kibana/pull/193795","mergeCommit":{"message":"[data
views] Remove regex for creating dot notation (#193795)\n\n##
Summary\r\n\r\nUse basic string and array functions instead of regex to
create short\r\ndot field
names.","sha":"ab9459ca4358451614c3b4fb09380af9841e7a66"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/193795","number":193795,"mergeCommit":{"message":"[data
views] Remove regex for creating dot notation (#193795)\n\n##
Summary\r\n\r\nUse basic string and array functions instead of regex to
create short\r\ndot field
names.","sha":"ab9459ca4358451614c3b4fb09380af9841e7a66"}}]}]
BACKPORT-->

Co-authored-by: Matthew Kime <[email protected]>
kibanamachine added a commit that referenced this pull request Sep 24, 2024
@kibanamachine @mattkime
[8.x] [data views] Remove regex for creating dot notation (#193795) (#…
87ac87a 
…193809)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[data views] Remove regex for creating dot notation
(#193795)](#193795)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Matthew
Kime","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-09-24T00:30:52Z","message":"[data
views] Remove regex for creating dot notation (#193795)\n\n##
Summary\r\n\r\nUse basic string and array functions instead of regex to
create short\r\ndot field
names.","sha":"ab9459ca4358451614c3b4fb09380af9841e7a66","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Data
Views","release_note:skip","v9.0.0","Team:DataDiscovery","backport:all-open"],"title":"[data
views] Remove regex for creating dot
notation","number":193795,"url":"https://github.com/elastic/kibana/pull/193795","mergeCommit":{"message":"[data
views] Remove regex for creating dot notation (#193795)\n\n##
Summary\r\n\r\nUse basic string and array functions instead of regex to
create short\r\ndot field
names.","sha":"ab9459ca4358451614c3b4fb09380af9841e7a66"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/193795","number":193795,"mergeCommit":{"message":"[data
views] Remove regex for creating dot notation (#193795)\n\n##
Summary\r\n\r\nUse basic string and array functions instead of regex to
create short\r\ndot field
names.","sha":"ab9459ca4358451614c3b4fb09380af9841e7a66"}}]}]
BACKPORT-->

Co-authored-by: Matthew Kime <[email protected]>
jughosta
jughosta reviewed Sep 24, 2024
View reviewed changes
src/plugins/data_views/common/fields/utils.ts
/**
* Convert a dot.notated.string into a short
* version (d.n.string)
*/
export function shortenDottedString(input: string): string {
return typeof input !== 'string' ? input : input.replace(DOT_PREFIX_RE, '$1.');
if (typeof input === 'string') {
const split = input.split('.');
Copy link
Contributor

@jughosta jughosta Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can some parts be empty? For example, can input be something like foo..bar.foo?

Copy link
Contributor Author

@mattkime mattkime Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

yes, field names can be anything. having two dots in a row would result in f.undefined.b.foo

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 address undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#193874

Copy link
Member

Choose a reason for hiding this comment

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

Just FYI I actually tried ingesting a document like foo..bar.foo and I got an error:

POST tmp-00001/_doc
{
  "asdf..basdf": "bar"
}

// Results in 
{
  "error": {
    "root_cause": [
      {
        "type": "document_parsing_exception",
        "reason": "[1:4] failed to parse: field name cannot contain only whitespace: ['asdf..basdf']"
      }
    ],
    "type": "document_parsing_exception",
    "reason": "[1:4] failed to parse: field name cannot contain only whitespace: ['asdf..basdf']",
    "caused_by": {
      "type": "illegal_argument_exception",
      "reason": "field name cannot contain only whitespace: ['asdf..basdf']"
    }
  },
  "status": 400
}

Copy link
Contributor Author

@mattkime mattkime Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

huh, I stand corrected. I swear this must be the only limitation on field names. Might be best to talk to an ES engineer.

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'll skip the follow up until someone can produce a field name with two dots in a row

@kertal
Copy link
Member

kertal commented Sep 24, 2024

LGTM! Did a little benchmark and this is much more performant:

image

nice, @lukasolson what do you use for this performance check?

mattkime added a commit that referenced this pull request Sep 24, 2024
@mattkime
[7.17] [data views] Remove regex for creating dot notation (#193795) (#…
a383439 
…193811)

# Backport

This will backport the following commits from `main` to `7.17`:
- [[data views] Remove regex for creating dot notation
(#193795)](#193795)

<!--- Backport version: 8.9.8 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Matthew
Kime","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-09-24T00:30:52Z","message":"[data
views] Remove regex for creating dot notation (#193795)\n\n##
Summary\r\n\r\nUse basic string and array functions instead of regex to
create short\r\ndot field
names.","sha":"ab9459ca4358451614c3b4fb09380af9841e7a66","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Data
Views","release_note:skip","v9.0.0","Team:DataDiscovery","backport:all-open"],"number":193795,"url":"https://github.com/elastic/kibana/pull/193795","mergeCommit":{"message":"[data
views] Remove regex for creating dot notation (#193795)\n\n##
Summary\r\n\r\nUse basic string and array functions instead of regex to
create short\r\ndot field
names.","sha":"ab9459ca4358451614c3b4fb09380af9841e7a66"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","labelRegex":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/193795","number":193795,"mergeCommit":{"message":"[data
views] Remove regex for creating dot notation (#193795)\n\n##
Summary\r\n\r\nUse basic string and array functions instead of regex to
create short\r\ndot field
names.","sha":"ab9459ca4358451614c3b4fb09380af9841e7a66"}},{"url":"https://github.com/elastic/kibana/pull/193808","number":193808,"branch":"8.15","state":"OPEN"},{"url":"https://github.com/elastic/kibana/pull/193809","number":193809,"branch":"8.x","state":"OPEN"}]}]
BACKPORT-->
@lukasolson
Copy link
Member

what do you use for this performance check?

https://jsbench.me/ - did a comparison with some simple strings as well as fairly long ones

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jughosta jughosta jughosta left review comments

@lukasolson lukasolson lukasolson approved these changes

Assignees

@mattkime mattkime

Labels
backport:all-open Backport to all branches that could still receive a release Feature:Data Views Data Views code and UI - index patterns before 8.0 release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. v7.17.25 v8.15.2 v8.16.0 v9.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@mattkime @kibana-ci @elasticmachine @kibanamachine @kertal @lukasolson @jughosta