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

Implemented Adbc.Column.interval/3 #84

Merged
merged 6 commits into from
May 20, 2024
Merged

Implemented Adbc.Column.interval/3 #84

merged 6 commits into from
May 20, 2024

Conversation

cocoa-xu
Copy link
Member

No description provided.

@josevalim
Copy link
Member

Wait, so they have both duration and interval? :(

In any case, it seems interval better mirrors Elixir's duration, although we don't have to support it right now!

@cocoa-xu
Copy link
Member Author

cocoa-xu commented May 20, 2024

In any case, it seems interval better mirrors Elixir's duration,

Yeah, I was think about to use Duration but my main hesitancy with using Duration is that Arrow's interval would only use some components, i.e., for data type NANOARROW_TYPE_INTERVAL_MONTH, the result value will only use the month field, while for the NANOARROW_TYPE_INTERVAL_DAY_TIME type, the result value will only use day and microsecond.

The worst part is, there's no conversion rule in arrow to convert from one type of interval to another -- which means if the user passes Adbc.Column.interval(Duration.new!(day: 60), :month) to us, it will turn out to be 0.

To address this, we might either note it in the inline docs. But some users are probably going to be confused why there is data loss if they assumed there're some kind of conversions. Or we can check every Duration and raise if unused fields are not 0 -- however this definitely has a performance penalty.

although we don't have to support it right now

Let's let future us deal with this XD

@cocoa-xu cocoa-xu merged commit af19e47 into main May 20, 2024
3 checks passed
@cocoa-xu cocoa-xu deleted the cx-interval branch May 20, 2024 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants