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

remove global metadata cache, refactor custom_metadata API #238

Merged
merged 9 commits into from
Sep 14, 2021
Merged

Conversation

jrevels
Copy link
Contributor

@jrevels jrevels commented Sep 12, 2021

essentially implements the approach described here #90 (comment)

I haven't run tests fully locally yet so may still have some lingering bugs. They pass 😎

I also haven't been super performance-conscious here so we should be careful to make sure regressions aren't being introduced.

Since we're doing a major bump for this anyway, I wonder whether it's a good time to fully delete some of the lingering deprecations here.

closes #90, #211

@jrevels jrevels marked this pull request as draft September 12, 2021 17:59
@codecov
Copy link

codecov bot commented Sep 12, 2021

Codecov Report

Merging #238 (7f74cdd) into main (7d59427) will increase coverage by 2.02%.
The diff coverage is 92.30%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #238      +/-   ##
==========================================
+ Coverage   83.79%   85.81%   +2.02%     
==========================================
  Files          26       26              
  Lines        3259     3265       +6     
==========================================
+ Hits         2731     2802      +71     
+ Misses        528      463      -65     
Impacted Files Coverage Δ
src/Arrow.jl 56.52% <ø> (-1.82%) ⬇️
src/ArrowTypes/src/ArrowTypes.jl 77.92% <ø> (+3.07%) ⬆️
src/arraytypes/bool.jl 88.70% <ø> (+1.61%) ⬆️
src/arraytypes/fixedsizelist.jl 84.53% <ø> (+4.12%) ⬆️
src/arraytypes/list.jl 92.24% <ø> (+3.87%) ⬆️
src/arraytypes/map.jl 100.00% <ø> (ø)
src/arraytypes/primitive.jl 84.31% <ø> (ø)
src/arraytypes/struct.jl 100.00% <ø> (+1.53%) ⬆️
src/arraytypes/unions.jl 94.32% <ø> (+2.12%) ⬆️
src/write.jl 94.57% <80.00%> (-0.06%) ⬇️
... and 22 more

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 7d59427...7f74cdd. Read the comment docs.

@jrevels jrevels marked this pull request as ready for review September 12, 2021 22:08
@jrevels
Copy link
Contributor Author

jrevels commented Sep 12, 2021

Flipping out of draft as tests pass and docs are updated (unless I missed anything).

Before merging, should we decide on:

Since we're doing a major bump for this anyway, I wonder whether it's a good time to fully delete some of the lingering deprecations here.

We also could do this in a separate PR before tagging

@jrevels jrevels requested a review from quinnj September 13, 2021 00:31
Copy link
Member

@ericphanson ericphanson left a comment

Choose a reason for hiding this comment

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

exciting! Didn't know about Base.ImmutableDict, cool.

src/write.jl Outdated Show resolved Hide resolved
Co-authored-by: Eric Hanson <5846501+ericphanson@users.noreply.github.com>
src/utils.jl Show resolved Hide resolved
Copy link
Member

@quinnj quinnj left a comment

Choose a reason for hiding this comment

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

LGTM; thanks for taking this on! I left one suggestion to simplify toiddict.

Co-authored-by: Jacob Quinn <quinn.jacobd@gmail.com>
src/utils.jl Outdated Show resolved Hide resolved
@jrevels
Copy link
Contributor Author

jrevels commented Sep 14, 2021

From Slack convo:

I'll merge this PR once CI passes and can open a follow-up later to delete the deprecated regions before we actually tag the v2.0 release.

@jrevels
Copy link
Contributor Author

jrevels commented Sep 14, 2021

Hmm. Looks like Julia 1.3 tests now fail because the foldl implementation of toidict relies on Base.ImmutableDict(::Pair, ::Pair) working, which I guess isn't defined on Julia 1.3

and the Julia 1.6 tests fail because foldl doesn't apply the given op on unary inputs (which is expected):

julia> foldl(Base.ImmutableDict, ("a" => "b",))
"a" => "b"

I think I'll just revert back to the original more verbose implementation, which also has that slight performance benefit anyway

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.

remove global metadata cache (OBJ_METADATA)
3 participants