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.

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

Add time_series_dimension property into _field_caps API method #5004

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kuzaxak
Copy link

@kuzaxak kuzaxak commented May 18, 2024

Description

time_series_dimension determine whether field will be used by Opensearch Dashboards to build histogram or not.

Fixing issue: #5003

How was this PR tested?

Added test to cover my change.

@CLAassistant
Copy link

CLAassistant commented May 18, 2024

CLA assistant check
All committers have signed the CLA.

`time_series_dimension` determine whether field will be used by Opensearch Dashboards to build histogram or not.

Fixing issue: quickwit-oss#5003
@kuzaxak kuzaxak force-pushed the add-time-series-dimension branch from 261e151 to bc8af38 Compare May 22, 2024 08:06
) -> Result<FieldCapabilityResponse, ElasticsearchError> {
let indexes_metadata = resolve_index_patterns(&index_id_patterns, &mut metastore).await?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let indexes_metadata = resolve_index_patterns(&index_id_patterns, &mut metastore).await?;
let indexes_metadata = resolve_index_patterns(&index_id_patterns, &mut metastore).await?;

I'm really not fond of doubling the calls to resolve_index_patterns.
We happen to do one in root_list_fields too.

It seems like we pre-build /cache the ListFieldsEntryResponse in the packager (and indexing time) so that root_list_fields(..) is a light operation.

It would make sense to add the bool property there, in the ListFieldsEntryResponse.

message ListFieldsEntryResponse {
  ...
  // True means that the field is the timestamp field of all of the requested indexes.
  bool is_timestamp = 8;
}

We then need to know how and when we populate it.

Solution 1:

The logical place would be the packager.
Pros: that's where we build the rest.
Cons: Right now the packager does not have access to the docmapper and does not know what the timestamp field is. We would have to pass that information. It is reasonable however.
Cons2: the data is not available for existing data.

Solution 2:

Keep it set to false in the packager, and actually compute it in the root_list_fields function.
At this point we have access to the index_metadatas already.
Pros: easy
Cons: ugly.

Copy link
Contributor

@fulmicoton fulmicoton left a comment

Choose a reason for hiding this comment

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

See comment inline. I don;'t think we want an extra call to the metastore here.

@kuzaxak
Copy link
Author

kuzaxak commented May 22, 2024

See comment inline. I don;'t think we want an extra call to the metastore here.

Clear, will have a look on weekend into the architecture of the service more deeply. Thanks for suggested solutions, will try to implement the nice one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants