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

Support RowTables #93

Merged
merged 1 commit into from
Jul 8, 2021
Merged

Support RowTables #93

merged 1 commit into from
Jul 8, 2021

Conversation

glennmoy
Copy link
Member

@glennmoy glennmoy commented Jun 21, 2021

Closes #64

Note that we cannot call apply! on rowtables since the elements of the NamedTuple are immutable in this instance.

e.g.

# can mutate the vector elements
coltable = NamedTuple{(:a, :b)}(a=[1, 2, 3], b=[4, 5, 6])

# cannot mutate the integer elements
rowtable = [
    NamedTuple{(:a, :b)}(a=1, b=4), 
    NamedTuple{(:a, :b)}(a=2, b=5), 
    NamedTuple{(:a, :b)}(a=3, b=6),
]

Converting to-from a columntable defeats the purpose as it wouldn't be mutating the underlying data in memory.
So we just don't support it.

@codecov
Copy link

codecov bot commented Jun 21, 2021

Codecov Report

Merging #93 (61ca2da) into main (cdb209a) will increase coverage by 0.07%.
The diff coverage is 100.00%.

❗ Current head 61ca2da differs from pull request most recent head 5c27b10. Consider uploading reports for the commit 5c27b10 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main      #93      +/-   ##
==========================================
+ Coverage   98.52%   98.60%   +0.07%     
==========================================
  Files          12       12              
  Lines         136      143       +7     
==========================================
+ Hits          134      141       +7     
  Misses          2        2              
Impacted Files Coverage Δ
src/apply.jl 100.00% <100.00%> (ø)

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 cdb209a...5c27b10. Read the comment docs.

test/types/tables.jl Outdated Show resolved Hide resolved
@glennmoy glennmoy force-pushed the gm/refactor_tests branch from 0d2e038 to 00376b7 Compare June 28, 2021 18:03
@glennmoy glennmoy force-pushed the gm/rowtable branch 2 times, most recently from f3f40e5 to e02dfa5 Compare June 28, 2021 18:35
Base automatically changed from gm/refactor_tests to main June 28, 2021 18:41
@glennmoy
Copy link
Member Author

reminder to bump the project

@glennmoy glennmoy requested a review from bencottier June 29, 2021 11:18
@glennmoy glennmoy force-pushed the gm/rowtable branch 2 times, most recently from bf5a235 to b6203ce Compare June 30, 2021 13:37
Copy link
Member

@mjram0s mjram0s left a comment

Choose a reason for hiding this comment

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

Just one comment but otherwise everything is pretty straightforward and LGTM

src/apply.jl Outdated
@@ -153,5 +171,5 @@ _postformat(::Cardinality, result, A, append_dim) = result
function _postformat(::ManyToOne, result, A, append_dim)
new_size = collect(size(A))
setindex!(new_size, 1, dim(A, append_dim))
return reshape(result, new_size...)
return copy(reshape(result, new_size...)) # return a copy to remove the reshape type
Copy link
Member

Choose a reason for hiding this comment

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

Was the reshape type a problem before? Just wondering why we weren't already returning a copy.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah it was a problem because calling parent on it later on removed the ReshapeArray type and returned the underlying AxisArray / KeyedArray (which is what we actually wanted to call parent on). So calling copy just performs that step in advance.

Note that we cannot call `apply!` on rowtables since the elements
of the NamedTuple rows are immutable in this instance.

e.g. row = NamedTuple{(:a, :b)}(a=1, b=2)

convert to-from a columntable defeats the purpose as it wouldn't be
mutating the underlying table.
@glennmoy glennmoy enabled auto-merge July 8, 2021 13:04
@glennmoy glennmoy merged commit 2c192ec into main Jul 8, 2021
@glennmoy glennmoy deleted the gm/rowtable branch July 8, 2021 13:07
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.

Tests do not check rowtables
2 participants