Skip to content

Conversation

@avantgardnerio
Copy link
Contributor

Which issue does this PR close?

Closes this comment #3110 (comment)

Rationale for this change

We added code that is no longer needed, thanks to upstream merges in arrow-rs & chrono

What changes are included in this PR?

Removal of temporary code.

Are there any user-facing changes?

The requirement to have a newer chrono.

@github-actions github-actions bot added the physical-expr Changes to the physical-expr crates label Aug 15, 2022
@avantgardnerio
Copy link
Contributor Author

@alamb heads up!

@alamb
Copy link
Contributor

alamb commented Aug 15, 2022

Looks great -- the CI hasn't yet run on this PR, but when it does I am feeling good about it 👍

@alamb
Copy link
Contributor

alamb commented Aug 15, 2022

Screen Shot 2022-08-15 at 2 50 31 PM

🤔 github claims it has conflicts -- maybe it needs a rebase against master?

@avantgardnerio
Copy link
Contributor Author

avantgardnerio commented Aug 15, 2022

thinking github claims it has conflicts -- maybe it needs a rebase against master?

Unfortunately due to the implementation of this:

https://github.com/apache/arrow-datafusion/blob/15a9a4becc4bb41262b10f1d5395fc1d026de753/datafusion/physical-expr/src/expressions/datetime.rs#L295

I think we have to wait for this:

chronotope/chrono#778

So that we can do this:

fn do_date_math<D>(prior: D, scalar: &ScalarValue, sign: i32) -> Result<D>
where
    D: Datelike + Add<Duration, Output = D> + Add<Months, Output = D>,

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants