You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "Jefffrey (via GitHub)" <gi...@apache.org> on 2023/11/28 10:58:09 UTC
[PR] Parquet: clear metadata of ParquetRecordBatchStream::schema [arrow-rs]
Jefffrey opened a new pull request, #5135:
URL: https://github.com/apache/arrow-rs/pull/5135
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #4023
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
Ensure metadata hashmap of Schema from `ParquetRecordBatchStream::schema` is empty, to ensure exact same schema as for its RecordBatches it outputs.
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
--
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
Re: [PR] Parquet: clear metadata of ParquetRecordBatchStream::schema [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5135:
URL: https://github.com/apache/arrow-rs/pull/5135#discussion_r1408353398
##########
parquet/src/arrow/async_reader/mod.rs:
##########
@@ -572,7 +572,15 @@ impl<T> std::fmt::Debug for ParquetRecordBatchStream<T> {
}
impl<T> ParquetRecordBatchStream<T> {
- /// Returns the [`SchemaRef`] for this parquet file
+ /// Returns the [`SchemaRef`] for this parquet file.
+ ///
+ /// Note that unlike its synchronous counterpart [`ParquetRecordBatchReader`], the [`SchemaRef`]
+ /// returned here will contain the original metadata, whereas [`ParquetRecordBatchReader`]
+ /// strips this metadata.
Review Comment:
Oh this is the stream not the builder :facepalm:
Yeah we should probably be returning the same schema as the RecordBatch we return, sorry for the confusion...
--
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
Re: [PR] Parquet: clear metadata of ParquetRecordBatchStream::schema [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #5135:
URL: https://github.com/apache/arrow-rs/pull/5135#discussion_r1408272866
##########
parquet/src/arrow/async_reader/mod.rs:
##########
@@ -572,7 +572,15 @@ impl<T> std::fmt::Debug for ParquetRecordBatchStream<T> {
}
impl<T> ParquetRecordBatchStream<T> {
- /// Returns the [`SchemaRef`] for this parquet file
+ /// Returns the [`SchemaRef`] for this parquet file.
+ ///
+ /// Note that unlike its synchronous counterpart [`ParquetRecordBatchReader`], the [`SchemaRef`]
+ /// returned here will contain the original metadata, whereas [`ParquetRecordBatchReader`]
+ /// strips this metadata.
Review Comment:
Yes, see how schema is created for `ParquetRecordBatchReader` here:
https://github.com/apache/arrow-rs/blob/093a10e46203be1a0e94ae117854701bf58d4c79/parquet/src/arrow/arrow_reader/mod.rs#L609
And here:
https://github.com/apache/arrow-rs/blob/093a10e46203be1a0e94ae117854701bf58d4c79/parquet/src/arrow/arrow_reader/mod.rs#L622-L625
They only account for the fields, ignoring the metadata.
--
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
Re: [PR] Parquet: clear metadata of ParquetRecordBatchStream::schema [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5135:
URL: https://github.com/apache/arrow-rs/pull/5135#discussion_r1409117069
##########
parquet/src/arrow/async_reader/mod.rs:
##########
@@ -385,13 +385,28 @@ impl<T: AsyncFileReader + Send + 'static> ParquetRecordBatchStreamBuilder<T> {
offset: self.offset,
};
+ // Ensure schema of ParquetRecordBatchStream respects projection, and does
+ // not store metadata (same as for ParquetRecordBatchReader and emitted RecordBatches)
+ let projected_fields = match reader.fields.as_deref().map(|pf| &pf.arrow_type) {
Review Comment:
I'll have a quick play, stand by
--
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
Re: [PR] Parquet: clear metadata of ParquetRecordBatchStream::schema [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5135:
URL: https://github.com/apache/arrow-rs/pull/5135#discussion_r1409109232
##########
parquet/src/arrow/async_reader/mod.rs:
##########
@@ -385,13 +385,28 @@ impl<T: AsyncFileReader + Send + 'static> ParquetRecordBatchStreamBuilder<T> {
offset: self.offset,
};
+ // Ensure schema of ParquetRecordBatchStream respects projection, and does
+ // not store metadata (same as for ParquetRecordBatchReader and emitted RecordBatches)
+ let projected_fields = match reader.fields.as_deref().map(|pf| &pf.arrow_type) {
Review Comment:
Although https://docs.rs/parquet/latest/parquet/arrow/fn.parquet_to_arrow_field_levels.html
takes a ProjectionMask and should apply it already?
--
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
Re: [PR] Parquet: clear metadata of ParquetRecordBatchStream::schema [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #5135:
URL: https://github.com/apache/arrow-rs/pull/5135#discussion_r1409109867
##########
parquet/src/arrow/async_reader/mod.rs:
##########
@@ -385,13 +385,28 @@ impl<T: AsyncFileReader + Send + 'static> ParquetRecordBatchStreamBuilder<T> {
offset: self.offset,
};
+ // Ensure schema of ParquetRecordBatchStream respects projection, and does
+ // not store metadata (same as for ParquetRecordBatchReader and emitted RecordBatches)
+ let projected_fields = match reader.fields.as_deref().map(|pf| &pf.arrow_type) {
Review Comment:
I was a bit worried about this, as I couldn't find a straightforward way that the schema was constructed from ParquetField + ProjectionMask, as it seems done in the ArrayReader construction indeed.
--
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
Re: [PR] Parquet: clear metadata of ParquetRecordBatchStream::schema [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5135:
URL: https://github.com/apache/arrow-rs/pull/5135#discussion_r1407879147
##########
parquet/src/arrow/async_reader/mod.rs:
##########
@@ -572,7 +572,15 @@ impl<T> std::fmt::Debug for ParquetRecordBatchStream<T> {
}
impl<T> ParquetRecordBatchStream<T> {
- /// Returns the [`SchemaRef`] for this parquet file
+ /// Returns the [`SchemaRef`] for this parquet file.
+ ///
+ /// Note that unlike its synchronous counterpart [`ParquetRecordBatchReader`], the [`SchemaRef`]
+ /// returned here will contain the original metadata, whereas [`ParquetRecordBatchReader`]
+ /// strips this metadata.
Review Comment:
Are you sure about this, they use the same logic? The difference is that the returned RecordBatch lack the metadata
--
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
Re: [PR] Parquet: clear metadata of ParquetRecordBatchStream::schema [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5135:
URL: https://github.com/apache/arrow-rs/pull/5135#discussion_r1408353398
##########
parquet/src/arrow/async_reader/mod.rs:
##########
@@ -572,7 +572,15 @@ impl<T> std::fmt::Debug for ParquetRecordBatchStream<T> {
}
impl<T> ParquetRecordBatchStream<T> {
- /// Returns the [`SchemaRef`] for this parquet file
+ /// Returns the [`SchemaRef`] for this parquet file.
+ ///
+ /// Note that unlike its synchronous counterpart [`ParquetRecordBatchReader`], the [`SchemaRef`]
+ /// returned here will contain the original metadata, whereas [`ParquetRecordBatchReader`]
+ /// strips this metadata.
Review Comment:
Oh this is the stream not the builder :facepalm:
Yeah we should probably be returning the same schema as the RecordBatch we return, sorry for the confusion... Could you revert your revert, sorry too many plates at the moment :sweat_smile:
--
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
Re: [PR] Parquet: clear metadata of ParquetRecordBatchStream::schema [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5135:
URL: https://github.com/apache/arrow-rs/pull/5135#issuecomment-1830712054
> I'll revert back to the initial change, as well as account for projection which I wasn't aware it didn't account for either
I think if you determine the schema from the ParquetField on ReaderFactory (which should be a DataType::StructArray) I think you might be able to get both in one
--
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
Re: [PR] Parquet: clear metadata of ParquetRecordBatchStream::schema [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5135:
URL: https://github.com/apache/arrow-rs/pull/5135#discussion_r1408353398
##########
parquet/src/arrow/async_reader/mod.rs:
##########
@@ -572,7 +572,15 @@ impl<T> std::fmt::Debug for ParquetRecordBatchStream<T> {
}
impl<T> ParquetRecordBatchStream<T> {
- /// Returns the [`SchemaRef`] for this parquet file
+ /// Returns the [`SchemaRef`] for this parquet file.
+ ///
+ /// Note that unlike its synchronous counterpart [`ParquetRecordBatchReader`], the [`SchemaRef`]
+ /// returned here will contain the original metadata, whereas [`ParquetRecordBatchReader`]
+ /// strips this metadata.
Review Comment:
Oh this is the stream not the builder :facepalm:
Yeah we should probably be returning the same schema as the RecordBatch we return, sorry for the confusion... Could you revert your revert, sorry too many plates at the moment :sweat_smile:
--
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
Re: [PR] Parquet: clear metadata of ParquetRecordBatchStream::schema [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #5135:
URL: https://github.com/apache/arrow-rs/pull/5135#issuecomment-1831651742
Re-implemented the metadata stripping for `ParquetRecordBatchStream::schema` and also ensure it respects the projected columns
--
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
Re: [PR] Parquet: clear metadata of ParquetRecordBatchStream::schema [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5135:
URL: https://github.com/apache/arrow-rs/pull/5135#issuecomment-1830663668
Ok sorry for going back and forth on this, I hadn't quite grasped what the issue here was
```
#[tokio::test]
async fn test_projection_schema() {
let mut metadata = HashMap::with_capacity(1);
metadata.insert("key".to_string(), "value".to_string());
let schema = Arc::new(
Schema::new(Fields::from(vec![
Field::new("a", DataType::Int32, true),
Field::new("c", DataType::UInt64, true),
Field::new("d", DataType::Float32, true),
]))
.with_metadata(metadata.clone()),
);
let mut file = tempfile().unwrap();
let mut writer = ArrowWriter::try_new(&mut file, schema.clone(), None).unwrap();
writer
.write(&RecordBatch::new_empty(schema.clone()))
.unwrap();
writer.close().unwrap();
let builder = ParquetRecordBatchReaderBuilder::try_new(file.try_clone().unwrap()).unwrap();
let sync_file_schema = builder.schema().clone();
let mask = ProjectionMask::leaves(&builder.parquet_schema(), [1, 2]);
let reader = builder.with_projection(mask).build().unwrap();
let sync_reader_schema = reader.schema();
assert_eq!(sync_file_schema.fields.len(), 3);
assert_eq!(sync_file_schema.metadata, metadata);
assert_eq!(sync_reader_schema.fields.len(), 2);
assert_eq!(sync_reader_schema.metadata, HashMap::default());
let file = tokio::fs::File::from(file);
let builder = ParquetRecordBatchStreamBuilder::new(file).await.unwrap();
let async_file_schema = builder.schema().clone();
let mask = ProjectionMask::leaves(&builder.parquet_schema(), [1, 2]);
let reader = builder.with_projection(mask).build().unwrap();
let async_reader_schema = reader.schema();
assert_eq!(async_file_schema.fields.len(), 3);
assert_eq!(async_file_schema.metadata, metadata);
assert_eq!(async_reader_schema.fields.len(), 2);
assert_eq!(async_reader_schema.metadata, HashMap::default());
}
```
I think demonstrates the issue, in particular the schema returned by ParquetRecordBatchStream is incorrect, it should return the projected schema with the metadata removed.
--
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
Re: [PR] Parquet: clear metadata of ParquetRecordBatchStream::schema [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5135:
URL: https://github.com/apache/arrow-rs/pull/5135#discussion_r1409105685
##########
parquet/src/arrow/async_reader/mod.rs:
##########
@@ -385,13 +385,28 @@ impl<T: AsyncFileReader + Send + 'static> ParquetRecordBatchStreamBuilder<T> {
offset: self.offset,
};
+ // Ensure schema of ParquetRecordBatchStream respects projection, and does
+ // not store metadata (same as for ParquetRecordBatchReader and emitted RecordBatches)
+ let projected_fields = match reader.fields.as_deref().map(|pf| &pf.arrow_type) {
Review Comment:
This isn't quite correct in the case of a projection of a nested schema, its possible this isn't actually determined until the ArrayReader is constructed...
--
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
Re: [PR] Parquet: clear metadata of ParquetRecordBatchStream::schema [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #5135:
URL: https://github.com/apache/arrow-rs/pull/5135#issuecomment-1833689525
I've reworked it to make use of the function introduced by #5149
Hopefully I've used it correctly here
--
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
Re: [PR] Parquet: clear metadata of ParquetRecordBatchStream::schema [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #5135:
URL: https://github.com/apache/arrow-rs/pull/5135#issuecomment-1829732851
> Whilst I agree with the spirit of this, stripping this metadata would be a major breaking change that would definitely break IOx and possibly some other workloads. Perhaps we could just document this potential disparity?
No problem, updated the docstring
--
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
Re: [PR] Parquet: clear metadata and project fields of ParquetRecordBatchStream::schema [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold merged PR #5135:
URL: https://github.com/apache/arrow-rs/pull/5135
--
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
Re: [PR] Parquet: clear metadata of ParquetRecordBatchStream::schema [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5135:
URL: https://github.com/apache/arrow-rs/pull/5135#issuecomment-1829622022
Whilst I agree with the spirit of this, stripping this metadata would be a major breaking change that would definitely break IOx and possibly some other workloads. Perhaps we could just document this potential disparity?
--
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
Re: [PR] Parquet: clear metadata of ParquetRecordBatchStream::schema [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5135:
URL: https://github.com/apache/arrow-rs/pull/5135#discussion_r1408353398
##########
parquet/src/arrow/async_reader/mod.rs:
##########
@@ -572,7 +572,15 @@ impl<T> std::fmt::Debug for ParquetRecordBatchStream<T> {
}
impl<T> ParquetRecordBatchStream<T> {
- /// Returns the [`SchemaRef`] for this parquet file
+ /// Returns the [`SchemaRef`] for this parquet file.
+ ///
+ /// Note that unlike its synchronous counterpart [`ParquetRecordBatchReader`], the [`SchemaRef`]
+ /// returned here will contain the original metadata, whereas [`ParquetRecordBatchReader`]
+ /// strips this metadata.
Review Comment:
Oh this is the stream not the builder :facepalm:
Yeah we should probably be returning the same schema as the RecordBatch we return, sorry for the confusion... Could you revert your revert, sorry :sweat_smile:
--
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
Re: [PR] Parquet: clear metadata of ParquetRecordBatchStream::schema [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #5135:
URL: https://github.com/apache/arrow-rs/pull/5135#issuecomment-1830706290
I'll revert back to the initial change, as well as account for projection which I wasn't aware it didn't account for either
--
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
Re: [PR] Parquet: clear metadata of ParquetRecordBatchStream::schema [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #5135:
URL: https://github.com/apache/arrow-rs/pull/5135#discussion_r1409109867
##########
parquet/src/arrow/async_reader/mod.rs:
##########
@@ -385,13 +385,28 @@ impl<T: AsyncFileReader + Send + 'static> ParquetRecordBatchStreamBuilder<T> {
offset: self.offset,
};
+ // Ensure schema of ParquetRecordBatchStream respects projection, and does
+ // not store metadata (same as for ParquetRecordBatchReader and emitted RecordBatches)
+ let projected_fields = match reader.fields.as_deref().map(|pf| &pf.arrow_type) {
Review Comment:
I was a bit worried about this, as I couldn't find a straightforward way that the schema was constructed from ParquetField + ProjectionMask, as it seems done in the ArrayReader construction indeed.
Edit: wasn't aware of https://docs.rs/parquet/latest/parquet/arrow/fn.parquet_to_arrow_field_levels.html, will check it out :+1:
--
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