test(integ-test): stabilize FIRST/LAST/TAKE across shards

[mengweieric]

Summary

FIRST, LAST, and TAKE depend on input encounter order, which is not stable across shards. This change makes their integration tests deterministic without changing production behavior.

The tests use two complementary strategies:

• makeresults provides a defined input stream for exact assertions about first versus last, grouped selection, null skipping, mixed aggregates, and TAKE cardinality.
• Multi-document index tests keep the real alias, text, timestamp, nested, boolean, eval, pushdown, and shard-reduction paths. They assert that selected values belong to the source field and group, without requiring a particular shard encounter order.

No production code is changed.

Coverage

The index-backed tests retain coverage for:

• FIRST and LAST over text, numeric, and timestamp fields
• grouped FIRST and LAST with exact count() and avg() assertions
• nested string, integer, and boolean fields grouped by severity
• alias fields using the original multi-document queries
• evaluated fields with FIRST, LAST, and two-element TAKE
• pushdown, paginating, and no-pushdown execution

TAKE assertions verify the requested result size, distinct source documents for the current unique-value fixtures, and source-value membership. Deterministic latest() and earliest() assertions remain exact.

Validation

• Full integTestRemote against an external cluster forced to five primary shards: 7,714 tests executed. All FIRST, LAST, and TAKE aggregate tests passed. Remaining failures were outside the modified test areas.
• The four new multi-document property tests passed in pushdown, paginating, and independently verified no-pushdown modes: 12/12 executions.
• A mutation check inverted FIRST and LAST accumulation. The exact makeresults tests failed as expected, while the multi-document membership tests remained valid. This verifies that the two layers enforce different parts of the contract.
• Rebased onto the latest main and reran the affected five-shard suites successfully.
• :integ-test:spotlessCheck, :integ-test:compileTestJava, and git diff --check pass.

Related: #5716

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 2c34ab4)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ No major issues detected

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 81eae66

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Add logging for optimization fallback The fallback logic when partitionKeys contains null values silently discards the sorted first/last optimization without logging or warning. This makes debugging difficult when group key resolution fails. Consider adding a debug or warning log before the fallback to help diagnose why the optimization was skipped. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [1691-1696] List<String> groupAliases = getGroupKeyNamesAfterAggregation(baseAttributes.getLeft()); int groupCount = groupAliases.size(); RelNode baseBranch = context.relBuilder.build(); context.relBuilder.push(input); Map<String, RexNode> groupKeysByAlias = new LinkedHashMap<>(); for (UnresolvedExpression groupExpr : groupExprList) { RexNode resolved = rexVisitor.analyze(groupExpr, context); extractAliasLiteral(resolved) .map(RexLiteral::stringValue) .ifPresent(name -> groupKeysByAlias.putIfAbsent(name, stripAlias(resolved))); } context.relBuilder.build(); List<RexNode> partitionKeys = groupAliases.stream().map(groupKeysByAlias::get).toList(); if (partitionKeys.stream().anyMatch(Objects::isNull)) { + LOG.debug("Falling back to ordinary aggregate: unable to resolve all group keys for sorted first/last optimization"); context.relBuilder.push(input); return aggregateWithTrimming(groupExprList, aggExprList, context, hintIgnoreNullBucket); } Suggestion importance[1-10]: 5 __ Why: Adding debug logging when falling back from the sorted first/last optimization would improve observability and help diagnose issues. However, this is a minor enhancement that doesn't fix a bug or address a critical issue.	Low
	Prevent row number alias collisions The row number alias uses a simple numeric suffix that could collide with user-defined column names if the input already contains columns like _row_number_sorted_first_last_0. Consider using a more unique identifier such as a UUID suffix or verifying the alias doesn't conflict with existing column names. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [1719] List<RelNode> windowBranches = new ArrayList<>(); for (int i = 0; i < orderedAggExprList.size(); i++) { UnresolvedExpression expression = orderedAggExprList.get(i); AggregateFunction aggregateFunction = extractAggregateFunction(expression); context.relBuilder.push(input); RexNode measure = rexVisitor.analyze(aggregateFunction.getField(), context); ... - String rowNumberAlias = ROW_NUMBER_COLUMN_FOR_SORTED_FIRST_LAST + i; + String rowNumberAlias = ROW_NUMBER_COLUMN_FOR_SORTED_FIRST_LAST + "_" + System.nanoTime() + "_" + i; Suggestion importance[1-10]: 4 __ Why: While collision with user-defined column names is theoretically possible, the _row_number_sorted_first_last_ prefix is sufficiently unique and follows the pattern used elsewhere in the codebase (e.g., ROW_NUMBER_COLUMN_FOR_DEDUP). Using System.nanoTime() would make debugging harder and is unnecessary complexity.	Low

---

Previous suggestions

Suggestions up to commit 73ddcdb

Category	Suggestion	Impact
Possible issue	Preserve builder state before modification The method calls stripInputSort which modifies the builder state by removing the Sort node, but then falls back to aggregateWithTrimming when collation is empty. This fallback path operates on the already-modified builder, potentially causing incorrect results. Store the input node before calling stripInputSort and restore it when taking the fallback path. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [1659-1662] private Pair<List<RexNode>, List<AggCall>> aggregateSortedFirstLastWithWindows( List<UnresolvedExpression> groupExprList, List<UnresolvedExpression> aggExprList, CalcitePlanContext context, boolean hintIgnoreNullBucket) { + RelNode originalInput = context.relBuilder.peek(); RelCollation inputCollation = stripInputSort(context.relBuilder); if (inputCollation == null || inputCollation.getFieldCollations().isEmpty()) { + context.relBuilder.clear(); + context.relBuilder.push(originalInput); return aggregateWithTrimming(groupExprList, aggExprList, context, hintIgnoreNullBucket); } Suggestion importance[1-10]: 9 __ Why: The suggestion identifies a critical bug where stripInputSort modifies the builder state before checking if the fallback path is needed. If the collation is empty, the fallback aggregateWithTrimming operates on the already-modified builder, which can produce incorrect results. The fix correctly preserves and restores the original input state.	High
	Fix aggregate condition null handling The filter applies IS_TRUE to the condition, which excludes rows where the condition evaluates to NULL or FALSE. However, standard SQL aggregate conditions should only exclude FALSE, treating NULL as not matching. This can cause incorrect results when the condition expression contains nullable fields. Use a filter that only excludes FALSE values. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [1691-1699] List<RexNode> measureFilters = new ArrayList<>(); measureFilters.add(context.relBuilder.isNotNull(measure)); if (aggregateFunction.condition() != null) { + RexNode condition = rexVisitor.analyze(aggregateFunction.condition(), context); measureFilters.add( - context.rexBuilder.makeCall( - SqlStdOperatorTable.IS_TRUE, - rexVisitor.analyze(aggregateFunction.condition(), context))); + context.relBuilder.or( + context.rexBuilder.makeCall(SqlStdOperatorTable.IS_TRUE, condition), + context.relBuilder.isNull(condition))); } context.relBuilder.filter(context.relBuilder.and(measureFilters)); Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that IS_TRUE excludes NULL values, which may not align with standard SQL aggregate condition semantics. However, the proposed fix (treating NULL as TRUE) may not be the intended behavior for PPL's FIRST/LAST conditions. The correct behavior depends on the specification, but the concern about NULL handling is valid and worth verification.	Medium
	Handle ungrouped aggregation join semantics The join condition uses IS_NOT_DISTINCT_FROM for group key matching, which treats NULL as equal to NULL. However, when groupCount is zero (ungrouped aggregation), the loop doesn't execute and joinCondition becomes literal(true). This creates a cross join that duplicates rows when multiple window branches exist. For ungrouped queries, verify that only one result row is produced. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [1738-1749] for (int i = 0; i < groupCount; i++) { joinConditions.add( context.relBuilder.call( SqlStdOperatorTable.IS_NOT_DISTINCT_FROM, context.relBuilder.field(2, 0, i), context.relBuilder.field(2, 1, i))); } +if (groupCount == 0 && windowBranches.size() > 1) { + joinConditions.add(context.relBuilder.literal(true)); +} Suggestion importance[1-10]: 3 __ Why: The suggestion raises a concern about cross joins in ungrouped aggregations, but the proposed fix is incorrect. Adding literal(true) to joinConditions when it's already empty doesn't change the behavior since the condition is already literal(true) when joinConditions.isEmpty(). The existing code handles this case correctly through the left join semantics.	Low

Suggestions up to commit b686a1f

Category	Suggestion	Impact
General	Validate Boolean casts before use The method performs unchecked casts of array elements to Boolean, which can throw ClassCastException if the values are not actually Boolean objects. Add null checks and type validation before casting to prevent runtime exceptions. core/src/main/java/org/opensearch/sql/calcite/udf/udaf/OrderedAggregateUtils.java [46-52] 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]; + if (!(candidateValues[offset + 1] instanceof Boolean) + || !(candidateValues[offset + 2] instanceof Boolean) + || !(candidateValues[offset + 3] instanceof Boolean)) { + throw new IllegalArgumentException("Sort metadata must be Boolean values"); + } 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; } Suggestion importance[1-10]: 3 __ Why: While adding type validation could prevent ClassCastException, the operand type checker PPLOperandTypes.ORDERED_BY_SORT already validates that these positions contain BOOLEAN types at query planning time (lines 398-401 in PPLOperandTypes.java). This runtime check would be redundant defensive programming with minimal practical benefit.	Low

Suggestions up to commit a22f944

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

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

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

[mengweieric]

Calcite’s FIRST_VALUE/LAST_VALUE windows preserve input cardinality and would bypass the existing aggregate pushdown, while PPL stats reduces to one row per group. I’ve instead kept FIRST/LAST unchanged and implemented the ordered variants as dedicated arg-min/arg-max aggregates that do not assume sorted input and still push down as sorted top_hits. Could you take another look?

[penghuo]

My concern is why FIRST/LAST required a UDAF, It should be window function, right?

source=account
  | sort age
  | stats first(firstname) by gender

The SQL will be

  SELECT gender, firstname AS "first(firstname)"
  FROM (
      SELECT gender, firstname,
             ROW_NUMBER() OVER (PARTITION BY gender ORDER BY age NULLS FIRST) AS rn
      FROM accounts
      WHERE firstname IS NOT NULL
  ) t
  WHERE rn = 1;

[mengweieric]

Yes, that is the right call. FIRST and LAST are now implemented as window functions rather than sort-aware aggregates, following the shape you outlined. A preceding sort is lowered into a partitioned ROW_NUMBER, the top-ranked row per group is selected, and null values are excluded before ranking so the first or last non-null value is returned. LAST simply reverses the ordering. The aggregate implementations themselves no longer have any notion of input ordering.

[penghuo]

After deep look, I think the first/last function by definition is non-deterministic.
https://spark.apache.org/docs/latest/api/sql/agg-functions/#first

If it is just IT failed in multiple shard use case, try to use makeresults | stats first(xxx) to get deterministic results.

[mengweieric]

Updated following your latest suggestion. The engine changes have been removed. Exact FIRST/LAST stream behavior is now tested with makeresults, while multi-document index tests retain alias, nested, grouped, eval, pushdown, paginating, and no-pushdown coverage using membership assertions that do not assume shard order. A full five-primary-shard integTestRemote run executed 7,714 tests with zero FIRST/LAST/TAKE aggregate failures.

[github-actions]

Persistent review updated to latest commit b686a1f

[github-actions]

Persistent review updated to latest commit 73ddcdb

[github-actions]

Persistent review updated to latest commit 81eae66

[github-actions]

Persistent review updated to latest commit 2c34ab4