Skip to content

unify cast_to function of ScalarValue - #13122

Merged
alamb merged 1 commit into
apache:mainfrom
JasonLi-cn:feature/unify_ScalarValue_cast_to
Oct 29, 2024
Merged

alamb merged 1 commit into
apache:mainfrom
JasonLi-cn:feature/unify_ScalarValue_cast_to

Conversation

@JasonLi-cn

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes #.

Rationale for this change

Both ColumnarValue and ScalarValue contain the cast_to function, but they are inconsistent:

ColumnarValue::Scalar(scalar) => {
let scalar_array =
if cast_type == &DataType::Timestamp(TimeUnit::Nanosecond, None) {
if let ScalarValue::Float64(Some(float_ts)) = scalar {
ScalarValue::Int64(Some(
(float_ts * 1_000_000_000_f64).trunc() as i64,
))
.to_array()?
} else {
scalar.to_array()?
}
} else {
scalar.to_array()?
};
let cast_array = kernels::cast::cast_with_options(
&scalar_array,
cast_type,
&cast_options,
)?;
let cast_scalar = ScalarValue::try_from_array(&cast_array, 0)?;
Ok(ColumnarValue::Scalar(cast_scalar))

pub fn cast_to(&self, data_type: &DataType) -> Result<Self> {
let cast_options = CastOptions {
safe: false,
format_options: Default::default(),
};
let cast_arr = cast_with_options(&self.to_array()?, data_type, &cast_options)?;
ScalarValue::try_from_array(&cast_arr, 0)
}

What changes are included in this PR?

unify cast_to of ColumnarValue and ScalarValue.

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions Bot added the common Related to common crate label Oct 26, 2024

@alamb alamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you @JasonLi-cn -- this makes sense to me. Nice find

@alamb

alamb commented Oct 29, 2024

Copy link
Copy Markdown
Contributor

🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

common Related to common crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants