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 field type to query shape #130

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

Conversation

dzane17
Copy link
Collaborator

@dzane17 dzane17 commented Sep 26, 2024

Description

Add field type to query shape

Issues Resolved

Field type RFC #69

Testing

% curl -X POST "localhost:9200/my_index/_search?pretty" -H 'Content-Type: application/json' -d '{
  "query": {
    "range": {
      "age": {
        "gte": 25,
        "lte": 40
      }
    }
  },
  "aggs": {
    "average_age": {
      "avg": {
        "field": "age"
      }
    }
  },
  "sort": [
    {
      "name": {
        "order": "asc"
      }
    }
  ],
  "size": 100
}'
[2024-09-26T17:07:32,230][INFO ][stdout                   ] [integTest-0] ========== SEARCH SHAPE ==========
[2024-09-26T17:07:32,230][INFO ][stdout                   ] [integTest-0] range [age, integer]
[2024-09-26T17:07:32,231][INFO ][stdout                   ] [integTest-0] aggregation:
[2024-09-26T17:07:32,231][INFO ][stdout                   ] [integTest-0]   avg [age, integer]
[2024-09-26T17:07:32,231][INFO ][stdout                   ] [integTest-0] sort:
[2024-09-26T17:07:32,232][INFO ][stdout                   ] [integTest-0]   asc [name, keyword]

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

if (successfulShardIndices == null) {
successfulShardIndices = Collections.emptySet();
}
String hashcode = QueryShapeGenerator.getShapeHashCodeAsString(
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should have a knob to enable/disable field type in the query shape?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or do we plan to show this if fieldName enabled?

Copy link
Collaborator

@deshsidd deshsidd Sep 27, 2024

Choose a reason for hiding this comment

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

Do we want separate knobs for showFields and field type?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did not think of a good way to handle this. A new method param for each field data is not sustainable. Maybe a list of string constants or enum would work best.

Adding knobs is not complex. Given that showFields is currently disabled, I vote we implement knobs when we decide to turn on showFields. This way we can tailor it directly for our needs and not spend extra effort now.

if (mappingMetadata == null) {
continue;
}
Map<String, Object> sourceMap = mappingMetadata.getSourceAsMap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be null or not of expected format as a corner case?

Map<String, Object> sourceMap = mappingMetadata.getSourceAsMap();

String fieldType = findFieldType((Map<String, Object>) sourceMap.get("properties"), fieldName);
if (fieldType != null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

So we are just returning the first field type we find?
I thought we were planning to get field type for all indices and if the field type is the same then return it else consider it ambiguous and process it differently?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My method is assuming a fieldName will be mapped to the same fieldType in all indices a large majority of the time. Also note, we are only checking the index mapping of indices which were searched as part of this request.

We do not have any concrete data on this so I am open to either method. The current implementation saves some time and memory (very little).

if (entry.getKey().equals(fieldName)) {
return (String) field.get("type");
}
// TODO: Add support for multi-fields
Copy link
Collaborator

Choose a reason for hiding this comment

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

When can we add this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This will need to come in a future iteration. The challenge is that a single multi field can be queried with name, name.keyword, name.lowercase, name.initials with the example mapping below. Also non-multi fields can contain "." in the field name. So name.initials could be a multi field or just a normal field name.

{
  "mappings": {
    "properties": {
      "name": {
        "type": "text",
        "fields": {
          "keyword": {
            "type": "keyword",
            "ignore_above": 256
          },
          "lowercase": {
            "type": "text",
            "normalizer": "lowercase_normalizer"
          },
          "initials": {
            "type": "text",
            "fields": {
              "keyword": {
                "type": "keyword",
                "ignore_above": 256
              }
            }
          }
        }
      }
    }
  },
  "settings": {
    "analysis": {
      "normalizer": {
        "lowercase_normalizer": {
          "type": "custom",
          "filter": ["lowercase"]
        }
      }
    }
  }
}

Comment on lines +245 to +266
static String getFieldType(String fieldName, Metadata metadata, Set<String> searchIndices) {
if (metadata == null) {
return null;
}
for (String searchIndex : searchIndices) {
IndexMetadata indexMetadata = metadata.index(searchIndex);
if (indexMetadata == null) {
continue;
}
MappingMetadata mappingMetadata = indexMetadata.mapping();
if (mappingMetadata == null) {
continue;
}
Map<String, Object> sourceMap = mappingMetadata.getSourceAsMap();

String fieldType = findFieldType((Map<String, Object>) sourceMap.get("properties"), fieldName);
if (fieldType != null) {
return fieldType;
}
}
return null;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we inject the IndicesServices into QueryInsightsPlugin and do something like below:

MappedFieldType mappedFieldType = indicesService.indexService(index).mapperService().fieldType(fieldName)
if (mappedFieldType != null) {
    return mappedFieldType.typeName();
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

We might need to update results.getSuccessfulResults().map(result -> result.getSearchShardTarget().getIndex() to results.getSuccessfulResults().map(result -> result.getSearchShardTarget().getShardId().getIndex() for getting the Index object instead of String

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

QueryInsightsPlugin is instantiated in core in node.java with the createComponents() method. This method createComponents() overrides the Plugin/TelemetryAwarePlugin class. We will need to overload createComponents to add IndicesServices.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TelemetryAwarePlugin is already one example of overloading createComponents() to inject telemetry related classes to relevant plugins. We would need something similar.

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.

3 participants