[ML] add geo point combined field to CSV import #77117

nreese · 2020-09-09T21:21:41Z

Fixes #76996

This PR updates CSV upload to make it possible to combine two columns into geo_point field.

When ImportView is first rendered, a geo_point combined field will be added if there are latitude and longitude columns. This means that users will get geo_point field with no additional configuration.
Under the advanced tab, users can click Add combined field to configure a new combined field. In this initial PR, the only option is to add a geo point field but this can be expanded in the future to include other templates.

Whether auto-generated or manually added by users, geo point combined field does the following

Adds a new field in mappings with type geo_point
Adds a set processor to the ingest pipeline that creates the geo_point field by combining the latitude and longitude fields.

elasticmachine · 2020-09-09T21:21:43Z

Pinging @elastic/kibana-gis (Team:Geo)

elasticmachine · 2020-09-09T21:21:43Z

Pinging @elastic/ml-ui (:ml)

nreese · 2020-09-10T16:14:43Z

@elasticmachine merge upstream

jgowdyelastic · 2020-09-14T13:58:24Z

I've tested this and it works well.
I like the idea of writing this is a way that further combined field templates can be added later.
It would be good to hear feedback from @peteharverson, @sophiec20 and @droberts195 on this feature.

peteharverson · 2020-09-15T10:44:22Z

I tried giving this a test, but am seeing the following console errors when moving to the Import page. Are changes needed to this PR following the edits made in #77035?

Update: Sorry for the noise. My ES build was out of date and did not include the change for the "properties" layer in find_file_structure mappings.

peteharverson · 2020-09-15T11:54:29Z

Had a test and this is a great enhancement!

Some quick thoughts on the edits to the UI:

Whilst we only have the single 'geo point' option for 'combined fields' I wonder if we should just remove the 'combined fields' labelling, and just label it as 'geo point fields' until if or when we add templates for other combined field types. Clicking the 'Add geo point field' would then just directly open the edit popover:
There should be a way of deleting geo_point fields (other than by directly deleting them from the mapping JSON editor)
There is no attempt to keep the new UI controls in sync with edits made directly to Mappings or Ingest Pipeline editors. Would these new controls be better moved to the Simple tab?
Improve the messaging if the user tries to give a new geo_point field the same name as an existing field in the mapping:

nreese · 2020-09-15T15:45:55Z

@elasticmachine merge upstream

…pings

nreese · 2020-09-15T19:09:17Z

Whilst we only have the single 'geo point' option for 'combined fields' I wonder if we should just remove the 'combined fields' labelling, and just label it as 'geo point fields' until if or when we add templates for other combined field types

Users appreciate a consistent UI from release to release making it best to provide a stable UI that can grow with future needs. If there are known additional combined field templates that would later cause "geo point fields" to change to "combined fields", then that required UI churn can confusion users. Is there another known use case for combined fields? Should this PR just add a second template to flesh out the concept?

There should be a way of deleting geo_point fields (other than by directly deleting them from the mapping JSON editor)

This has been addressed and there is now a delete button for each combined field that will remove the combined field from the mappings and pipeline

There is no attempt to keep the new UI controls in sync with edits made directly to Mappings or Ingest Pipeline editors. Would these new controls be better moved to the Simple tab?

+1 on moving to the simple tab. I think the feature will be more visible to novice users from the simple tab. I think the feature is hidden on the advance tab since the main focus is on manually editing JSON configurations. I will defer to @@elastic/ml-ui for a final say. Please let me know where it should be placed.

Improve the messaging if the user tries to give a new geo_point field the same name as an existing field in the mapping:

This has been addressed and now name collision checking will look for collisions with other combined fields and any key in mappings.properties.

…_upload

nreese · 2020-09-17T19:52:40Z

@peteharverson

The Import page is hanging for me with a lot of log files - I guess when it doesn't find any candidates for combined fields?

This error is occurring for files that do not provide column_names in results. FindFileStructureResponse.column_names is typed as string[].

Is the bug that column_names is not provided or is the bug that FindFileStructureResponse.column_names should be typed as column_names?: string[];?

jgowdyelastic · 2020-09-17T20:35:06Z

Is the bug that column_names is not provided or is the bug that FindFileStructureResponse.column_names should be typed as column_names?: string[];?

Yes, column_names should be optional.

nreese · 2020-09-17T20:43:30Z

We have discussed this some more, and the proposal is to add a readonly display of the combined fields that have been auto-added to the Simple tab

I like this proposal and have added a readonly view in the simple tab

nreese · 2020-09-17T21:05:31Z

The Import page is hanging for me with a lot of log files - I guess when it doesn't find any candidates for combined fields?

This has been resolved. FindFileStructureResponse.column_names has been updated to reflect that column_names can be undefined and corresponding uses of column_names have been updated to handle column_names can be

...ins/ml/public/application/datavisualizer/file_based/components/combined_fields/geo_point.tsx

...gins/ml/public/application/datavisualizer/file_based/components/import_settings/advanced.tsx

jgowdyelastic · 2020-09-18T11:51:26Z

I'm able to get this feature into a confused state by adding a custom named combined field and then pressing reset after an import.

Delete the auto added combined field
Add a new one with a custom name e.g."foo"
Import the data
Press reset

foo still exists in the mappings and pipeline JSON but Latitude,Longitude => foo (geo_point) isn't listed in the combined fields list.
Instead the default name is listed (Latitude,Longitude => location (geo_point) but location isn't in the JSON.

…geo_point_csv_upload

nreese · 2020-09-18T13:56:53Z

I'm able to get this feature into a confused state by adding a custom named combined field and then pressing reset after an import.

Thanks for pointing out. I have updated the logic in getDefaultState to use state.combinedFields on reset

jgowdyelastic

This is a nice feature.
LGTM

peteharverson

Tested latest edits and LGTM.

kibanamachine · 2020-09-18T15:31:40Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 4a21384

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
ml	1409	+12	1397

async chunks size

id	value	diff	baseline
ml	8.1MB	+57.2KB	8.1MB

History

💚 Build #75536 succeeded 22ac2a1
💔 Build #75516 failed 335122e
💚 Build #75444 succeeded 98723df
💔 Build #75371 failed 12efff0
💚 Build #75380 succeeded 10e1a17

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

* [ML] add geo point combined field to CSV import * remove some geo_point specific logic * Account for properties layer in find_file_structure mappings * improve checking of name collision to include combined fields and mappings * add delete button * fix function name * fill in unknowns with defined types * tslint changes * get tslint passing * show readonly combined fields in simple tab * handle column_names being undefined * add unit tests for modifying mappings and pipeline * review feedback * do not change combinedFields on reset Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

nreese · 2020-09-18T16:24:08Z

7.x/7.10 #77921

* [ML] add geo point combined field to CSV import * remove some geo_point specific logic * Account for properties layer in find_file_structure mappings * improve checking of name collision to include combined fields and mappings * add delete button * fix function name * fill in unknowns with defined types * tslint changes * get tslint passing * show readonly combined fields in simple tab * handle column_names being undefined * add unit tests for modifying mappings and pipeline * review feedback * do not change combinedFields on reset Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

kmartastic · 2020-09-18T20:55:03Z

@nreese @peteharverson @jgowdyelastic

Was there ever any consideration to placing the UI for adding a combined field in the Advanced tab to be aligned with the "Mappings" section?

I relate new fields to mappings. Not sure if that's normal/expected.

RaniaElKatatny · 2022-02-28T01:54:00Z

In the geo point form, the dropdown lists of longitude and latitude are returning long fields only. I have longitude and latitude fields of type double but they are not appearing in the dropdown lists.
@nreese

nreese · 2022-02-28T16:22:48Z

In the geo point form, the dropdown lists of longitude and latitude are returning long fields only. I have longitude and latitude fields of type double but they are not appearing in the dropdown lists.

Would you mind moving this conversation to discuss? When you open a topic, please include a sample csv that is not displaying the lat/lon fields. Thanks.

[ML] add geo point combined field to CSV import

cccc3a2

nreese added release_note:enhancement WIP Work in progress [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation :ml v8.0.0 Feature:File and Index Data Viz ML file and index data visualizer v7.10.0 labels Sep 9, 2020

nreese requested a review from a team as a code owner September 9, 2020 21:21

nreese mentioned this pull request Sep 9, 2020

CSV upload - Indexing geo_point data from latitude and longitude columns #76996

Closed

remove some geo_point specific logic

e9eeb77

jgowdyelastic self-requested a review September 10, 2020 08:41

elasticmachine and others added 2 commits September 10, 2020 10:14

Merge branch 'master' into geo_point_csv_upload

a754bb1

Account for properties layer in find_file_structure mappings

31572f8

elasticmachine and others added 4 commits September 15, 2020 09:46

Merge branch 'master' into geo_point_csv_upload

dd2c5dd

improve checking of name collision to include combined fields and map…

d766b16

…pings

add delete button

933e5d8

fix function name

8155169

nreese added 4 commits September 15, 2020 15:51

fill in unknowns with defined types

d4e8809

Merge branch 'master' of github.com:elastic/kibana into geo_point_csv…

b545eaf

…_upload

tslint changes

41b09db

Merge branch 'master' of github.com:elastic/kibana into geo_point_csv…

1bc1553

…_upload

Merge branch 'master' into geo_point_csv_upload

cbc02d2

show readonly combined fields in simple tab

12efff0

handle column_names being undefined

10e1a17

Merge branch 'master' into geo_point_csv_upload

98723df

jgowdyelastic reviewed Sep 18, 2020

View reviewed changes

...ins/ml/public/application/datavisualizer/file_based/components/combined_fields/geo_point.tsx Outdated Show resolved Hide resolved

...gins/ml/public/application/datavisualizer/file_based/components/import_settings/advanced.tsx Show resolved Hide resolved

nreese added 4 commits September 18, 2020 06:29

add unit tests for modifying mappings and pipeline

f40f4c1

Merge branch 'geo_point_csv_upload' of github.com:nreese/kibana into …

335122e

…geo_point_csv_upload

review feedback

22ac2a1

do not change combinedFields on reset

4a21384

nreese requested review from jgowdyelastic and peteharverson September 18, 2020 13:56

nreese added review and removed WIP Work in progress labels Sep 18, 2020

jgowdyelastic approved these changes Sep 18, 2020

View reviewed changes

peteharverson approved these changes Sep 18, 2020

View reviewed changes

nreese merged commit f3c9d10 into elastic:master Sep 18, 2020

nreese mentioned this pull request Sep 18, 2020

[7.x] [ML] add geo point combined field to CSV import (#77117) #77921

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] add geo point combined field to CSV import #77117

[ML] add geo point combined field to CSV import #77117

nreese commented Sep 9, 2020

elasticmachine commented Sep 9, 2020

elasticmachine commented Sep 9, 2020

nreese commented Sep 10, 2020

jgowdyelastic commented Sep 14, 2020

peteharverson commented Sep 15, 2020 •

edited

Loading

peteharverson commented Sep 15, 2020

nreese commented Sep 15, 2020

nreese commented Sep 15, 2020

nreese commented Sep 17, 2020

jgowdyelastic commented Sep 17, 2020

nreese commented Sep 17, 2020

nreese commented Sep 17, 2020

jgowdyelastic commented Sep 18, 2020 •

edited

Loading

nreese commented Sep 18, 2020

jgowdyelastic left a comment

peteharverson left a comment

kibanamachine commented Sep 18, 2020

nreese commented Sep 18, 2020

kmartastic commented Sep 18, 2020

RaniaElKatatny commented Feb 28, 2022

nreese commented Feb 28, 2022

[ML] add geo point combined field to CSV import #77117

[ML] add geo point combined field to CSV import #77117

Conversation

nreese commented Sep 9, 2020

elasticmachine commented Sep 9, 2020

elasticmachine commented Sep 9, 2020

nreese commented Sep 10, 2020

jgowdyelastic commented Sep 14, 2020

peteharverson commented Sep 15, 2020 • edited Loading

peteharverson commented Sep 15, 2020

nreese commented Sep 15, 2020

nreese commented Sep 15, 2020

nreese commented Sep 17, 2020

jgowdyelastic commented Sep 17, 2020

nreese commented Sep 17, 2020

nreese commented Sep 17, 2020

jgowdyelastic commented Sep 18, 2020 • edited Loading

nreese commented Sep 18, 2020

jgowdyelastic left a comment

Choose a reason for hiding this comment

peteharverson left a comment

Choose a reason for hiding this comment

kibanamachine commented Sep 18, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

async chunks size

History

nreese commented Sep 18, 2020

kmartastic commented Sep 18, 2020

RaniaElKatatny commented Feb 28, 2022

nreese commented Feb 28, 2022

peteharverson commented Sep 15, 2020 •

edited

Loading

jgowdyelastic commented Sep 18, 2020 •

edited

Loading