datafusion icon indicating copy to clipboard operation
datafusion copied to clipboard

Move the to_timestamp* functions to datafusion-functions

Open Omega359 opened this issue 1 year ago • 2 comments

Is your feature request related to a problem or challenge?

As part of https://github.com/apache/arrow-datafusion/issues/9285 the to_timestamp* datetime functions should be migrated to the new datafusion-functions crate in the new structure

Describe the solution you'd like

Functions are migrated to the new crate and use the new patterns as described in https://github.com/apache/arrow-datafusion/issues/9286 & https://github.com/apache/arrow-datafusion/pull/9216,

Describe alternatives you've considered

No response

Additional context

No response

Omega359 avatar Feb 20 '24 14:02 Omega359

take

Omega359 avatar Feb 20 '24 14:02 Omega359

Not sure what to do about references to to_timestamp function in test code such as

  • core/tests/parquet/row_group_pruning.rs
  • core/tests/parquet/page_pruning.rs
  • optimizer/simplify_expressions/simplify_exprs.rs

This seems mostly to be in tests however I'm unsure of the wisdom of adding a dependency to datafusion-functions in these modules (core/optimizer/etc)

The issue really isn't limited to to_timestamp functions - any tests in those crates that use functions that we move to the new crate will have the same issue.

Omega359 avatar Feb 21 '24 20:02 Omega359

Not sure what to do about references to to_timestamp function in test code such as

  • core/tests/parquet/row_group_pruning.rs
  • core/tests/parquet/page_pruning.rs

I think the core tests will be fine as they should already have access to the (core) datafusion crate and thus to the functions in question

  • optimizer/simplify_expressions/simplify_exprs.rs

This seems mostly to be in tests however I'm unsure of the wisdom of adding a dependency to datafusion-functions in these modules (core/optimizer/etc)

I agree we should avoid this dependency if possible

One potential way to fix this dependency is to move most of the actual tests out of optimizer/simplify_expressions/simplify_exprs.rs and into core/tests/ somewhere -- perhaps into https://github.com/apache/arrow-datafusion/blob/a9f0c7afe2ea3bb14bfcceedfcd33cfe4b954035/datafusion/core/tests/simplification.rs#L1-L0

Many of those simplify tests seem to be tests that certain functions get simplified rather than a unit test of the optimizer. Thus they make more sense to be in the core crate from my perspective.

What do you think @Omega359 ? I am sorry for the late response

The issue really isn't limited to to_timestamp functions - any tests in those crates that use functions that we move to the new crate will have the same issue.

alamb avatar Feb 26 '24 20:02 alamb

Thanks @alamb, I'll look at moving those tests out to the core/tests/ area.

Omega359 avatar Feb 27 '24 00:02 Omega359