Skip to content

Conversation

@HuSen8891
Copy link
Contributor

Which issue does this PR close?

Closes #12815

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt) labels Oct 10, 2024
@jonathanc-n
Copy link
Contributor

@HuSen8891 This lgtm!

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @HuSen8891 and @jonathanc-n -- this looks great 🦾

2 NULL

query I
SELECT NTH_VALUE(tt0.v1, NULL) OVER (PARTITION BY tt0.v2 ORDER BY tt0.v1) FROM t AS tt0;
Copy link
Contributor

Choose a reason for hiding this comment

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

I double checked that without the change in this PR, this test panics as described in the ticket:

thread 'tokio-runtime-worker' panicked at datafusion/physical-expr/src/window/nth_value.rs:195:63:
attempt to subtract with overflow
stack backtrace:
   0: rust_begin_unwind
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/std/src/panicking.rs:665:5
   1: core::panicking::panic_fmt

@HuSen8891 HuSen8891 requested a review from alamb October 13, 2024 11:37
@alamb alamb merged commit 16589b5 into apache:main Oct 14, 2024
@alamb
Copy link
Contributor

alamb commented Oct 14, 2024

Thanks agin @HuSen8891 @jonathanc-n

hailelagi pushed a commit to hailelagi/datafusion that referenced this pull request Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Panic in nth_value window function (SQLancer)

3 participants