No longer write Parquet column metadata after column chunks and in the footer #6117

etseidl · 2024-07-25T17:35:41Z

Which issue does this PR close?

Closes #6115.

Rationale for this change

This metadata is now deprecated.

What changes are included in this PR?

Leaves file_offset as 0 and deprecates setting the field. Also stops writing an extra copy of ColumnMetaData.

Are there any user-facing changes?

Yes, since some metadata is no longer written (but considering it was incorrect, likely it is not widely used). Also deprecates ColumnChunkMetaDataBuilder::set_file_offset().

) * bump `tonic` to 0.12 and `prost` to 0.13 for `arrow-flight` Signed-off-by: Bugen Zhao <i@bugenzhao.com> * fix example tests Signed-off-by: Bugen Zhao <i@bugenzhao.com> --------- Signed-off-by: Bugen Zhao <i@bugenzhao.com>

…ally copies data (apache#6043) * deprecate auto copy, ask explicit reference * update comments * make cargo doc happy

* improve dispaly for interval. * update test in pretty, and fix display problem. * tmp * fix tests in arrow-cast. * fix tests in pretty. * fix style.

* update to latest thrift (as of 11 Jul 2024) from parquet-format * pass None for optional size statistics * escape HTML tags * don't need to escape brackets in arrays

… end (#…" (apache#5933) This reverts commit 22e0b44.

This reverts commit 756b1fb.

* Update pyo3 requirement from 0.21.1 to 0.22.1 Updates the requirements on [pyo3](https://github.com/pyo3/pyo3) to permit the latest version. - [Release notes](https://github.com/pyo3/pyo3/releases) - [Changelog](https://github.com/PyO3/pyo3/blob/main/CHANGELOG.md) - [Commits](PyO3/pyo3@v0.21.1...v0.22.1) --- updated-dependencies: - dependency-name: pyo3 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> * refactor: remove deprecated `FromPyArrow::from_pyarrow` "GIL Refs" are being phased out. * chore: update `pyo3` in integration tests --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* update to latest thrift (as of 11 Jul 2024) from parquet-format * pass None for optional size statistics * escape HTML tags * don't need to escape brackets in arrays * add support for unencoded_byte_array_data_bytes * add comments * change sig of ColumnMetrics::update_variable_length_bytes() * rename ParquetOffsetIndex to OffsetSizeIndex * rename some functions * suggestion from review Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org> * add Default trait to ColumnMetrics as suggested in review * rename OffsetSizeIndex to OffsetIndexMetaData --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

Updates the requirements on [pyo3](https://github.com/pyo3/pyo3) to permit the latest version. - [Release notes](https://github.com/pyo3/pyo3/releases) - [Changelog](https://github.com/PyO3/pyo3/blob/v0.22.2/CHANGELOG.md) - [Commits](PyO3/pyo3@v0.21.1...v0.22.2) --- updated-dependencies: - dependency-name: pyo3 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

…MetaData` (apache#6095) * deprecate read_page_locations * add to_thrift() to OffsetIndexMetaData

Co-authored-by: Ed Seidl <etseidl@users.noreply.github.com>

alamb · 2024-07-26T10:15:49Z

I merged the 53 dev branch ~~and that seems to have closed this PR~~ -- any chance you can retarget main?

Update: I restored the branch

alamb

Thank you @etseidl

I reviewed this carefully, not just the code but also the other uses of this field. I think this is a good change.

I did find a few other places that I think could be useful to deprecate / add some warnings / notes about. For example

https://github.com/apache/arrow-rs/blob/978264d49dc27225b570378e491ad69ae19f45cb/parquet/src/file/metadata/mod.rs#L685-L684

alamb · 2024-07-26T12:09:05Z

parquet/src/column/writer/mod.rs

@@ -1023,8 +1017,6 @@ impl<'a, E: ColumnValueEncoder> GenericColumnWriter<'a, E> {
        }

        let metadata = builder.build()?;
-        self.page_writer.write_metadata(&metadata)?;


This is the major functional change I think -- I expect it would result in slightly smaller parquet files as the per column metadata is no longer written twice

@wgtmac is here some impl using this metadata? Would a flag being better here? 🤔

I think this has been discussed in the ML and should be safe to remove this.

https://lists.apache.org/thread/r6r2cvzrdoorq6h6gqwh0b1hbfbhxv29
apache/parquet-format#440

Would you mind find the detail message for that? I'll linking it to C++ issues there

I think the best evidence that no readers use the copy outside the footer is this https://lists.apache.org/thread/ob2szsb0cv6fpwmvknss9jot1oqnxzsp

If any readers were using the alternate metadata they would have submitted bug reports by now 😉

I've sync with gang and I got the info that Parquet-Java doesn't write that and doesn't impl doesn't read that, let's rush forward

parquet/src/file/metadata/mod.rs

alamb · 2024-07-26T12:21:58Z

Can you please retarget this PR to the main branch and we'll get it merged in ?

jhorstmann · 2024-07-26T12:29:24Z

parquet/src/file/writer.rs

        let map_offset = |x| x - src_offset + write_offset as i64;
        let mut builder = ColumnChunkMetaData::builder(metadata.column_descr_ptr())
            .set_compression(metadata.compression())
            .set_encodings(metadata.encodings().clone())
-            .set_file_offset(file_offset)


Below in line 677 the metadata is still written to self.buf, should that also be skipped? And maybe the whole PageWriter::write_metadata method could be removed or deprecated?

Sorry, maybe this needs to be made clearer in the documentation. The spec still requires the field to be written so old readers don't break (i.e. it is still marked required in the thrift IDL). The correct behavior now is to write a 0 here.

Oh, I misunderstood your comment...the write below is correct as this is writing the copy of the ColumnMetaData in the footer. The change in this PR is to not also write a copy of ColumnMetaData at the end of each column chunk.

the write below is correct as this is writing the copy of the ColumnMetaData in the footer

The code is hard to follow, but my understanding is that this method copies an existing ColumnChunk from a ChunkReader. The buf seems to be the same that page data is written too, and I assume that append_column could be called multiple times . The metadata seems to also get collected inside the on_close closure and will in the end be written by SerializedFileWriter::write_metadata.

My understanding is that this code:

close.metadata = builder.build()?;

Sets the metadata that will be eventually written into the file footer.

This code, perhaps what @jhorstmann is referring to (the line numbers appear to have changed):

SerializedPageWriter::new(self.buf).write_metadata(&metadata)?;

Also seems to write the contents of close.metadata into the page.

In this case, it does seem like it could be avoided too

Ah, ok, I had somehow convinced myself that this method was in the normal write path and that the write in question was necessary :( I think I need to add a check that file_offset is 0 to several tests. And also check for gaps between column chunks.

Ok, that rippled a bit. 😅 Let me know if this is ok, or if write_metadata should be deprecated first.

I think removing write_metadata looks good to me

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

mapleFU · 2024-07-26T15:32:59Z

Does this pr remain un-related patches? I found this is hard to review 🤔

etseidl · 2024-07-26T15:36:50Z

Does this pr remain un-related patches? I found this is hard to review 🤔

Eek, I'm not quite sure what happened...let me try to merge with master and see if that clears things up. Thanks @mapleFU for pointing this out!

Ok, it's fixed now.

alamb

Thank you @etseidl and @jhorstmann -- I think this looks good to me. I'll leave this PR open for another day or two and then I plan to merge it

alamb · 2024-07-29T20:57:08Z

parquet/src/file/metadata/mod.rs

-    /// Byte offset in `file_path()`.
+    /// Byte offset of `ColumnMetaData` in `file_path()`.
+    ///
+    /// Note that the meaning of this field has been inconsistent between implementations


alamb · 2024-07-29T20:57:55Z

parquet/src/file/writer.rs

        let map_offset = |x| x - src_offset + write_offset as i64;
        let mut builder = ColumnChunkMetaData::builder(metadata.column_descr_ptr())
            .set_compression(metadata.compression())
            .set_encodings(metadata.encodings().clone())
-            .set_file_offset(file_offset)


I think removing write_metadata looks good to me

alamb · 2024-08-02T18:05:10Z

Thanks again 🚀

BugenZhao and others added 13 commits July 16, 2024 18:05

Remove impl<T: AsRef<[u8]>> From<T> for Buffer that easily accident…

8f76248

…ally copies data (apache#6043) * deprecate auto copy, ask explicit reference * update comments * make cargo doc happy

Make display of interval types more pretty (apache#6006)

bb5f12b

* improve dispaly for interval. * update test in pretty, and fix display problem. * tmp * fix tests in arrow-cast. * fix tests in pretty. * fix style.

Update snafu (apache#5930)

756b1fb

Update Parquet thrift generated structures (apache#6045)

fe04e09

* update to latest thrift (as of 11 Jul 2024) from parquet-format * pass None for optional size statistics * escape HTML tags * don't need to escape brackets in arrays

Revert "Revert "Write Bloom filters between row groups instead of the…

2e7f7ef

… end (#…" (apache#5933) This reverts commit 22e0b44.

Revert "Update snafu (apache#5930)" (apache#6069)

effccc1

This reverts commit 756b1fb.

remove repeated codes to make the codes more concise. (apache#6080)

05e681d

Deprecate read_page_locations() and simplify offset index in `Parquet…

3bc9987

…MetaData` (apache#6095) * deprecate read_page_locations * add to_thrift() to OffsetIndexMetaData

no longer write inline column metadata

20e11ec

github-actions bot added the parquet Changes to the parquet crate label Jul 25, 2024

alamb and others added 3 commits July 25, 2024 17:12

Merge remote-tracking branch 'apache/master' into 53.0.0-dev

095130f

Update parquet/src/column/writer/mod.rs

a6353d1

Co-authored-by: Ed Seidl <etseidl@users.noreply.github.com>

Merge remote-tracking branch 'origin/53.0.0-dev' into no_column_meta

d122b1f

alamb deleted the branch apache:master July 26, 2024 10:11

alamb closed this Jul 26, 2024

alamb reopened this Jul 26, 2024

alamb added the api-change Changes to the arrow API label Jul 26, 2024

alamb changed the title ~~No longer write inline Parquet column metadata~~ No longer write Parquet column metadata after column chunks *and* in the footer Jul 26, 2024

alamb approved these changes Jul 26, 2024

View reviewed changes

jhorstmann reviewed Jul 26, 2024

View reviewed changes

etseidl changed the base branch from 53.0.0-dev to master July 26, 2024 14:45

suggestion from review

957499d

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

github-actions bot added arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate labels Jul 26, 2024

add some more documentation

a033e43

Merge remote-tracking branch 'origin/master' into no_column_meta

571ce65

github-actions bot removed arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate labels Jul 26, 2024

etseidl added 2 commits July 26, 2024 13:22

Merge remote-tracking branch 'origin/master' into no_column_meta

07f9a1d

remove write_metadata from PageWriter

444b14f

alamb approved these changes Jul 29, 2024

View reviewed changes

alamb merged commit f2de2cd into apache:master Aug 2, 2024
16 checks passed

etseidl deleted the no_column_meta branch August 8, 2024 17:17

alamb mentioned this pull request Aug 31, 2024

ColumnMetaData should no longer be written inline with data #6115

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

No longer write Parquet column metadata after column chunks and in the footer #6117

No longer write Parquet column metadata after column chunks and in the footer #6117

etseidl commented Jul 25, 2024 •

edited

Loading

alamb commented Jul 26, 2024 •

edited

Loading

alamb left a comment

alamb Jul 26, 2024

mapleFU Jul 26, 2024

wgtmac Jul 26, 2024

mapleFU Jul 26, 2024

etseidl Jul 26, 2024

mapleFU Jul 26, 2024

alamb commented Jul 26, 2024

jhorstmann Jul 26, 2024

etseidl Jul 26, 2024 •

edited

Loading

jhorstmann Jul 27, 2024

alamb Jul 27, 2024

etseidl Jul 27, 2024 •

edited

Loading

etseidl Jul 28, 2024

alamb Jul 29, 2024

mapleFU commented Jul 26, 2024

etseidl commented Jul 26, 2024 •

edited

Loading

alamb left a comment

alamb Jul 29, 2024

alamb Jul 29, 2024

alamb commented Aug 2, 2024

No longer write Parquet column metadata after column chunks *and* in the footer #6117

No longer write Parquet column metadata after column chunks *and* in the footer #6117

Conversation

etseidl commented Jul 25, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

alamb commented Jul 26, 2024 • edited Loading

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Jul 26, 2024

Choose a reason for hiding this comment

etseidl Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etseidl Jul 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mapleFU commented Jul 26, 2024

etseidl commented Jul 26, 2024 • edited Loading

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Aug 2, 2024

No longer write Parquet column metadata after column chunks and in the footer #6117

No longer write Parquet column metadata after column chunks and in the footer #6117

etseidl commented Jul 25, 2024 •

edited

Loading

alamb commented Jul 26, 2024 •

edited

Loading

etseidl Jul 26, 2024 •

edited

Loading

etseidl Jul 27, 2024 •

edited

Loading

etseidl commented Jul 26, 2024 •

edited

Loading