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

Removal of arithmetic operations for temporal values to binary.rs #5846

Merged
merged 2 commits into from
Apr 6, 2023
Merged

Removal of arithmetic operations for temporal values to binary.rs #5846

merged 2 commits into from
Apr 6, 2023

Conversation

berkaysynnada
Copy link
Contributor

Which issue does this PR close?

Closes #5704.

#5764 (comment)
#5764 (comment)

Rationale for this change

Before moving the temporal arithmetic to arrow-rs, it is more convenient to deploy them in binary.rs.

What changes are included in this PR?

the macros and arithmetic functions in datetime.rs are moved to binary.rs.

Are these changes tested?

yes, with existing tests.

Are there any user-facing changes?

@github-actions github-actions bot added the physical-expr Physical Expressions label Apr 3, 2023
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 @berkaysynnada

@berkaysynnada berkaysynnada changed the title removal of arithmetic operations for temporal values to binary.rs Removal of arithmetic operations for temporal values to binary.rs Apr 4, 2023
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.

What I am hoping we can do is to move the temporal logic into kernels such as

pub fn add_dyn(lhs: ArrayRef, rhs: ArrayRef) -> Result<ArrayRef> {
  match(lhs.data_type(), rhs.data_type()) {
    (DataType::Timestamp, DataType::Interval) => {
    // call timestamp + interval code
    },
    // handle other temporal types
    _ => {
      // fall back to kernels in arrow-rs
      arrow::compute::add_dyn(...)
    }
}

With this pattern, once the arrow kernel supports interval arithmetic, we can remove the shim datafusion.

We followed this pattern initially when implementing much of the decimal arithmetic, and it worked well I think

https://github.com/apache/arrow-datafusion/blob/main/datafusion/physical-expr/src/expressions/binary/kernels_arrow.rs

Is this something you can help with @berkaysynnada ?

@berkaysynnada
Copy link
Contributor Author

berkaysynnada commented Apr 5, 2023

What I am hoping we can do is to move the temporal logic into kernels such as

pub fn add_dyn(lhs: ArrayRef, rhs: ArrayRef) -> Result<ArrayRef> {
  match(lhs.data_type(), rhs.data_type()) {
    (DataType::Timestamp, DataType::Interval) => {
    // call timestamp + interval code
    },
    // handle other temporal types
    _ => {
      // fall back to kernels in arrow-rs
      arrow::compute::add_dyn(...)
    }
}

With this pattern, once the arrow kernel supports interval arithmetic, we can remove the shim datafusion.

We followed this pattern initially when implementing much of the decimal arithmetic, and it worked well I think

https://github.com/apache/arrow-datafusion/blob/main/datafusion/physical-expr/src/expressions/binary/kernels_arrow.rs

Is this something you can help with @berkaysynnada ?

I have other commitments currently. I would be happy to help when I have more time, but anyone interested in this issue can contact me when needed.

@alamb alamb merged commit aa41c27 into apache:main Apr 6, 2023
@berkaysynnada berkaysynnada deleted the feature/removal-of-temporal-arithmetic-to-binary branch April 7, 2023 07:55
@berkaysynnada berkaysynnada restored the feature/removal-of-temporal-arithmetic-to-binary branch April 7, 2023 11:12
@berkaysynnada berkaysynnada deleted the feature/removal-of-temporal-arithmetic-to-binary branch April 7, 2023 11:12
@berkaysynnada
Copy link
Contributor Author

What I am hoping we can do is to move the temporal logic into kernels such as

pub fn add_dyn(lhs: ArrayRef, rhs: ArrayRef) -> Result<ArrayRef> {
  match(lhs.data_type(), rhs.data_type()) {
    (DataType::Timestamp, DataType::Interval) => {
    // call timestamp + interval code
    },
    // handle other temporal types
    _ => {
      // fall back to kernels in arrow-rs
      arrow::compute::add_dyn(...)
    }
}

With this pattern, once the arrow kernel supports interval arithmetic, we can remove the shim datafusion.

We followed this pattern initially when implementing much of the decimal arithmetic, and it worked well I think

https://github.com/apache/arrow-datafusion/blob/main/datafusion/physical-expr/src/expressions/binary/kernels_arrow.rs

Is this something you can help with @berkaysynnada ?

@alamb would you mind taking a look at this PR when you have time? I tried to implement the changes you suggested. Since the only evaluation of array vs array operations are merged yet (array vs scalar implementation PR will be opened soon), I worked on them. The same approach can be applied to array vs scalar part after it is merged. Thanks for your feedback

@alamb
Copy link
Contributor

alamb commented Apr 11, 2023

I plan to review this PR today

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

Successfully merging this pull request may close these issues.

Timestamp and Interval arithmetics
3 participants