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

Lead and Lag window functions should support default value with datatype other than Int64 #9001

Merged
merged 1 commit into from
Jan 26, 2024

Conversation

viirya
Copy link
Member

@viirya viirya commented Jan 25, 2024

Which issue does this PR close?

Closes #8307.

Rationale for this change

Noticed this when reviewing #8989. Currently we only support default value with Int64 data type for Lead and Lag window functions. This patch enables other data types to be used as default value.

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 Physical Expressions sqllogictest SQL Logic Tests (.slt) labels Jan 25, 2024
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.

Looks good to me -- thanks @viirya

cc @mustafasrepo and @comphead

@@ -2364,6 +2364,16 @@ impl ScalarValue {
ScalarValue::try_from_array(&cast_arr, 0)
}

/// Try to cast this value to a ScalarValue of type `data_type`
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@simonvandel
Copy link
Contributor

Thanks! I think this is the fix to #8307, so you can use that issue number in the PR description.

@@ -2364,6 +2364,16 @@ impl ScalarValue {
ScalarValue::try_from_array(&cast_arr, 0)
}

/// Try to cast this value to a ScalarValue of type `data_type`
pub fn cast_to(&self, data_type: &DataType) -> Result<Self> {
Copy link
Contributor

@comphead comphead Jan 26, 2024

Choose a reason for hiding this comment

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

I was introducing the same method to overcome unnecessary string conversion in separate PR 👍

Copy link
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

@viirya viirya merged commit b3fe6aa into apache:main Jan 26, 2024
24 checks passed
@viirya
Copy link
Member Author

viirya commented Jan 26, 2024

Thank you @alamb @mustafasrepo @simonvandel @comphead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
physical-expr Physical Expressions sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lag and lead window functions does not accept compatible default value
5 participants