Add 'lookup' as valid index mode for ES|QL lookup joins#936
Conversation
We'll need tests to validate this I think. I'm not sure if Fleet will just take any value from |
jsoriano
left a comment
jsoriano
left a comment
There was a problem hiding this comment.
We would need some test package to include this, ideally also in compliance, so this is tested with an actual kibana.
| - "lookup" # Enables ES|QL LOOKUP JOIN https://www.elastic.co/docs/reference/query-languages/esql/esql-lookup-join | ||
| examples: | ||
| - "time_series" | ||
| - "lookup" |
There was a problem hiding this comment.
We will need a patch below under versions to remove this from previous versions.
| ## This file documents changes in the package specification. It is NOT a package specification file. | ||
| ## Newer entries go at the bottom of each in-development version. | ||
| ## | ||
| - version: 3.6.0 |
There was a problem hiding this comment.
This change would release a 3.6.0 version, we need a -next suffix. Also, this change could be released in 3.4.2, or 3.5.0, depending if it needs explicit support in Fleet or not.
If this doesn't require explicit support in Fleet we could include this in 3.4.2-next. If it does, we could include it in 3.5.0-next.
There was a problem hiding this comment.
As I do not think it needs explicit Fleet support, I am going for 3.4.2-next
@jsoriano - this is my first change... How would I test against a real kibana? Can you guide me with this? I did all the changes as suggested/requested, and also added test data - which should work. |
Sure, now that you have a test package you could start by adding a feature file to Feature: ES|QL
Features related to ES|QL
@3.4.2
Scenario: Installer leverages lookup index mode
Given the "good_lookup_index" package is installed
And a policy is created with "good_lookup_index" package and "0.1.4" version
Then index template "good_lookup_index.foo" is configured for "lookup index mode"Then you would need to modify the If you want to try locally, you can run the compliance tests with elastic-package stack up -v -d --version 9.1.0-SNAPSHOT
eval $(elastic-package stack shellinit)
TEST_SPEC_FEATURES=features/esql.feature TEST_SPEC_VERSION=3.4.2 make -C compliance/ test |
jsoriano
left a comment
jsoriano
left a comment
There was a problem hiding this comment.
This is looking good, thanks for adding the test case.
There was a problem hiding this comment.
You would need to add this package to the list of packages in code/go/pkg/validator/validator_test.go, so it is actually tested.
|
I had to update the spec to add the index mode to I ran the test, and in the Kibana logs I see the package loaded, but it then tries to do stuff with I was digging into the Kibana logs, as I got this output from testing the feature: Inside Kibana I see the transform: 
I see the new index template: 
Would you be able to point me into the right direction for debugging this? |
|
Also see the actual index: And it is working as lookup index: 
|
| changes: | ||
| - description: Add support for lookup indices. | ||
| type: enhancement | ||
| link: https://github.com/elastic/package-spec/pull/936 |
There was a problem hiding this comment.
Please move it below 3.5.0-next.
| mode: | ||
| description: > | ||
| Mode of the index, can be `standard`, `time_series` or `lookup`. | ||
| The `lookup` mode is used for data streams that are used as lookup indices. | ||
| type: string | ||
| enum: | ||
| - standard | ||
| - time_series | ||
| - lookup |
There was a problem hiding this comment.
We have the other index_mode property as an indication to handle time series mode in a different way. Would it be possible to keep using this setting only?
| mode: | |
| description: > | |
| Mode of the index, can be `standard`, `time_series` or `lookup`. | |
| The `lookup` mode is used for data streams that are used as lookup indices. | |
| type: string | |
| enum: | |
| - standard | |
| - time_series | |
| - lookup |
| type: string | ||
| enum: | ||
| - "time_series" # Enables time series data streams https://www.elastic.co/guide/en/elasticsearch/reference/master/tsds.html | ||
| - "lookup" # Enables ES|QL LOOKUP JOIN https://www.elastic.co/docs/reference/query-languages/esql/esql-lookup-join |
There was a problem hiding this comment.
Could you test if setting the index mode with this setting works?
| settings: | ||
| index: | ||
| mode: "lookup" No newline at end of file |
There was a problem hiding this comment.
Is this feature only intended for transforms? If it is, we could maybe add this only to transforms. If it isn't could you add the setting to the data stream manifest?
…ntry - specific PR number.
…dex. Also, fix the versions in changelog, as requested.
Co-authored-by: Jaime Soriano Pastor <jaime.soriano@elastic.co>
JohannesMahne
force-pushed
the
add-lookup-index-mode
branch
from
8f9025e to
89fc4a4
Compare
…Removed the lookup config from streams definition, and added it to the elasticsearch_index_template definition. Updated the patch. In the transform spec I had to use the normal index_template but add the definition of the index.mode.
|
@jsoriano - I reviewed all your comments and was able to resolve most of them. The definition of the Can you please have another look? I also updated the feature flag, so the test are all going through well. |
There was a problem hiding this comment.
If this is only needed for transforms, we need to think how to add it without affecting data streams. Sadly I don't know how to do it without duplicating the definition for index templates 🤔
Also, we should not add another method to define time series data streams.
| description: Elasticsearch index template for the transform's destination index | ||
| $ref: "#/definitions/index_template" | ||
| required: false | ||
| allOf: |
There was a problem hiding this comment.
Not sure if this is really merging both schemas.
Co-authored-by: Jaime Soriano Pastor <jaime.soriano@elastic.co>
Co-authored-by: Jaime Soriano Pastor <jaime.soriano@elastic.co>
…dex. Also, fix the versions in changelog, as requested.
…earch.properties.index_mode. Data streams: properties.elasticsearch_index_template.properties.settings.properties.index.properties.mode.
|
@jsoriano - I added the lookup index mode only on the transform, as discussed in slack. |
…g_cluster_cost/transform.yml
…g_cluster_cost/fields/fields.yml
…g_cluster_cost/manifest.yml
jsoriano
left a comment
jsoriano
left a comment
There was a problem hiding this comment.
Thanks for the latest changes to limit changes to lookup indexes in transforms.
I think current approach is fine because transforms are already different to data streams, and we don't get into having to support TSDB in transform indexes.
To be clear, this leaves elastic/elastic-package#2829 behind, we will have to address this as a separate effort. cc @Coler-e.
Co-authored-by: Jaime Soriano Pastor <jaime.soriano@elastic.co>
|
@jsoriano - I tested removing |
💚 Build Succeeded
History
|
|
@jsoriano - Thank you for approving the PR. What is the process of getting this merged? I do not have authorization to do this. |
| type: object | ||
| additionalProperties: false | ||
| properties: | ||
| mode: |
There was a problem hiding this comment.
We should probably add a patch so this is not used on packages with a format version older than 3.4. I will do this in a follow up.
| ## | ||
| ## Describes the specification for a transform's manifest.yml file | ||
| ## | ||
| spec: |
There was a problem hiding this comment.
We have removed the spec level from this file, so we are effectively removing validation from here. Attempting to fix this in #964.
9 participants
What does this PR do?
It adds
lookupas a valid value for index mode, so that an index created by an integration can be a lookup index, to be used in a ES|QL LOOKUP JOIN.Why is it important?
Chargeback integration (Draft integration - PR raised) needs.
Checklist
test/packagesthat prove my change is effective.spec/changelog.yml.Related issues