You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/02/16 09:22:38 UTC
[GitHub] [arrow-rs] shanisolomon opened a new pull request #1320: fix test bug and cc metadata to_thrift
shanisolomon opened a new pull request #1320:
URL: https://github.com/apache/arrow-rs/pull/1320
# Which issue does this PR close?
Closes #1319.
# Rationale for this change
test_column_chunk_metadata_thrift_conversion didn't check full thrift conversion. It only checked from_thrift as it compared two thrift objects.
I altered it to compare two `ColumnChunkMetaData` objects, thus checking both to_thrift and from_thrift.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] codecov-commenter commented on pull request #1320: fix test bug and cc metadata to_thrift
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #1320:
URL: https://github.com/apache/arrow-rs/pull/1320#issuecomment-1041301209
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1320?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#1320](https://codecov.io/gh/apache/arrow-rs/pull/1320?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8b928ab) into [master](https://codecov.io/gh/apache/arrow-rs/commit/747e72a0c3bf5771c7bfde7b09c5166c5aa51bc3?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (747e72a) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/1320?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #1320 +/- ##
=======================================
Coverage 83.01% 83.02%
=======================================
Files 180 180
Lines 52810 52811 +1
=======================================
+ Hits 43840 43844 +4
+ Misses 8970 8967 -3
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1320?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [parquet/src/file/metadata.rs](https://codecov.io/gh/apache/arrow-rs/pull/1320/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvZmlsZS9tZXRhZGF0YS5ycw==) | `92.50% <100.00%> (+1.10%)` | :arrow_up: |
| [arrow/src/datatypes/field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1320/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2RhdGF0eXBlcy9maWVsZC5ycw==) | `53.79% <0.00%> (-0.31%)` | :arrow_down: |
| [arrow/src/datatypes/datatype.rs](https://codecov.io/gh/apache/arrow-rs/pull/1320/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2RhdGF0eXBlcy9kYXRhdHlwZS5ycw==) | `66.80% <0.00%> (+0.39%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1320?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1320?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [747e72a...8b928ab](https://codecov.io/gh/apache/arrow-rs/pull/1320?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on a change in pull request #1320: fix test bug and cc metadata to_thrift
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1320:
URL: https://github.com/apache/arrow-rs/pull/1320#discussion_r807864414
##########
File path: parquet/src/file/metadata.rs
##########
@@ -533,7 +533,7 @@ impl ColumnChunkMetaData {
dictionary_page_offset: self.dictionary_page_offset,
statistics: statistics::to_thrift(self.statistics.as_ref()),
encoding_stats: None,
- bloom_filter_offset: None,
+ bloom_filter_offset: self.bloom_filter_offset,
Review comment:
👍
##########
File path: parquet/src/file/metadata.rs
##########
@@ -739,17 +739,15 @@ mod tests {
.set_total_uncompressed_size(3000)
.set_data_page_offset(4000)
.set_dictionary_page_offset(Some(5000))
+ .set_bloom_filter_offset(Some(6000))
.build()
.unwrap();
- let col_chunk_exp = col_metadata.to_thrift();
-
let col_chunk_res =
- ColumnChunkMetaData::from_thrift(column_descr, col_chunk_exp.clone())
- .unwrap()
- .to_thrift();
+ ColumnChunkMetaData::from_thrift(column_descr, col_metadata.to_thrift())
+ .unwrap();
- assert_eq!(col_chunk_res, col_chunk_exp);
+ assert_eq!(col_chunk_res, col_metadata);
Review comment:
I thought a little about if there was any value it keeping the check that `col_chunk_res.to_thrift()` and `col_metadata.to_thrift()` were also equal, but I think that given that this PR is deriving `PartialEq` on `ColumnChunkMetadata` there is no extra value in comparing the thrift serialized version
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb merged pull request #1320: fix test bug and ensure that bloom filter metadata is serialized in `to_thrift`
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #1320:
URL: https://github.com/apache/arrow-rs/pull/1320
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org