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

Move the to_timestamp* functions to datafusion-functions #9291

Closed
Tracked by #9285
Omega359 opened this issue Feb 20, 2024 · 4 comments · Fixed by #9388
Closed
Tracked by #9285

Move the to_timestamp* functions to datafusion-functions #9291

Omega359 opened this issue Feb 20, 2024 · 4 comments · Fixed by #9388
Assignees
Labels
enhancement New feature or request

Comments

@Omega359
Copy link
Contributor

Is your feature request related to a problem or challenge?

As part of #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 #9286 & #9216,

Describe alternatives you've considered

No response

Additional context

No response

@Omega359 Omega359 added the enhancement New feature or request label Feb 20, 2024
@Omega359
Copy link
Contributor Author

take

@Omega359
Copy link
Contributor Author

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.

@alamb
Copy link
Contributor

alamb commented Feb 26, 2024

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.

@Omega359
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants