[ES|QL] [Lens] Field picker problems

[mbondyra]

Kibana version: 8.15, main

When adding esql support for Lens, we’ve decided that columnIds of lens columns will be taken from the ‘fieldnames’ taken from esql query. This is an example of a column for esql layer:

{
    "columnId": "BUCKET(@timestamp, 1 day)",
    "fieldName": "BUCKET(@timestamp, 1 day)",
    "meta": {
        "type": "date",
        "esType": "date"
    }
}

Unfortunately, columnId having another meaning other than just being an id for a dimension (used to identify its properties) makes it easy to write some errors in the code (some already exist or some can happen in the future). The example of these bugs include:

Drag and drop bug

https://github.com/elastic/kibana/issues/185636 (it is actually fixed already ‘by mistake’, here: https://github.com/elastic/kibana/pull/187142/files) - it happens because drag and drop replaces all the columns content except for columnIds that stay in place. As we use these ids to link some of the properties from cache or activeData, they are linked to the wrong columns.

Field picker bug 1

When choosing another field for the existing dimension, the field picker doesn't properly identify the column. Here’s how to reproduce it.

a. Create an esql chart from this command:

    from kibana_sample_data_logs
    | stats total_bytes = sum(bytes) by extension, bytes % 5

b. open extension dimension. c. change to bytes. It works as expected, but under the hood, the state of this column is changed to:

{
    "columnId": "extension",
    "fieldName": "bytes % 5",
    "meta": {
        "type": "number",
        "esType": "long"
    }
}

d. Intend to switch back to extension - it will never switch because it already assumes the content is up to date, as columnId is set on extension.

Field picker bug 2

If we create an extra column (not the ones genereted on running the query), they get duplicated when they should be overwriten. a. Create an esql chart from this command:

      from kibana_sample_data_logs
      | stats total_bytes = sum(bytes) by extension, bytes % 5

b. remove extension dimension c. click on horizontal axis and add extension dimension by hand - it will get uuid, not the fieldname id. d. Now, everytime you click on the extension id to choose it from the field picker again (over and over again) it will just add a new column with the same uuid.

This can cause more problems in the future and can linger with us when adding more features so it would be good to fix it asap.

[elasticmachine]

Pinging @elastic/kibana-esql (Team:ESQL)

[elasticmachine]

Pinging @elastic/kibana-visualizations (Team:Visualizations)

[stratoula]

@mbondyra this logic seems to be in Lens. no?

[mbondyra]

@mbondyra this logic seems to be in Lens. no?

All this code exists inside Lens directory, but I'll assume you mean Lens as editor frame and common Lens code and esql as text-based folder (code added to integrate esql inside Lens) Adding an id for a new column when you click on the empty dimension is in Lens, but that works correctly. What doesn't work correctly is:

1. Mapping these ids to how you look up the the esql fieldnames to get their properties (it's not Lens, it's how esql integrates with Lens)
2. When we run esql query, the new columns don't get uuids, they get ids based on the fieldnames - it's how esql internally works.

I think it will be easier to understand the technical problem when you reproduce the bugs.

[stratoula]

Yes thanx, I mean if it is on code related to the ES|QL team (which owns the editor and the various utilities). The bug exists in the textbased datasource of Lens so I changed the labels accordingly.

I managed to reproduce the bugs thanx for the detailed examples. I am not very sure what you are suggesting though. To use uuids instead of the name of the column? Because this is how Lens works? Do I get it correctly?

What does it mean from the compatibility perspective? And why using the uuids will solve the problem?

[mbondyra]

To use uuids instead of the name of the column? Because this is how Lens works? Do I get it correctly?

Why it is important to use uuids in Lens and not ids based on some fields properties:

This is because Lens was designed with the concept that column IDs should be interchangeable.

• You should be able to modify any column properties, like fields, and the ID of the column doesn't change so fieldname as an id doesn't make sense
• In some parts of the code, ID can change while the content remains the same

Using field names as IDs doesn't make sense since it can confuse anyone analyzing the saved object or writing code based on it (and we can see that it did, for example when I was developing dnd for esql). We also refered to the ID to get the information from the cache matching it with the fieldname, but we should not because it can refer already to another field. Not using the uuids encourages that it can be done when it fact it can be buggy.

Although not using uuids isn't technically a bug, it has caused several issues already and isn't a good practice, given how Lens is written.

When it comes to compatibility, it should work fine as id is just an id – using uuids is just the best option but we can still have a 'bytes' column that has an id 'memory' in the saved saved objects columns and it would work. We want to use uuids to break the connection that shouldn't be there, but not to fix the issues (they have to be fixed on their own, if there are any)

[stratoula]

The aforementioned bugs are due to not setting correctly the selected field when I did the refactoring to fix the performance issue. They are fixed here https://github.com/elastic/kibana/pull/188993

I think that there is no huge gain on using uuids instead of the column names especially because we want to map ES|QL columns to chart dimensions and not do something more sophisticated as we do in the dataview mode. So in the dataview mode you might have a duplicated column (for example if you want to shift) but in ES|QL it will never be like that because you are creating these columns in the query which means an new column in the table.

But I leave this to you, if you want to refactor to use uuids is ok by me as soon as we dont create backwards compatibility problems. I think for now my PR fixes the bugs and we are good to go.