Skip to content

handle 0 and NULL value of NTH_VALUE function#12676

Merged
comphead merged 3 commits intoapache:mainfrom
thinh2:n-th-value-null-handler
Oct 1, 2024
Merged

handle 0 and NULL value of NTH_VALUE function#12676
comphead merged 3 commits intoapache:mainfrom
thinh2:n-th-value-null-handler

Conversation

@thinh2
Copy link
Copy Markdown
Contributor

@thinh2 thinh2 commented Sep 30, 2024

Which issue does this PR close?

Closes #12320.

Rationale for this change

The root cause in #12320 is because get_signed_integer_value will return an error on NULL or Int64(NULL) value. To process the nth_value function, it requires the n value should be an integer. Therefore, I decided to convert NULL value to 0 in get_signed_integer_value and implement handler for the 0 case. This will evaluate to NULL value for the nth_value(v1, NULL) expression.

Furthermore, it also evaluate the nth_value(v1, 0) to NULL instead of generating panic, which match the expected behaviour in duckdb.

What changes are included in this PR?

[x] Return 0 for NULL value in get_signed_integer
[x] Handle nth_value execution when n is 0
[x] Test file updated to test nth_value(v2, 0), nth_value(v2, NULL) and nth_value(v2, v2 * NULL).

Are these changes tested?

Yes

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 Sep 30, 2024
Copy link
Copy Markdown
Contributor

@jayzhan211 jayzhan211 left a comment

Choose a reason for hiding this comment

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

👍

Comment thread datafusion/physical-plan/src/windows/mod.rs Outdated
@thinh2 thinh2 requested a review from comphead October 1, 2024 16:55
Copy link
Copy Markdown
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

lgtm thanks @thinh2

@comphead comphead merged commit f54712d into apache:main Oct 1, 2024
@thinh2 thinh2 deleted the n-th-value-null-handler branch October 1, 2024 17:11
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.

Bug in nth_value() window function for NULL input (SQLancer)

3 participants