Skip to content

Add a make_date function - #9040

Merged
alamb merged 3 commits into
apache:mainfrom
Omega359:feature/9024_make_date
Jan 30, 2024
Merged

Add a make_date function#9040
alamb merged 3 commits into
apache:mainfrom
Omega359:feature/9024_make_date

Conversation

@Omega359

Copy link
Copy Markdown
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-actionsgithub-actionsBot added logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt) labels Jan 29, 2024

@alambalamb 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 @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

🏆

Comment threaddatafusion/physical-expr/src/datetime_expressions.rs
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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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()
);
}
letOk(m) = m else{
return exec_err!(
"Month value '{:?}' is out of range",
value_fn(&months, i).unwrap()
);
};

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.

Proposed PR to do this in #9072

Comment threaddatafusion/sqllogictest/test_files/timestamps.slt
@Omega359Omega359 changed the title Add a make_date function #9024 Add a make_date functionJan 30, 2024
@alamb
alamb merged commit 78447d6 into apache:mainJan 30, 2024
@alamb

Copy link
Copy Markdown
Contributor

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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

logical-exprLogical plan and expressionsphysical-exprChanges to the physical-expr cratessqllogictestSQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a make_date function

2 participants

@Omega359@alamb