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.

Sign up for GitHub

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

Jump to bottom

Add field type to query shape #130

Closed
wants to merge 1 commit into from
Closed

Add field type to query shape #130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/main/java/org/opensearch/plugin/insights/core/listener/QueryInsightsListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
Expand Down Expand Up @@ -239,7 +240,16 @@ private void constructSearchQueryRecord(final SearchPhaseContext context, final
);
}

String hashcode = QueryShapeGenerator.getShapeHashCodeAsString(request.source(), false);
Set<String> successfulShardIndices = searchRequestContext.getSuccessfulSearchShardIndices();
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 •
edited
Loading

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.

request.source(),
false,
clusterService.state().getMetadata(),
successfulShardIndices
);

Map<Attribute, Object> attributes = new HashMap<>();
attributes.put(Attribute.SEARCH_TYPE, request.searchType().toString().toLowerCase(Locale.ROOT));
Expand Down
117 changes: 98 additions & 19 deletions ...ain/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,12 @@
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Set;
import org.apache.lucene.util.BytesRef;
import org.opensearch.cluster.metadata.IndexMetadata;
import org.opensearch.cluster.metadata.MappingMetadata;
import org.opensearch.cluster.metadata.Metadata;
import org.opensearch.common.hash.MurmurHash3;
import org.opensearch.core.common.io.stream.NamedWriteable;
import org.opensearch.index.query.QueryBuilder;
Expand All @@ -34,16 +39,28 @@ public class QueryShapeGenerator {
* Method to get query shape hash code given a source
* @param source search request source
* @param showFields whether to include field data in query shape
* @param metadata contains index mappings
* @param searchIndices successful indices searched
* @return Hash code of query shape as a MurmurHash3.Hash128 object (128-bit)
*/
public static MurmurHash3.Hash128 getShapeHashCode(SearchSourceBuilder source, Boolean showFields) {
final String shape = buildShape(source, showFields);
public static MurmurHash3.Hash128 getShapeHashCode(
SearchSourceBuilder source,
Boolean showFields,
Metadata metadata,
Set<String> searchIndices
) {
final String shape = buildShape(source, showFields, metadata, searchIndices);
final BytesRef shapeBytes = new BytesRef(shape);
return MurmurHash3.hash128(shapeBytes.bytes, 0, shapeBytes.length, 0, new MurmurHash3.Hash128());
}

public static String getShapeHashCodeAsString(SearchSourceBuilder source, Boolean showFields) {
MurmurHash3.Hash128 hashcode = getShapeHashCode(source, showFields);
public static String getShapeHashCodeAsString(
SearchSourceBuilder source,
Boolean showFields,
Metadata metadata,
Set<String> searchIndices
) {
MurmurHash3.Hash128 hashcode = getShapeHashCode(source, showFields, metadata, searchIndices);
String hashAsString = Long.toHexString(hashcode.h1) + Long.toHexString(hashcode.h2);
return hashAsString;
}
Expand All @@ -52,13 +69,15 @@ public static String getShapeHashCodeAsString(SearchSourceBuilder source, Boolea
* Method to build search query shape given a source
* @param source search request source
* @param showFields whether to append field data
* @param metadata contains index mappings
* @param searchIndices successful indices searched
* @return Search query shape as String
*/
public static String buildShape(SearchSourceBuilder source, Boolean showFields) {
public static String buildShape(SearchSourceBuilder source, Boolean showFields, Metadata metadata, Set<String> searchIndices) {
StringBuilder shape = new StringBuilder();
shape.append(buildQueryShape(source.query(), showFields));
shape.append(buildAggregationShape(source.aggregations(), showFields));
shape.append(buildSortShape(source.sorts(), showFields));
shape.append(buildQueryShape(source.query(), showFields, metadata, searchIndices));
shape.append(buildAggregationShape(source.aggregations(), showFields, metadata, searchIndices));
shape.append(buildSortShape(source.sorts(), showFields, metadata, searchIndices));
return shape.toString();
}

Expand All @@ -68,11 +87,11 @@ public static String buildShape(SearchSourceBuilder source, Boolean showFields)
* @param showFields whether to append field data
* @return Query-section shape as String
*/
static String buildQueryShape(QueryBuilder queryBuilder, Boolean showFields) {
static String buildQueryShape(QueryBuilder queryBuilder, Boolean showFields, Metadata metadata, Set<String> searchIndices) {
if (queryBuilder == null) {
return EMPTY_STRING;
}
QueryShapeVisitor shapeVisitor = new QueryShapeVisitor();
QueryShapeVisitor shapeVisitor = new QueryShapeVisitor(metadata, searchIndices);
queryBuilder.visit(shapeVisitor);
return shapeVisitor.prettyPrintTree(EMPTY_STRING, showFields);
}
Expand All @@ -83,7 +102,12 @@ static String buildQueryShape(QueryBuilder queryBuilder, Boolean showFields) {
* @param showFields whether to append field data
* @return Aggregation shape as String
*/
static String buildAggregationShape(AggregatorFactories.Builder aggregationsBuilder, Boolean showFields) {
static String buildAggregationShape(
AggregatorFactories.Builder aggregationsBuilder,
Boolean showFields,
Metadata metadata,
Set<String> searchIndices
) {
if (aggregationsBuilder == null) {
return EMPTY_STRING;
}
Expand All @@ -92,7 +116,9 @@ static String buildAggregationShape(AggregatorFactories.Builder aggregationsBuil
aggregationsBuilder.getPipelineAggregatorFactories(),
new StringBuilder(),
new StringBuilder(),
showFields
showFields,
metadata,
searchIndices
);
return aggregationShape.toString();
}
Expand All @@ -102,7 +128,9 @@ static StringBuilder recursiveAggregationShapeBuilder(
Collection<PipelineAggregationBuilder> pipelineAggregations,
StringBuilder outputBuilder,
StringBuilder baseIndent,
Boolean showFields
Boolean showFields,
Metadata metadata,
Set<String> searchIndices
) {
//// Normal Aggregations ////
if (aggregationBuilders.isEmpty() == false) {
Expand All @@ -113,7 +141,7 @@ static StringBuilder recursiveAggregationShapeBuilder(
StringBuilder stringBuilder = new StringBuilder();
stringBuilder.append(baseIndent).append(ONE_SPACE_INDENT.repeat(2)).append(aggBuilder.getType());
if (showFields) {
stringBuilder.append(buildFieldDataString(aggBuilder));
stringBuilder.append(buildFieldDataString(aggBuilder, metadata, searchIndices));
}
stringBuilder.append("\n");

Expand All @@ -124,7 +152,9 @@ static StringBuilder recursiveAggregationShapeBuilder(
aggBuilder.getPipelineAggregations(),
stringBuilder,
baseIndent.append(ONE_SPACE_INDENT.repeat(4)),
showFields
showFields,
metadata,
searchIndices
);
baseIndent.delete(0, 4);
}
Expand Down Expand Up @@ -167,7 +197,7 @@ static StringBuilder recursiveAggregationShapeBuilder(
* @param showFields whether to append field data
* @return Sort shape as String
*/
static String buildSortShape(List<SortBuilder<?>> sortBuilderList, Boolean showFields) {
static String buildSortShape(List<SortBuilder<?>> sortBuilderList, Boolean showFields, Metadata metadata, Set<String> searchIndices) {
if (sortBuilderList == null || sortBuilderList.isEmpty()) {
return EMPTY_STRING;
}
Expand All @@ -179,7 +209,7 @@ static String buildSortShape(List<SortBuilder<?>> sortBuilderList, Boolean showF
StringBuilder stringBuilder = new StringBuilder();
stringBuilder.append(ONE_SPACE_INDENT.repeat(2)).append(sortBuilder.order());
if (showFields) {
stringBuilder.append(buildFieldDataString(sortBuilder));
stringBuilder.append(buildFieldDataString(sortBuilder, metadata, searchIndices));
}
shapeStrings.add(stringBuilder.toString());
}
Expand All @@ -195,11 +225,60 @@ static String buildSortShape(List<SortBuilder<?>> sortBuilderList, Boolean showF
* @return String: comma separated list with leading space in square brackets
* Ex: " [my_field, width:5]"
*/
static String buildFieldDataString(NamedWriteable builder) {
static String buildFieldDataString(NamedWriteable builder, Metadata metadata, Set<String> searchIndices) {
List<String> fieldDataList = new ArrayList<>();
if (builder instanceof WithFieldName) {
fieldDataList.add(((WithFieldName) builder).fieldName());
String fieldName = ((WithFieldName) builder).fieldName();
fieldDataList.add(fieldName);
String fieldType = getFieldType(fieldName, metadata, searchIndices);
if (fieldType != null) {
fieldDataList.add(fieldType);
}
}
return " [" + String.join(", ", fieldDataList) + "]";
}

/**
* Method to get a field's type
* @return String field type
*/
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();
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?


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).

return fieldType;
}
}
return null;
}
Comment on lines +245 to +266
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.


public static String findFieldType(Map<String, Object> properties, String fieldName) {
for (Map.Entry<String, Object> entry : properties.entrySet()) {
Map<String, Object> field = (Map<String, Object>) entry.getValue();
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"]
        }
      }
    }
  }
}

// if (field.containsKey("fields")) {
// String type = findFieldType((Map<String, Object>) field.get("fields"), fieldName);
// if (type != null) {
// return type;
// }
// }
}
return null;
}
}
23 changes: 16 additions & 7 deletions src/main/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Set;
import org.apache.lucene.search.BooleanClause;
import org.opensearch.cluster.metadata.Metadata;
import org.opensearch.common.SetOnce;
import org.opensearch.index.query.QueryBuilder;
import org.opensearch.index.query.QueryBuilderVisitor;
Expand All @@ -28,11 +30,23 @@ public final class QueryShapeVisitor implements QueryBuilderVisitor {
private final SetOnce<String> queryType = new SetOnce<>();
private final SetOnce<String> fieldData = new SetOnce<>();
private final Map<BooleanClause.Occur, List<QueryShapeVisitor>> childVisitors = new EnumMap<>(BooleanClause.Occur.class);
private final Metadata metadata;
private final Set<String> searchIndices;

/**
* Default constructor
* @param metadata contains index mappings
* @param searchIndices successful indices searched
*/
public QueryShapeVisitor(Metadata metadata, Set<String> searchIndices) {
this.metadata = metadata;
this.searchIndices = searchIndices;
}

@Override
public void accept(QueryBuilder queryBuilder) {
queryType.set(queryBuilder.getName());
fieldData.set(buildFieldDataString(queryBuilder));
fieldData.set(buildFieldDataString(queryBuilder, metadata, searchIndices));
}

@Override
Expand All @@ -47,7 +61,7 @@ public QueryBuilderVisitor getChildVisitor(BooleanClause.Occur occur) {

@Override
public void accept(QueryBuilder qb) {
currentChild = new QueryShapeVisitor();
currentChild = new QueryShapeVisitor(metadata, searchIndices);
childVisitorList.add(currentChild);
currentChild.accept(qb);
}
Expand Down Expand Up @@ -106,9 +120,4 @@ public String prettyPrintTree(String indent, Boolean showFields) {
}
return outputBuilder.toString();
}

/**
* Default constructor
*/
public QueryShapeVisitor() {}
}
5 changes: 0 additions & 5 deletions .../java/org/opensearch/plugin/insights/core/service/categorizer/SearchQueryCategorizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,6 @@ public void categorize(SearchQueryRecord record) {
incrementQueryTypeCounters(source.query(), measurements);
incrementQueryAggregationCounters(source.aggregations(), measurements);
incrementQuerySortCounters(source.sorts(), measurements);

if (logger.isTraceEnabled()) {
String searchShape = QueryShapeGenerator.buildShape(source, true);
logger.trace(searchShape);
}
}

private void incrementQuerySortCounters(List<SortBuilder<?>> sorts, Map<MetricType, Measurement> measurements) {
Expand Down
Loading
Loading