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

Add a make_date function #9040

Merged
merged 3 commits into from
Jan 30, 2024
Merged

Add a make_date function #9040

merged 3 commits into from
Jan 30, 2024

Conversation

Omega359
Copy link
Contributor

Which issue does this PR close?

Closes #9024

Are these changes tested?

Yes, in code and sql logic tests.

Are there any user-facing changes?

A new make_date function is available, documented in user guide with examples in the datafusion-examples module.

@github-actions github-actions bot added logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt) labels Jan 29, 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.

Thank you @Omega359 -- this is a really nice contribution. Well documented and well tested.

The implementation also seems to match the postgres documentation https://www.postgresql.org/docs/current/functions-datetime.html

🏆

let months = cast_column(&args[1], &DataType::Int32, None)?;
let days = cast_column(&args[2], &DataType::Int32, None)?;

let value_fn = |col: &ColumnarValue, pos: usize| -> Result<i32> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This could probably be implemented more efficiently with some sort of iterator that wrapped a ColumnarValue and returned i32s -- that way you could iterate over all three arrays with zip and not have to do the downcast eah time, etc

-- and then you could generate specialized implementations for ColumnarValue::Array and ColumnarValue::Scalar

However, given make_date is likely to be run with all constants I don't think there is any pressing need to optimize the implementation unless someone has a particular need for a faster implementation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially I was tempted to try adding an IntoIterator for ColumnarValue that accepted an optional size (for scalar) which returns an option of either the array value at the current pos or the scalar value. That seemed a bit much given my current Rust skill level though.

Comment on lines +559 to +564
if m.is_err() {
return exec_err!(
"Month value '{:?}' is out of range",
value_fn(&months, i).unwrap()
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can write this logic like so to avoid the unwraps below:

Suggested change
if m.is_err() {
return exec_err!(
"Month value '{:?}' is out of range",
value_fn(&months, i).unwrap()
);
}
let Ok(m) = m else {
return exec_err!(
"Month value '{:?}' is out of range",
value_fn(&months, i).unwrap()
);
};

Copy link
Contributor

Choose a reason for hiding this comment

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

Proposed PR to do this in #9072

@Omega359 Omega359 changed the title Add a make_date function #9024 Add a make_date function Jan 30, 2024
@alamb alamb merged commit 78447d6 into apache:main Jan 30, 2024
24 checks passed
@alamb
Copy link
Contributor

alamb commented Jan 30, 2024

Thanks again @Omega359

I made a small PR here to remove the unwraps: #9072

Looking more carefully at the code in datetime_expressions.rs think we can probably remove more of the unwraps by using Result::map_err, but I think that is a matter of style (though it would some some runtime checks too)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
logical-expr Logical plan and expressions 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.

Add a make_date function
2 participants