Skip to content

chore: add SQL tests for FIRST/LAST aggregates#3891

Merged
comphead merged 4 commits intoapache:mainfrom
comphead:agg_nulls
Apr 4, 2026
Merged

chore: add SQL tests for FIRST/LAST aggregates#3891
comphead merged 4 commits intoapache:mainfrom
comphead:agg_nulls

Conversation

@comphead
Copy link
Copy Markdown
Contributor

@comphead comphead commented Apr 2, 2026

Which issue does this PR close?

Closes #1630 #1646 .

Rationale for this change

Summary

Adds comprehensive SQL file tests for first and last aggregation functions with IGNORE NULLS semantics.

Motivation

The existing first_last.sql test file had only 3 basic queries covering first/last without any IGNORE NULLS coverage. The Scala test suite (CometAggregateSuite) has some IGNORE NULLS tests
but several are disabled (TODO #1646). SQL file tests are the preferred approach for expression testing in Comet and provide better coverage across the ConfigMatrix (dictionary encoding on/off).

What's added

Tests are organized into three sections: FIRST, LAST, and FIRST + LAST combined, each covering:

  • Basic behavior — default (respect nulls) and IGNORE NULLS
  • All-null groups — verifies NULL is returned when no non-null values exist
  • Empty table — zero-row input
  • Single row — one non-null value, and one null value
  • Multiple data types — int, bigint, double, string, boolean (expect_fallback due to SortAggregate)
  • Null positions — nulls at the beginning, middle, and end of groups
  • Boolean parameter formfirst(val, true) / first(val, false) equivalence with IGNORE NULLS syntax
  • Expressions producing nullsIF(val % 2 = 0, NULL, val) inside aggregation
  • Mixed aggregationsfirst/last IGNORE NULLS alongside regular first/last, count, sum
  • HAVING clause — filtering groups based on IGNORE NULLS result
  • Decimal typeDECIMAL(10,2)
  • Date typeDATE values
  • Large group (1000 rows) — exercises multi-batch aggregation with sparse non-null values (every 10th row)

What changes are included in this PR?

How are these changes tested?

@kazuyukitanimura
Copy link
Copy Markdown
Contributor

Thanks @comphead I think we cannot close #1630 yet because we are referring this issue e.g. docs/source/user-guide/latest/compatibility.md

The test itself looks good

Copy link
Copy Markdown
Contributor

@mbutrovich mbutrovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @comphead! Always good to have more tests.

@comphead
Copy link
Copy Markdown
Contributor Author

comphead commented Apr 3, 2026

Thanks @comphead I think we cannot close #1630 yet because we are referring this issue e.g. docs/source/user-guide/latest/compatibility.md

I think we can close #1630 as it referred to not using IGNORE NULLS flag, which is fixed now. However we need to track somewhere the deterministic feature, if we can actually reproduce it. WDYT?

@kazuyukitanimura
Copy link
Copy Markdown
Contributor

Thanks @comphead I think we cannot close #1630 yet because we are referring this issue e.g. docs/source/user-guide/latest/compatibility.md

I think we can close #1630 as it referred to not using IGNORE NULLS flag, which is fixed now. However we need to track somewhere the deterministic feature, if we can actually reproduce it. WDYT?

Oh I see. You are right, we can close it. Let's just update the docs though
docs/source/user-guide/latest/compatibility.md
docs/source/user-guide/latest/expressions.md

@comphead
Copy link
Copy Markdown
Contributor Author

comphead commented Apr 3, 2026

I'll check also #1646 if everything works, we can just close 2 tickets and remove deterministic warning, otherwise we can refer to 1646

Copy link
Copy Markdown
Contributor

@kazuyukitanimura kazuyukitanimura left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pending ci

@comphead
Copy link
Copy Markdown
Contributor Author

comphead commented Apr 4, 2026

Thanks everyone for review

@comphead comphead merged commit b6a07e0 into apache:main Apr 4, 2026
159 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

We do not respect ignoreNulls in first_value / last_value aggregates

3 participants