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.

Sign up for GitHub

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

Jump to bottom

Generify parquet write path (#1764) #2045

Merged
merged 8 commits into from
Jul 17, 2022
Merged

Generify parquet write path (#1764) #2045

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
aa72844
Generify parquet write path (#1764)
tustvold Jul 6, 2022
64e62c5
More docs
tustvold Jul 11, 2022
680bb77
Lint
tustvold Jul 12, 2022
2f961f2
Fix doc
tustvold Jul 12, 2022
42cca6b
Review feedback
tustvold Jul 16, 2022
1775f8f
Merge remote-tracking branch 'upstream/master' into generify-write-path
tustvold Jul 16, 2022
7df5199
Merge remote-tracking branch 'upstream/master' into generify-write-path
tustvold Jul 17, 2022
496bb70
Fix doc
tustvold Jul 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Review feedback
  • Loading branch information
@tustvold
tustvold committed Jul 16, 2022
commit 42cca6b0315e0de12d2e520ed242dc73ea932d10
4 changes: 3 additions & 1 deletion parquet/src/column/writer/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,6 @@ type ColumnCloseResult = (
/// Typed column writer for a primitive column.
pub type ColumnWriterImpl<'a, T> = GenericColumnWriter<'a, ColumnValueEncoderImpl<T>>;

#[doc(hidden)]
pub struct GenericColumnWriter<'a, E: ColumnValueEncoder> {
// Column writer properties
descr: ColumnDescPtr,
Expand Down Expand Up @@ -188,6 +187,9 @@ pub struct GenericColumnWriter<'a, E: ColumnValueEncoder> {
max_column_value: Option<E::T>,
num_column_nulls: u64,
column_distinct_count: Option<u64>,

/// The order of encodings within the generated metadata does not impact its meaning,
/// but we use a BTreeSet so that the output is deterministic
encodings: BTreeSet<Encoding>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this a BTreeSet (which requires Ord on Encoding) rather than a HashSet? Is there any reason the ordering is important?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It makes the output deterministic, which makes testing easier, but in terms of correctness, there should be no requirement for a particular order

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense -- Perhaps it is worth a comment explaining the rationale


// Reused buffers
Expand Down
1 change: 0 additions & 1 deletion parquet/src/file/statistics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,7 +398,6 @@ impl fmt::Display for Statistics {
/// Typed implementation for [`Statistics`].
pub type TypedStatistics<T> = ValueStatistics<<T as DataType>::T>;
Copy link
Contributor

Choose a reason for hiding this comment

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

I am sorry my rust-fu isn't good enough -- what is the purpose of renaming TypedStatistics to ValueStatistics (and doing the DataType::T nonsense in the typedef)? Is that needed to #[derive] works?

Copy link
Contributor Author

@tustvold tustvold Jul 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It is needed so you can create Statistics from a T: ParquetValueType, whereas previously it was being created (in a very hacky way) using DataType and casting via byte slices


#[doc(hidden)]
/// Statistics for a particular [`ParquetValueType`]
#[derive(Clone, Eq, PartialEq)]
pub struct ValueStatistics<T> {
Expand Down