-
Notifications
You must be signed in to change notification settings - Fork 7
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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; | ||
} | ||
|
@@ -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(); | ||
} | ||
|
||
|
@@ -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); | ||
} | ||
|
@@ -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; | ||
} | ||
|
@@ -92,7 +116,9 @@ static String buildAggregationShape(AggregatorFactories.Builder aggregationsBuil | |
aggregationsBuilder.getPipelineAggregatorFactories(), | ||
new StringBuilder(), | ||
new StringBuilder(), | ||
showFields | ||
showFields, | ||
metadata, | ||
searchIndices | ||
); | ||
return aggregationShape.toString(); | ||
} | ||
|
@@ -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) { | ||
|
@@ -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"); | ||
|
||
|
@@ -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); | ||
} | ||
|
@@ -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; | ||
} | ||
|
@@ -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()); | ||
} | ||
|
@@ -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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So we are just returning the first field type we find? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we inject the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We might need to update There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When can we add this? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||
// if (field.containsKey("fields")) { | ||
// String type = findFieldType((Map<String, Object>) field.get("fields"), fieldName); | ||
// if (type != null) { | ||
// return type; | ||
// } | ||
// } | ||
} | ||
return null; | ||
} | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.