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

Arrow support #167

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Arrow support #167

wants to merge 10 commits into from

Conversation

omus
Copy link
Collaborator

@omus omus commented Jul 8, 2021

Replaces #156. Changes here include the commits from that PR and uses the updated serialization framework introduces in apache/arrow-julia#156

@omus
Copy link
Collaborator Author

omus commented Jul 8, 2021

I'm expecting Julia 1.0 to fail as ArrowTypes doesn't support that version of Julia currently

t = Arrow.Table(Arrow.tobuffer(table))

# Arrow.jl converts all Period types into Second
@test_broken eltype(t.col) == HourEnding{ZonedDateTime, Open, Closed}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll probably end up disabling AnchoredInterval support to start with because of this

Copy link
Member

@rofinn rofinn Jul 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't explicitly override this behaviour? Given how much data we have saved in HE format needing to save hour data as Intervals would be a significant performance hit and likely wouldn't make this PR worth it for us. Also, doesn't Arrow have it's own interval type which does the same thing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've forgotten a bunch about this PR but it should be possible to encode the period span in the serialized Arrow form. I do remember I was mainly focused on Interval support so I think this comment was more about that the implementation currently in place is incomplete.

@codecov
Copy link

codecov bot commented Jul 8, 2021

Codecov Report

Merging #167 (973e286) into master (f873f70) will increase coverage by 1.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #167      +/-   ##
==========================================
+ Coverage   81.09%   82.18%   +1.09%     
==========================================
  Files          11       12       +1     
  Lines         603      640      +37     
==========================================
+ Hits          489      526      +37     
  Misses        114      114              
Impacted Files Coverage Δ
src/Intervals.jl 100.00% <ø> (ø)
src/arrow.jl 100.00% <100.00%> (ø)
src/interval.jl 96.26% <0.00%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f873f70...973e286. Read the comment docs.

@omus
Copy link
Collaborator Author

omus commented Jul 8, 2021

I'm expecting Julia 1.0 to fail as ArrowTypes doesn't support that version of Julia currently

Fixing this in: apache/arrow-julia#223

Copy link
Collaborator Author

@omus omus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can remove depending on Requires.jl

src/Intervals.jl Outdated Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
@omus
Copy link
Collaborator Author

omus commented Jul 23, 2021

Should probably wait for apache/arrow-julia#229 which allows us some additional flexibility with how we encode Interval types.

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