Skip to content

ensure null-counts are written for all-null columns #307

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

Merged
sunchao merged 1 commit into from
May 25, 2021
Merged

ensure null-counts are written for all-null columns #307

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
17 changes: 17 additions & 0 deletions parquet/src/arrow/arrow_writer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1560,4 +1560,21 @@ mod tests {
panic!("Statistics::Int64 missing")
}
}

#[test]
Copy link
Contributor Author

@crepererum crepererum May 17, 2021

Choose a reason for hiding this comment

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

I am not entirely happy with this test since it requires an arrow data structure to actually test an parquet-part. It would probably be nice to have a more "pure" test as well, however I wasn't able to formulate an easy one.

Copy link
Contributor

@alamb alamb May 18, 2021

Choose a reason for hiding this comment

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

I would say this looks more like an "integration" test rather than "unit" test (in the sense that you connect some higher level APIs and ensure the output is reasonable). I personally don't see any problems with this approach.

to remove the dependency on Arrow you would probably have to use the SerializedFileWriter API directly: https://docs.rs/parquet/4.0.0/parquet/file/writer/trait.FileWriter.html

But I think that would end up being quite a bit more code

fn statistics_null_counts_only_nulls() {
// check that null-count statistics for "only NULL"-columns are correct
let values = Arc::new(UInt64Array::from(vec![None, None]));
let file = one_column_roundtrip("null_counts", values, true);

// check statistics are valid
let reader = SerializedFileReader::new(file).unwrap();
let metadata = reader.metadata();
assert_eq!(metadata.num_row_groups(), 1);
let row_group = metadata.row_group(0);
assert_eq!(row_group.num_columns(), 1);
let column = row_group.column(0);
let stats = column.statistics().unwrap();
assert_eq!(stats.null_count(), 2);
}
}
4 changes: 3 additions & 1 deletion parquet/src/column/writer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -607,9 +607,11 @@ impl<T: DataType> ColumnWriterImpl<T> {
let max_def_level = self.descr.max_def_level();
let max_rep_level = self.descr.max_rep_level();

// always update column NULL count, no matter if page stats are used
self.num_column_nulls += self.num_page_nulls;
Copy link
Member

@sunchao sunchao May 19, 2021

Choose a reason for hiding this comment

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

hmm just curious why we don't need to update self.num_page_nulls accordingly, it is updated as follow:

            for &level in levels {
                if level == self.descr.max_def_level() {
                    values_to_write += 1;
                } else if calculate_page_stats {
                    self.num_page_nulls += 1
                }
            }

Copy link
Contributor Author

@crepererum crepererum May 20, 2021

Choose a reason for hiding this comment

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

There are two variables here: num_page_nulls (which IS updated) and num_column_nulls (which prior to this PR was only updated if min/max values were set, which cannot happen for all-NULL columns). And the latter one (num_column_nulls) will be used to write the stats.

Copy link
Member

@sunchao sunchao May 24, 2021

Choose a reason for hiding this comment

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

which IS updated

(Sorry for the slow response) This is the part I don't quite understand. add_data_page is called in two places: flush_data_pages and write_mini_batch. I can see this handles the first case but don't see how it applies for the second case too.

Copy link
Contributor Author

@crepererum crepererum May 25, 2021

Choose a reason for hiding this comment

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

I'm not sure I follow:

  • Is your point that self.num_column_nulls += self.num_page_nulls; should only be triggered during flush_data_pages, not during write_mini_batch?
  • Or is the question under which condition write_mini_batch calls add_data_page? For that: As far as I can tell it is NOT called during the test at least because of should_add_data_page decides not to do so (likely because of the small test case).

Copy link
Member

@sunchao sunchao May 25, 2021

Choose a reason for hiding this comment

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

I meant the second: write_mini_batch will call add_data_page if encoded data page size exceeds the limit. In this case it seems num_page_nulls and subsequently num_column_nulls will not be set.

Copy link
Contributor Author

@crepererum crepererum May 25, 2021

Choose a reason for hiding this comment

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

I've just checked:

#[test]
fn statistics_null_counts_big_mixed() {
    // check that null-count statistics work for larger data sizes
    let len = 1_000_000u64;
    let data: Vec<_> = (0..len).map(|x| if x % 2 == 0 {Some(x)} else {None}).collect();
    let values = Arc::new(UInt64Array::from(data));
    let file = one_column_roundtrip("null_counts_big_mixed", values, true);

    // check statistics are valid
    let reader = SerializedFileReader::new(file).unwrap();
    let metadata = reader.metadata();
    assert_eq!(metadata.num_row_groups(), 1);
    let row_group = metadata.row_group(0);
    assert_eq!(row_group.num_columns(), 1);
    let column = row_group.column(0);
    let stats = column.statistics().unwrap();
    assert_eq!(stats.null_count(), len / 2);
}

This triggers the add_data_page via write_mini_batch code path (with a small patch to the roundtrip test code to set a higher batch size). That works (with this PR) and both values are set. TBH that's what I expected since I don't really understand your reasoning behind "In this case it seems num_page_nulls and subsequently num_column_nulls will not be set.".

Copy link
Member

@sunchao sunchao May 25, 2021

Choose a reason for hiding this comment

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

Oops, you're right. I misread calculate_page_stats in the code. My apologies.


let page_statistics = if calculate_page_stat {
self.update_column_min_max();
self.num_column_nulls += self.num_page_nulls;
Some(self.make_page_statistics())
} else {
None
Expand Down