[BUG] make first/last follow a preceding sort

[mengweieric]

Description

first(x) and last(x) ignored a preceding PPL sort because document order is not globally defined across shards, and Calcite can discard input collation below an aggregate.

When an explicit input sort exists, the planner now resolves first/last to internal FIRST_BY_SORT/LAST_BY_SORT aggregates carrying the complete sort tuple. Each key carries its value, direction, null placement, and IP-type metadata. This provides one logical representation for both execution paths:

• Pushdown: direct field keys become equivalent multi-field top_hits sorts. LAST_BY_SORT reverses direction and null placement for every key.
• No pushdown: a dedicated Calcite aggregate implementor skips only null measures and preserves nullable sort keys for lexicographic comparison.

A pushdown-only dedup sort hint cannot replace the operand encoding: it can tell AggregateAnalyzer how to sort top_hits, but it cannot provide each row's sort-key values to the enumerable accumulator.

The implementation preserves existing behavior when no explicit sort exists. Null measures are skipped, null sort keys follow the explicit null placement, IP keys use numeric IP comparison, and equal complete sort tuples remain unordered. Computed/script sort keys fall back to enumerable execution rather than changing null semantics.

Validation

• Five-primary-shard FIRST/LAST regressions with the true winners distributed across shards
• Full mixed-direction multi-key collation
• Null measures and null sort keys
• IP sort keys
• Default pushdown and CalciteNoPushdownIT
• Exact previously failing CI seed F0DAFDABA04C8A0E
• Pushdown and no-pushdown explain fixtures
• Ordered operand metadata and aggregate-filter fail-closed unit tests
• Existing FIRST/LAST planner and integration tests
• spotlessCheck and git diff --check

Deterministic Before/After Evidence

The comparison used the same cluster, the same five-shard index, and the same three documents. Placement was intentionally adversarial: the newest document was forced onto shard 0 and the oldest onto shard 4, so shard-ordinal tie-breaking produces the wrong answer reliably rather than by chance. Only the plugin JAR changed between captures.

The user's intent survives into the logical plan

The following line is identical before and after:

LogicalSort(sort0=[$8], dir0=[ASC-nulls-first])

The requested ordering is therefore present in the logical plan. The pre-fix error occurs downstream, where that ordering is not encoded in the generated aggregation request.

Before: pre-fix baseline

Baseline commit: 5f5876dd0 (the parent of this PR).

Function	Logical aggregate	Generated top_hits sort	Result	Expected	Verdict
FIRST	FIRST($0)	Absent	2024-01-03	2024-01-01	Wrong
LAST	LAST($0)	[{"_doc":{"order":"desc"}}]	2024-01-03	2024-01-03	Correct only because of this placement

FIRST emits top_hits without a sort key. LAST sorts on _doc, which is a shard-local Lucene document ID and has no global meaning across shards. Neither request carries the user's created_at ordering.

LAST happens to return the expected value because the newest document is on shard 0, so the fallback shard-ordinal tie-break selects it. Changing document placement can change the result.

After: this PR

A representative rewritten aggregate is:

LogicalAggregate(FIRST(`@timestamp`)=[FIRST_BY_SORT($0, $0, $1, $2, $1) FILTER $3])
  LogicalProject(@timestamp=[$8], $f1=[false], $f2=[true], $f3=[IS NOT NULL($8)])

The aggregate now carries the sort key, direction (false for ascending), null placement (true for nulls first), and IP-type flag. It also carries a non-null filter for the measure.

Function	Generated top_hits sort	Result	Expected	Verdict
FIRST	created_at ASC, missing: _first	2024-01-01	2024-01-01	Correct
LAST	created_at DESC, missing: _last	2024-01-03	2024-01-03	Correct

FIRST and LAST are also wrapped in an exists(created_at) filter aggregation, preserving their first/last-non-null contract during pushdown. The @timestamp alias is resolved to its concrete mapped field, created_at, in the generated DSL.

What this proves

OpenSearch performs a global sort-aware top_hits merge when the request includes comparable field-sort values. For FIRST and LAST, the explicit-sort contract makes dropping the available collation a confirmed product defect; this PR preserves that collation through planning and request generation.

The capture above validates pushdown. The same adversarial fixture was also executed with pushdown disabled:

Execution path	FIRST	LAST
Pushdown enabled	2024-01-01	2024-01-03
Pushdown disabled	2024-01-01	2024-01-03

FIRST and LAST agree and return the expected values on both paths.

Tie behavior is not part of this comparison

The fixture uses three distinct timestamps. Rows with equal complete sort tuples remain unordered by design and are not involved in this before/after result.

Related Issues

Addresses the FIRST/LAST portions of #5716.
Previous PR: #4223

Check List

• New functionality includes testing.
• New functionality has been documented.
• New functionality has javadoc added.
• New functionality has a user manual doc added.
• New PPL command checklist all confirmed.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff or -s.
• Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit a22f944)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue The compare method does not validate that candidateValues and retainedKeys have matching key counts before iterating. If retainedKeys.length is zero but candidateValues contains sort keys, the loop never executes and the method returns 0, treating rows with different sort keys as equal. This can occur if the first row added to an accumulator has a null measure (skipped by the aggregate function) but valid sort keys, leaving retainedKeys empty while subsequent rows carry sort keys. static int compare(Object[] candidateValues, Object[] retainedKeys) { if (!hasSortKeys(candidateValues) || retainedKeys.length == 0) { throw new IllegalArgumentException("Ordered comparison requires at least one sort key"); } int keyCount = (candidateValues.length - 1) / ARGUMENTS_PER_SORT_KEY; if (retainedKeys.length != keyCount) { throw new IllegalArgumentException("Sort-key count changed while aggregating"); } for (int key = 0; key < keyCount; key++) { int offset = 1 + key * ARGUMENTS_PER_SORT_KEY; Object candidate = candidateValues[offset]; boolean descending = (Boolean) candidateValues[offset + 1]; boolean nullsFirst = (Boolean) candidateValues[offset + 2]; boolean ipType = (Boolean) candidateValues[offset + 3]; int comparison = compareValue(candidate, retainedKeys[key], descending, nullsFirst, ipType); if (comparison != 0) { return comparison; } } return 0; } Possible Issue The createOrderedTopHitsBuilder method throws AggregateAnalyzerException when encountering computed or script sort keys, forcing enumerable execution. However, the exception message states "Ordered FIRST/LAST pushdown requires direct field sort keys" without clarifying that this is an intentional fallback rather than a user error. If the planner does not catch this exception and retry with enumerable execution, the query fails instead of falling back gracefully. /** Build a top_hits aggregation whose sort exactly mirrors the explicit PPL input collation. */ private static TopHitsAggregationBuilder createOrderedTopHitsBuilder( AggregateCall aggCall, List<Pair<RexNode, String>> args, String aggName, AggregateBuilderHelper helper, boolean first) { final int argumentsPerSortKey = 4; if (args.size() < 1 + argumentsPerSortKey || (args.size() - 1) % argumentsPerSortKey != 0) { throw new AggregateAnalyzerException("Invalid ordered FIRST/LAST arguments"); } TopHitsAggregationBuilder builder = createTopHitsBuilder( aggCall, List.of(args.getFirst()), aggName, helper, 1, true, false, null, null); for (int offset = 1; offset < args.size(); offset += argumentsPerSortKey) { RexNode sortKey = args.get(offset).getKey(); if (!(sortKey instanceof RexInputRef)) { // Script-sort null semantics are not equivalent to field-sort missing placement. Let the // planner retain the enumerable ordered aggregate instead of silently changing semantics. throw new AggregateAnalyzerException( "Ordered FIRST/LAST pushdown requires direct field sort keys"); } boolean descending = helper.inferValue(args.get(offset + 1).getKey(), Boolean.class); boolean nullsFirst = helper.inferValue(args.get(offset + 2).getKey(), Boolean.class); // Validate the internal operand layout even though IP fields use the same OpenSearch sort. helper.inferValue(args.get(offset + 3).getKey(), Boolean.class); if (!first) { descending = !descending; nullsFirst = !nullsFirst; } NamedFieldExpression fieldExpression = helper.inferNamedField(sortKey); String sortField = fieldExpression.getReferenceForTermQuery(); if (sortField == null) { throw new AggregateAnalyzerException( "Ordered FIRST/LAST pushdown requires a sortable field"); } builder.sort( SortBuilders.fieldSort(sortField) .order(descending ? SortOrder.DESC : SortOrder.ASC) .missing(nullsFirst ? "_first" : "_last")); } return builder; }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to a22f944

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Clone extracted sort keys defensively The sortKeys array is shared and could be modified externally after extraction. Clone the extracted sort keys to prevent unintended mutations that could corrupt the accumulator's state during concurrent aggregation. core/src/main/java/org/opensearch/sql/calcite/udf/udaf/FirstAggFunction.java [57-63] public synchronized void consider(Object value, Object[] values) { if (!hasValue || OrderedAggregateUtils.compare(values, sortKeys) < 0) { this.first = value; - this.sortKeys = OrderedAggregateUtils.extractSortKeys(values); + Object[] extracted = OrderedAggregateUtils.extractSortKeys(values); + this.sortKeys = extracted.clone(); this.hasValue = true; } } Suggestion importance[1-10]: 4 __ Why: The suggestion to clone sortKeys provides defensive copying against potential external mutations. However, since extractSortKeys already creates a new array and the method is synchronized, the risk is low. The improvement is minor but could prevent subtle bugs in concurrent scenarios.	Low
	Add bounds check for array access The method accesses args.get(offset + 3) without verifying that offset + 3 is within bounds. Although the modulo check should prevent this, add an explicit bounds check before accessing array elements to prevent potential IndexOutOfBoundsException if the validation logic has a flaw. opensearch/src/main/java/org/opensearch/sql/opensearch/request/AggregateAnalyzer.java [864-907] -private static TopHitsAggregationBuilder createOrderedTopHitsBuilder( - AggregateCall aggCall, - List<Pair<RexNode, String>> args, - String aggName, - AggregateBuilderHelper helper, - boolean first) { - final int argumentsPerSortKey = 4; - if (args.size() < 1 + argumentsPerSortKey || (args.size() - 1) % argumentsPerSortKey != 0) { - throw new AggregateAnalyzerException("Invalid ordered FIRST/LAST arguments"); +for (int offset = 1; offset < args.size(); offset += argumentsPerSortKey) { + if (offset + argumentsPerSortKey - 1 >= args.size()) { + throw new AggregateAnalyzerException("Incomplete sort key group at offset " + offset); } - ... - for (int offset = 1; offset < args.size(); offset += argumentsPerSortKey) { - RexNode sortKey = args.get(offset).getKey(); - if (!(sortKey instanceof RexInputRef)) { - throw new AggregateAnalyzerException(...); - } - boolean descending = helper.inferValue(args.get(offset + 1).getKey(), Boolean.class); - boolean nullsFirst = helper.inferValue(args.get(offset + 2).getKey(), Boolean.class); - helper.inferValue(args.get(offset + 3).getKey(), Boolean.class); - ... + RexNode sortKey = args.get(offset).getKey(); + if (!(sortKey instanceof RexInputRef)) { + throw new AggregateAnalyzerException(...); } + boolean descending = helper.inferValue(args.get(offset + 1).getKey(), Boolean.class); + boolean nullsFirst = helper.inferValue(args.get(offset + 2).getKey(), Boolean.class); + helper.inferValue(args.get(offset + 3).getKey(), Boolean.class); ... } Suggestion importance[1-10]: 2 __ Why: The modulo validation at line 871 ((args.size() - 1) % argumentsPerSortKey != 0) already ensures complete sort key groups exist, making the suggested explicit bounds check redundant. The validation logic is sound and the additional check adds unnecessary complexity without meaningful safety improvement.	Low
Possible issue	Validate Boolean flags before casting Add null checks before casting operands to Boolean to prevent NullPointerException when sort metadata flags are unexpectedly null. The code assumes these flags are always non-null, but defensive programming would catch configuration errors earlier. core/src/main/java/org/opensearch/sql/calcite/udf/udaf/OrderedAggregateUtils.java [38-58] static int compare(Object[] candidateValues, Object[] retainedKeys) { if (!hasSortKeys(candidateValues) || retainedKeys.length == 0) { throw new IllegalArgumentException("Ordered comparison requires at least one sort key"); } int keyCount = (candidateValues.length - 1) / ARGUMENTS_PER_SORT_KEY; if (retainedKeys.length != keyCount) { throw new IllegalArgumentException("Sort-key count changed while aggregating"); } for (int key = 0; key < keyCount; key++) { int offset = 1 + key * ARGUMENTS_PER_SORT_KEY; Object candidate = candidateValues[offset]; - boolean descending = (Boolean) candidateValues[offset + 1]; - boolean nullsFirst = (Boolean) candidateValues[offset + 2]; - boolean ipType = (Boolean) candidateValues[offset + 3]; + Object descendingObj = candidateValues[offset + 1]; + Object nullsFirstObj = candidateValues[offset + 2]; + Object ipTypeObj = candidateValues[offset + 3]; + if (!(descendingObj instanceof Boolean) || !(nullsFirstObj instanceof Boolean) || !(ipTypeObj instanceof Boolean)) { + throw new IllegalArgumentException("Sort metadata flags must be Boolean"); + } + boolean descending = (Boolean) descendingObj; + boolean nullsFirst = (Boolean) nullsFirstObj; + boolean ipType = (Boolean) ipTypeObj; int comparison = compareValue(candidate, retainedKeys[key], descending, nullsFirst, ipType); if (comparison != 0) { return comparison; } } return 0; } Suggestion importance[1-10]: 3 __ Why: While adding null checks is defensive, the PPLOperandTypes.ORDERED_BY_SORT validator already ensures these operands are BOOLEAN type at query validation time, making runtime null checks redundant. The suggestion adds unnecessary overhead for a condition that should never occur in validated queries.	Low

---

Previous suggestions

Suggestions up to commit fe7d053

Category	Suggestion	Impact
Possible issue	Handle NumberFormatException in numeric comparison Handle potential NumberFormatException when converting numbers to BigDecimal via toString(). Some Number implementations may produce non-parseable string representations, which would cause runtime failures during aggregation. core/src/main/java/org/opensearch/sql/calcite/udf/udaf/OrderedAggregateUtils.java [78-87] } else if (left instanceof Number leftNumber && right instanceof Number rightNumber) { if (left instanceof Float || left instanceof Double || right instanceof Float || right instanceof Double) { comparison = Double.compare(leftNumber.doubleValue(), rightNumber.doubleValue()); } else { - comparison = - new BigDecimal(leftNumber.toString()).compareTo(new BigDecimal(rightNumber.toString())); + try { + comparison = + new BigDecimal(leftNumber.toString()).compareTo(new BigDecimal(rightNumber.toString())); + } catch (NumberFormatException e) { + throw new IllegalArgumentException( + "Unable to compare numeric values: " + left.getClass().getName(), e); + } } Suggestion importance[1-10]: 4 __ Why: Adding exception handling for NumberFormatException when converting Number to BigDecimal is a reasonable defensive measure. However, in practice, standard Number implementations like Integer, Long, and BigInteger produce parseable strings, making this edge case unlikely. The suggestion adds safety but addresses a low-probability scenario.	Low
	Add null check for retainedKeys Add a null check for retainedKeys before checking its length to prevent potential NullPointerException. This is critical since FirstAccumulator and LastAccumulator initialize sortKeys to an empty array, but defensive programming should handle null inputs. core/src/main/java/org/opensearch/sql/calcite/udf/udaf/OrderedAggregateUtils.java [38-41] static int compare(Object[] candidateValues, Object[] retainedKeys) { - if (!hasSortKeys(candidateValues) || retainedKeys.length == 0) { + if (!hasSortKeys(candidateValues) || retainedKeys == null || retainedKeys.length == 0) { throw new IllegalArgumentException("Ordered comparison requires at least one sort key"); } Suggestion importance[1-10]: 2 __ Why: The suggestion is overly defensive. The sortKeys field in both FirstAccumulator and LastAccumulator is initialized to new Object[0] (line 43 in FirstAggFunction.java and line 43 in LastAggFunction.java), so it can never be null in normal operation. The existing check for retainedKeys.length == 0 is sufficient.	Low
	Add bounds checking for argument access Add bounds checking before accessing args.get(offset + 1), args.get(offset + 2), and args.get(offset + 3) to prevent IndexOutOfBoundsException. While the initial validation checks the argument count, defensive programming should verify each access is within bounds. opensearch/src/main/java/org/opensearch/sql/opensearch/request/AggregateAnalyzer.java [896-903] for (int offset = 1; offset < args.size(); offset += argumentsPerSortKey) { + if (offset + argumentsPerSortKey - 1 >= args.size()) { + throw new AggregateAnalyzerException("Invalid ordered FIRST/LAST arguments"); + } RexNode sortKey = args.get(offset).getKey(); if (!(sortKey instanceof RexInputRef)) { throw new AggregateAnalyzerException( "Ordered FIRST/LAST pushdown requires direct field sort keys"); } boolean descending = helper.inferValue(args.get(offset + 1).getKey(), Boolean.class); boolean nullsFirst = helper.inferValue(args.get(offset + 2).getKey(), Boolean.class); helper.inferValue(args.get(offset + 3).getKey(), Boolean.class); Suggestion importance[1-10]: 1 __ Why: The suggestion is redundant. The initial validation at lines 871-873 already ensures that (args.size() - 1) % argumentsPerSortKey == 0, which guarantees that the loop will never access out-of-bounds indices. Adding another check inside the loop would be unnecessary and would duplicate the validation logic.	Low
General	Use null for initial sortKeys Initialize sortKeys to null instead of an empty array to avoid unnecessary object allocation when sort keys are not used. The compare method already handles empty arrays, so using null as the initial state is more memory-efficient and semantically clearer. core/src/main/java/org/opensearch/sql/calcite/udf/udaf/FirstAggFunction.java [43] -private Object[] sortKeys = new Object[0]; +private Object[] sortKeys = null; Suggestion importance[1-10]: 3 __ Why: While using null instead of an empty array could save a small amount of memory, the current implementation with new Object[0] is a common Java pattern and works correctly with the compare method. The improvement is marginal and primarily stylistic.	Low

Suggestions up to commit fe7d053

Category	Suggestion	Impact
Possible issue	Add null checks before unboxing Add null-safety checks for the boolean flags before unboxing to prevent NullPointerException. The code directly unboxes Boolean objects at offsets +1, +2, and +3 without verifying they are non-null, which could cause runtime failures if the internal operand layout is corrupted. core/src/main/java/org/opensearch/sql/calcite/udf/udaf/OrderedAggregateUtils.java [38-58] static int compare(Object[] candidateValues, Object[] retainedKeys) { if (!hasSortKeys(candidateValues) || retainedKeys.length == 0) { throw new IllegalArgumentException("Ordered comparison requires at least one sort key"); } int keyCount = (candidateValues.length - 1) / ARGUMENTS_PER_SORT_KEY; if (retainedKeys.length != keyCount) { throw new IllegalArgumentException("Sort-key count changed while aggregating"); } for (int key = 0; key < keyCount; key++) { int offset = 1 + key * ARGUMENTS_PER_SORT_KEY; Object candidate = candidateValues[offset]; - boolean descending = (Boolean) candidateValues[offset + 1]; - boolean nullsFirst = (Boolean) candidateValues[offset + 2]; - boolean ipType = (Boolean) candidateValues[offset + 3]; + Boolean descendingObj = (Boolean) candidateValues[offset + 1]; + Boolean nullsFirstObj = (Boolean) candidateValues[offset + 2]; + Boolean ipTypeObj = (Boolean) candidateValues[offset + 3]; + if (descendingObj == null || nullsFirstObj == null || ipTypeObj == null) { + throw new IllegalArgumentException("Sort metadata flags cannot be null"); + } + boolean descending = descendingObj; + boolean nullsFirst = nullsFirstObj; + boolean ipType = ipTypeObj; int comparison = compareValue(candidate, retainedKeys[key], descending, nullsFirst, ipType); if (comparison != 0) { return comparison; } } return 0; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential NullPointerException when unboxing Boolean objects. However, since these values come from internal operands constructed by the framework itself (as seen in PPLFuncImpTable.java where literals are created), this is more of a defensive programming improvement than a critical bug fix. The validation in PPLOperandTypes.ORDERED_BY_SORT already ensures these are boolean types.	Medium
General	Store defensive copy of sort keys The sortKeys array is shared and could be modified externally after extraction. Store a defensive copy to prevent potential data corruption if the caller modifies the original array after passing it to consider. core/src/main/java/org/opensearch/sql/calcite/udf/udaf/FirstAggFunction.java [57-63] public synchronized void consider(Object value, Object[] values) { if (!hasValue || OrderedAggregateUtils.compare(values, sortKeys) < 0) { this.first = value; - this.sortKeys = OrderedAggregateUtils.extractSortKeys(values); + Object[] extracted = OrderedAggregateUtils.extractSortKeys(values); + this.sortKeys = Arrays.copyOf(extracted, extracted.length); this.hasValue = true; } } Suggestion importance[1-10]: 5 __ Why: While defensive copying is generally good practice, the extractSortKeys method already creates a new array (line 30 in OrderedAggregateUtils.java), so the returned array is not shared with the caller. The suggestion adds unnecessary overhead without addressing an actual vulnerability in this context.	Low
	Add bounds check in loop Add bounds checking before accessing args.get(offset + 1), args.get(offset + 2), and args.get(offset + 3) to prevent IndexOutOfBoundsException. Although the initial validation checks the modulo condition, an explicit bounds check provides defense against edge cases. opensearch/src/main/java/org/opensearch/sql/opensearch/request/AggregateAnalyzer.java [876-890] -private static TopHitsAggregationBuilder createOrderedTopHitsBuilder( - AggregateCall aggCall, - List<Pair<RexNode, String>> args, - String aggName, - AggregateBuilderHelper helper, - boolean first) { - final int argumentsPerSortKey = 4; - if (args.size() < 1 + argumentsPerSortKey || (args.size() - 1) % argumentsPerSortKey != 0) { - throw new AggregateAnalyzerException("Invalid ordered FIRST/LAST arguments"); +for (int offset = 1; offset < args.size(); offset += argumentsPerSortKey) { + if (offset + 3 >= args.size()) { + throw new AggregateAnalyzerException("Incomplete sort key metadata at offset " + offset); } - ... - for (int offset = 1; offset < args.size(); offset += argumentsPerSortKey) { - RexNode sortKey = args.get(offset).getKey(); - if (!(sortKey instanceof RexInputRef)) { - throw new AggregateAnalyzerException(...); - } - boolean descending = helper.inferValue(args.get(offset + 1).getKey(), Boolean.class); - boolean nullsFirst = helper.inferValue(args.get(offset + 2).getKey(), Boolean.class); - helper.inferValue(args.get(offset + 3).getKey(), Boolean.class); + RexNode sortKey = args.get(offset).getKey(); + if (!(sortKey instanceof RexInputRef)) { + throw new AggregateAnalyzerException(...); + } + boolean descending = helper.inferValue(args.get(offset + 1).getKey(), Boolean.class); + boolean nullsFirst = helper.inferValue(args.get(offset + 2).getKey(), Boolean.class); + helper.inferValue(args.get(offset + 3).getKey(), Boolean.class); Suggestion importance[1-10]: 3 __ Why: The initial validation at line 871-873 already ensures (args.size() - 1) % argumentsPerSortKey == 0, which mathematically guarantees that offset + 3 will never exceed args.size() within the loop bounds. The suggested check is redundant and adds unnecessary complexity without improving safety.	Low

Suggestions up to commit 518a04b

Category	Suggestion	Impact
General	Optimize memory allocation for sort keys Initialize sortKeys as null instead of an empty array to avoid unnecessary object allocation when sort keys are not used. This reduces memory overhead for the common case where FIRST operates without explicit sorting. core/src/main/java/org/opensearch/sql/calcite/udf/udaf/FirstAggFunction.java [43] -private Object[] sortKeys = new Object[0]; +private Object[] sortKeys = null; Suggestion importance[1-10]: 3 __ Why: While initializing sortKeys as null instead of an empty array could save a small amount of memory, this optimization is marginal. The code would need additional null checks in OrderedAggregateUtils.compare() which already checks for retainedKeys.length == 0. The current implementation is clearer and safer.	Low

Suggestions up to commit a22f944

Category	Suggestion	Impact
Possible issue	Add null-safety for Boolean unboxing Add null-safety checks before unboxing Boolean values to prevent potential NullPointerException. The operands at offsets +1, +2, and +3 are expected to be Boolean, but if they are null, the unboxing will throw an exception during aggregation. core/src/main/java/org/opensearch/sql/calcite/udf/udaf/OrderedAggregateUtils.java [38-58] static int compare(Object[] candidateValues, Object[] retainedKeys) { if (!hasSortKeys(candidateValues) || retainedKeys.length == 0) { throw new IllegalArgumentException("Ordered comparison requires at least one sort key"); } int keyCount = (candidateValues.length - 1) / ARGUMENTS_PER_SORT_KEY; if (retainedKeys.length != keyCount) { throw new IllegalArgumentException("Sort-key count changed while aggregating"); } for (int key = 0; key < keyCount; key++) { int offset = 1 + key * ARGUMENTS_PER_SORT_KEY; Object candidate = candidateValues[offset]; - boolean descending = (Boolean) candidateValues[offset + 1]; - boolean nullsFirst = (Boolean) candidateValues[offset + 2]; - boolean ipType = (Boolean) candidateValues[offset + 3]; + Boolean descendingObj = (Boolean) candidateValues[offset + 1]; + Boolean nullsFirstObj = (Boolean) candidateValues[offset + 2]; + Boolean ipTypeObj = (Boolean) candidateValues[offset + 3]; + if (descendingObj == null || nullsFirstObj == null || ipTypeObj == null) { + throw new IllegalArgumentException("Sort metadata flags cannot be null"); + } + boolean descending = descendingObj; + boolean nullsFirst = nullsFirstObj; + boolean ipType = ipTypeObj; int comparison = compareValue(candidate, retainedKeys[key], descending, nullsFirst, ipType); if (comparison != 0) { return comparison; } } return 0; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential NullPointerException risk when unboxing Boolean values. However, the operands are validated by PPLOperandTypes.ORDERED_BY_SORT which ensures they are Boolean types, making null values unlikely in practice. The suggestion improves defensive programming but addresses a low-probability scenario.	Medium
	Validate Boolean inference results before unboxing The inferValue calls for Boolean flags could return null if the value cannot be inferred, leading to NullPointerException when unboxing. Add explicit null checks after each inferValue call to provide clearer error messages and prevent runtime failures. opensearch/src/main/java/org/opensearch/sql/opensearch/request/AggregateAnalyzer.java [879-890] -private static TopHitsAggregationBuilder createOrderedTopHitsBuilder( - AggregateCall aggCall, - List<Pair<RexNode, String>> args, - String aggName, - AggregateBuilderHelper helper, - boolean first) { - final int argumentsPerSortKey = 4; - if (args.size() < 1 + argumentsPerSortKey || (args.size() - 1) % argumentsPerSortKey != 0) { - throw new AggregateAnalyzerException("Invalid ordered FIRST/LAST arguments"); +for (int offset = 1; offset < args.size(); offset += argumentsPerSortKey) { + RexNode sortKey = args.get(offset).getKey(); + if (!(sortKey instanceof RexInputRef)) { + throw new AggregateAnalyzerException( + "Ordered FIRST/LAST pushdown requires direct field sort keys"); } - ... - for (int offset = 1; offset < args.size(); offset += argumentsPerSortKey) { - RexNode sortKey = args.get(offset).getKey(); - if (!(sortKey instanceof RexInputRef)) { - throw new AggregateAnalyzerException( - "Ordered FIRST/LAST pushdown requires direct field sort keys"); - } - boolean descending = helper.inferValue(args.get(offset + 1).getKey(), Boolean.class); - boolean nullsFirst = helper.inferValue(args.get(offset + 2).getKey(), Boolean.class); - helper.inferValue(args.get(offset + 3).getKey(), Boolean.class); + Boolean descendingObj = helper.inferValue(args.get(offset + 1).getKey(), Boolean.class); + Boolean nullsFirstObj = helper.inferValue(args.get(offset + 2).getKey(), Boolean.class); + Boolean ipTypeObj = helper.inferValue(args.get(offset + 3).getKey(), Boolean.class); + if (descendingObj == null || nullsFirstObj == null || ipTypeObj == null) { + throw new AggregateAnalyzerException("Sort metadata flags must be non-null Boolean literals"); + } + boolean descending = descendingObj; + boolean nullsFirst = nullsFirstObj; Suggestion importance[1-10]: 7 __ Why: The suggestion identifies a valid concern about potential null returns from inferValue. Adding explicit null checks would provide clearer error messages and prevent NullPointerException. However, the operands are validated by the type checker, making null returns unlikely. This is a defensive improvement rather than fixing a critical bug.	Medium
General	Prevent concurrent modification of sort keys The sortKeys array is shared and could be modified by concurrent threads. Consider creating a defensive copy when extracting sort keys to prevent race conditions where one thread's comparison reads partially updated keys from another thread's extraction. core/src/main/java/org/opensearch/sql/calcite/udf/udaf/FirstAggFunction.java [57-63] public synchronized void consider(Object value, Object[] values) { if (!hasValue || OrderedAggregateUtils.compare(values, sortKeys) < 0) { this.first = value; - this.sortKeys = OrderedAggregateUtils.extractSortKeys(values); + Object[] newKeys = OrderedAggregateUtils.extractSortKeys(values); + this.sortKeys = Arrays.copyOf(newKeys, newKeys.length); this.hasValue = true; } } Suggestion importance[1-10]: 3 __ Why: The consider method is already synchronized, which prevents concurrent access to the sortKeys field. The extractSortKeys method creates a new array, so there's no shared mutable state. The suggested defensive copy is redundant and adds unnecessary overhead without addressing an actual concurrency issue.	Low

Suggestions up to commit 6e32d15

Category	Suggestion	Impact
Possible issue	Add volatile modifier to sortKeys The sortKeys field should be marked as volatile to ensure thread-safe visibility when accessed across threads, consistent with the volatile modifiers on first and hasValue. Without this, concurrent updates in consider() may not be visible to other threads. core/src/main/java/org/opensearch/sql/calcite/udf/udaf/FirstAggFunction.java [43] private volatile Object first; private volatile boolean hasValue; -private Object[] sortKeys = new Object[0]; +private volatile Object[] sortKeys = new Object[0]; Suggestion importance[1-10]: 8 __ Why: The sortKeys field is accessed and modified in the synchronized consider() method but lacks the volatile modifier present on first and hasValue. This could lead to visibility issues in concurrent scenarios where the field might be read outside synchronized blocks.	Medium
General	Create defensive copies of sort keys The extracted keys array contains references to mutable objects from the input values array. If these objects are modified externally, the stored sort keys could change unexpectedly. Consider creating defensive copies of the extracted values to prevent unintended mutations. core/src/main/java/org/opensearch/sql/calcite/udf/udaf/OrderedAggregateUtils.java [28-35] static Object[] extractSortKeys(Object[] values) { int keyCount = (values.length - 1) / ARGUMENTS_PER_SORT_KEY; Object[] keys = new Object[keyCount]; for (int key = 0; key < keyCount; key++) { - keys[key] = values[1 + key * ARGUMENTS_PER_SORT_KEY]; + Object value = values[1 + key * ARGUMENTS_PER_SORT_KEY]; + keys[key] = value instanceof byte[] ? ((byte[]) value).clone() : value; } return keys; } Suggestion importance[1-10]: 3 __ Why: While creating defensive copies could prevent external mutations, the suggestion only handles byte[] arrays. The concern is valid but the implementation is incomplete as other mutable types in the values could also be modified. The impact is limited since the code appears to control the input sources.	Low

[github-actions]

Persistent review updated to latest commit 21cc90f

[github-actions]

Persistent review updated to latest commit 6e32d15

[github-actions]

Persistent review updated to latest commit a22f944

[github-actions]

Persistent review updated to latest commit 4436897

[github-actions]

Persistent review updated to latest commit fe7d053

[github-actions]

Persistent review updated to latest commit fe7d053

[github-actions]

Persistent review updated to latest commit a22f944

[penghuo]

Why not regiester FIRST/LAST as window function. A UDAF itself should not know the input is sorted or not.
@dai-chen has comments on previous PR. #4223 (review)