Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

Validity taken into account when writing StructArray to json #511

Merged
merged 13 commits into from Oct 10, 2021

Conversation

VasanthakumarV
Copy link
Contributor

@VasanthakumarV VasanthakumarV commented Oct 8, 2021

Close #500

@VasanthakumarV VasanthakumarV marked this pull request as draft October 8, 2021 17:54
@codecov
Copy link

codecov bot commented Oct 8, 2021

Codecov Report

Merging #511 (ddf2e9f) into main (8a7f1d7) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #511      +/-   ##
==========================================
+ Coverage   80.17%   80.20%   +0.02%     
==========================================
  Files         381      381              
  Lines       23659    23722      +63     
==========================================
+ Hits        18969    19026      +57     
- Misses       4690     4696       +6     
Impacted Files Coverage Δ
src/io/json/write/serialize.rs 63.41% <100.00%> (+1.15%) ⬆️
tests/it/io/json/write.rs 100.00% <100.00%> (ø)
src/io/parquet/read/statistics/fixlen.rs 39.53% <0.00%> (-4.66%) ⬇️
src/io/ipc/read/reader.rs 82.20% <0.00%> (-1.42%) ⬇️
src/io/parquet/read/mod.rs 59.59% <0.00%> (-0.41%) ⬇️
src/io/parquet/read/schema/convert.rs 91.26% <0.00%> (-0.05%) ⬇️
src/array/ffi.rs 65.00% <0.00%> (ø)
src/array/list/mutable.rs 66.33% <0.00%> (ø)
src/compute/arithmetics/decimal/add.rs 83.09% <0.00%> (ø)
src/compute/arithmetics/decimal/div.rs 81.01% <0.00%> (ø)
... and 10 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 8a7f1d7...ddf2e9f. Read the comment docs.

Copy link
Contributor Author

@VasanthakumarV VasanthakumarV left a comment

Choose a reason for hiding this comment

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

@jorgecarleitao Should nullable from Field influence StructArray's validity?

tests/it/io/json/write.rs Outdated Show resolved Hide resolved
Copy link
Owner

@jorgecarleitao jorgecarleitao left a comment

Choose a reason for hiding this comment

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

Interesting approach. I would imagine that we do not need to combine the validities for this, and instead just write a null to the whole field, but maybe you already went through that exercise and deemed not feasible?

If yes, could you clarify this on the code with a comment? If not, I think it is worth the shot, since it would make the file smaller

tests/it/io/json/write.rs Outdated Show resolved Hide resolved
Copy link
Owner

@jorgecarleitao jorgecarleitao left a comment

Choose a reason for hiding this comment

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

This looks awesome, and very clean with the zip_validity 💯

Left two nits that clippy would complain.

src/io/json/write/serialize.rs Outdated Show resolved Hide resolved
src/io/json/write/serialize.rs Outdated Show resolved Hide resolved
@VasanthakumarV VasanthakumarV changed the title [WIP] Validity taken into account when writing StructArray to json Validity taken into account when writing StructArray to json Oct 10, 2021
@VasanthakumarV VasanthakumarV marked this pull request as ready for review October 10, 2021 11:21
@jorgecarleitao jorgecarleitao added the bug Something isn't working label Oct 10, 2021
@jorgecarleitao jorgecarleitao merged commit eead22f into jorgecarleitao:main Oct 10, 2021
@VasanthakumarV VasanthakumarV deleted the validity_serialize branch October 12, 2021 02:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

write to json should take validity into account when writing StructArray
2 participants