Skip to content
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

fix(agg): fix first_value and last_value to not ignore NULLs #19332

Merged
merged 5 commits into from
Nov 13, 2024

Conversation

stdrc
Copy link
Member

@stdrc stdrc commented Nov 11, 2024

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

To align behavior with PG, we should not ignore NULLs when computing first_value and last_value.

This PR affects batch aggregate, batch over window. Also, a previous PR #19233 has already changed the behavior of streaming over window to the correct one.

Fixes #18927.

Checklist

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • I have added test labels as necessary. See details.
  • I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
  • My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
  • All checks passed in ./risedev check (or alias, ./risedev c)
  • My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)
  • My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

  • My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

  • Fix the behavior of first_value and last_value in batch aggregate query and both batch & streaming window function query to align with PostgreSQL, so that they don't ignore NULL values. This may be a breaking change if you have streaming jobs containing first_value/last_value window function calls and there are NULL values before.

Copy link
Member Author

stdrc commented Nov 11, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @stdrc and the rest of your teammates on Graphite Graphite

@stdrc stdrc changed the title fix first_value and last_value to not ignore NULLs fix(agg): fix first_value and last_value to not ignore NULLs Nov 11, 2024
@github-actions github-actions bot added type/fix Bug fix and removed Invalid PR Title labels Nov 11, 2024
@stdrc stdrc marked this pull request as ready for review November 11, 2024 09:47
@stdrc stdrc requested a review from xiangjinwu November 11, 2024 09:47
@stdrc stdrc force-pushed the rc/fix-first-last-value-2 branch from cbd44a1 to 088edaa Compare November 12, 2024 12:20
@stdrc stdrc requested a review from kwannoel November 12, 2024 12:20
@stdrc stdrc force-pushed the rc/fix-first-last-value-2 branch from 088edaa to 8e9b078 Compare November 12, 2024 12:52
@stdrc stdrc enabled auto-merge November 12, 2024 12:56
if let Some(state) = &state.0 {
state.clone()
} else {
None
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this lead to breaking changes? Since we ignore nulls previously.

Copy link
Contributor

Choose a reason for hiding this comment

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

If so we need to include a release note.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added

@stdrc stdrc force-pushed the rc/fix-first-last-value-2 branch from 8e9b078 to 7f5b7c7 Compare November 13, 2024 02:44
@stdrc stdrc force-pushed the rc/fix-first-last-value-2 branch from 7f5b7c7 to 155765d Compare November 13, 2024 03:28
@stdrc stdrc added the user-facing-changes Contains changes that are visible to users label Nov 13, 2024
@stdrc stdrc added this pull request to the merge queue Nov 13, 2024
Copy link
Member Author

stdrc commented Nov 13, 2024

Merge activity

  • Nov 12, 11:03 PM EST: Graphite couldn't merge this PR because it failed for an unknown reason (This repository has GitHub's merge queue enabled, which is currently incompatible with Graphite).

Merged via the queue into main with commit c21a771 Nov 13, 2024
33 of 34 checks passed
@stdrc stdrc deleted the rc/fix-first-last-value-2 branch November 13, 2024 04:38
github-actions bot pushed a commit that referenced this pull request Nov 13, 2024
github-merge-queue bot pushed a commit that referenced this pull request Nov 13, 2024
) (#19365)

Signed-off-by: Richard Chien <[email protected]>
Co-authored-by: Richard Chien <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need-cherry-pick-release-2.1 type/fix Bug fix user-facing-changes Contains changes that are visible to users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

last_value different result from mv and ad hoc query
3 participants