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

Allow creating of TimeUnit instances without direct dependency on parquet-format #2708

Closed
pacman82 opened this issue Sep 12, 2022 · 4 comments
Labels
enhancement Any new improvement worthy of a entry in the changelog parquet Changes to the parquet crate

Comments

@pacman82
Copy link

Is your feature request related to a problem or challenge? Please describe what you are trying to do.

Maintaimer of the downstream odbc2parquet tool here. I would like to switch from converted type to logical types. One bump in the road of doing so is the fact that odbc2parquet now directly needs to depend also on the parquet-format crate instead of just parquet. The cause being, that it is impossible to create an instance of TimeUnit for the logical type, due to it being depended on e.g. MilliSeconds or MicroSeconds type of the parquet-format crate.

Describe the solution you'd like

I think I do not fully grasp why the current design has been choosen, so I am not particularly biased towards any solution. I could brainstorm a few ideas though:

  • Drop the MilliSeconds type tag from the TimeUnit::MILLIS variant.
  • Give TimeUnit constructors for milliseconds, microseconds, etc.
  • Reexport MilliSeconds, MicroSeconds, etc. ..

Describe alternatives you've considered

Currently I'll just add the extra direct dependency to parquet-format. If this is what you want me to do, consider this issue void.
Additional context

@pacman82 pacman82 added the enhancement Any new improvement worthy of a entry in the changelog label Sep 12, 2022
@alamb
Copy link
Contributor

alamb commented Sep 12, 2022

👋 I believe #2626 from @tustvold has removed the dependency entirely on parquet-format (by inlining the definitions) so hopefully you won't need any direct dependencies after the next release (tracked by #2665 )

@pacman82
Copy link
Author

@alamb Thanks for the information and quick response.
@tustvold Thanks for inlining.

Cheers, Markus

@alamb
Copy link
Contributor

alamb commented Oct 3, 2022

I believe this is now resolved

@alamb alamb closed this as completed Oct 3, 2022
@pacman82
Copy link
Author

pacman82 commented Oct 4, 2022

I believe this is now resolved

Yes it is. Already in use downstream. Thanks!

@tustvold tustvold added the parquet Changes to the parquet crate label Oct 4, 2022
@tustvold tustvold changed the title Allow creating of TimeUnit instances without directy dependency on parquet-format Allow creating of TimeUnit instances without direct dependency on parquet-format Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Any new improvement worthy of a entry in the changelog parquet Changes to the parquet crate
Projects
None yet
Development

No branches or pull requests

3 participants