Skip to content

Commit 0343bfd

Browse files
committed
apply some refactors
Signed-off-by: Vinay Krishna Pudyodu <vinkrish.neo@gmail.com>
1 parent e49b514 commit 0343bfd

2 files changed

Lines changed: 44 additions & 22 deletions

File tree

sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/converter/CollationResolver.java

Lines changed: 42 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -57,42 +57,62 @@ public static List<RelFieldCollation> resolve(AggregationMetadata metadata, RelD
5757
* then looks up the field's post-agg schema index(es) to create collations.
5858
* CompoundOrders are never seen here — they are flattened by AggregationMetadataBuilder.addBucketOrder.
5959
*/
60+
/** Resolved field name and sort direction from a BucketOrder. */
61+
private record OrderTarget(String fieldName, boolean ascending) {
62+
}
63+
6064
private static void resolveOrder(BucketOrder order, Map<String, List<Integer>> postAggFieldIndex, List<RelFieldCollation> collations)
6165
throws ConversionException {
62-
String fieldName;
63-
boolean ascending;
66+
// Determine which field to sort on and in which direction
67+
OrderTarget target = resolveOrderTarget(order);
6468

65-
if (order instanceof InternalOrder.Aggregation aggOrder) {
66-
fieldName = aggOrder.path().toString();
67-
ascending = aggOrder.equals(BucketOrder.aggregation(fieldName, true));
68-
} else if (InternalOrder.isKeyOrder(order)) {
69-
fieldName = "_key";
70-
ascending = InternalOrder.isKeyAsc(order);
71-
} else if (InternalOrder.isCountDesc(order)) {
72-
fieldName = "_count";
73-
ascending = false;
74-
} else if (order.equals(BucketOrder.count(true))) {
75-
fieldName = "_count";
76-
ascending = true;
77-
} else {
78-
throw new ConversionException("Unsupported BucketOrder type: " + order.getClass().getName());
79-
}
80-
81-
List<Integer> indices = postAggFieldIndex.get(fieldName);
69+
// Look up the field's post-agg schema index(es); _key may resolve to multiple indices
70+
List<Integer> indices = postAggFieldIndex.get(target.fieldName);
8271
if (indices == null) {
83-
throw new ConversionException("Sort field '" + fieldName + "' not found. Available: " + postAggFieldIndex.keySet());
72+
throw new ConversionException("Sort field '" + target.fieldName + "' not found. Available: " + postAggFieldIndex.keySet());
8473
}
8574

86-
RelFieldCollation.Direction direction = ascending ? RelFieldCollation.Direction.ASCENDING : RelFieldCollation.Direction.DESCENDING;
87-
RelFieldCollation.NullDirection nullDirection = ascending
75+
// Map direction: ASC nulls last, DESC nulls first (matches OpenSearch default)
76+
RelFieldCollation.Direction direction = target.ascending
77+
? RelFieldCollation.Direction.ASCENDING
78+
: RelFieldCollation.Direction.DESCENDING;
79+
RelFieldCollation.NullDirection nullDirection = target.ascending
8880
? RelFieldCollation.NullDirection.LAST
8981
: RelFieldCollation.NullDirection.FIRST;
9082

83+
// Create a collation per index (one for most fields, multiple for _key with nested GROUP BY)
9184
for (int idx : indices) {
9285
collations.add(new RelFieldCollation(idx, direction, nullDirection));
9386
}
9487
}
9588

89+
private static OrderTarget resolveOrderTarget(BucketOrder order) throws ConversionException {
90+
if (order instanceof InternalOrder.Aggregation aggOrder) {
91+
return resolveMetricOrder(aggOrder);
92+
} else if (InternalOrder.isKeyOrder(order)) {
93+
return resolveKeyOrder(order);
94+
} else if (InternalOrder.isCountDesc(order) || order.equals(BucketOrder.count(true))) {
95+
return resolveCountOrder(order);
96+
}
97+
throw new ConversionException("Unsupported BucketOrder type: " + order.getClass().getName());
98+
}
99+
100+
// InternalOrder.Aggregation.order is private with no accessor; equality check is the only way
101+
private static OrderTarget resolveMetricOrder(InternalOrder.Aggregation aggOrder) {
102+
String fieldName = aggOrder.path().toString();
103+
boolean ascending = aggOrder.equals(BucketOrder.aggregation(fieldName, true));
104+
return new OrderTarget(fieldName, ascending);
105+
}
106+
107+
private static OrderTarget resolveKeyOrder(BucketOrder order) {
108+
return new OrderTarget("_key", InternalOrder.isKeyAsc(order));
109+
}
110+
111+
// no InternalOrder.isCountAsc() helper; count desc is checked via isCountDesc(), count asc via equality
112+
private static OrderTarget resolveCountOrder(BucketOrder order) {
113+
return new OrderTarget("_count", !InternalOrder.isCountDesc(order));
114+
}
115+
96116
/**
97117
* Builds a map from sort field names to their indices in the post-aggregation schema.
98118
*

sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/converter/ProjectConverter.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,8 @@ private static Pattern toWildcardRegex(String wildcardPattern) {
101101
return Pattern.compile(regex.toString());
102102
}
103103

104+
// Unlike includes (resolveField throws for unknown fields), excludes silently ignore
105+
// nonexistent field names — consistent with OpenSearch core behavior.
104106
private Set<String> buildExcludeSet(String[] excludes, RelDataType rowType) {
105107
Set<String> excludeSet = new HashSet<>();
106108
for (String pattern : excludes) {

0 commit comments

Comments
 (0)