Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 2.4k
Upgrade DataFusion to arrow-rs/parquet 58.0.0 / object_store 0.13.0#19728
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.
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
120d2b040a6a71f50b9c387563d84afd46f70e2841b29b6626a2e631ca1196663a85eb277aac4bb29b0c35b97fad2e8da61002408c062756917a464c1d27d31b979c6c3df7bed79a8a021fabb536cfd5ec273703855b51bbdd19a60d0cc96ccf7a141a65bf7206de80778ac16ddd778dafb5ba30622248e4449fd987f87970a958a5452d509276a8acae9716d5681376762cb714a36012ad66f116d34ad8d7580a70d22dae313fd750b7a724ae020333710fd7afb8f1189f6e01985cde5eb5680ffc49b257ce0fa325831dbf17d4add0464e57d7884258251af00edb3f12d5cdFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Uh oh!
There was an error while loading. Please reload this page.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -36,10 +36,11 @@ use datafusion::{ | ||
| execution::object_store::{DefaultObjectStoreRegistry, ObjectStoreRegistry}, | ||
| }; | ||
| use futures::stream::{BoxStream, Stream}; | ||
| use futures::{StreamExt, TryStreamExt}; | ||
| use object_store::{ | ||
| GetOptions, GetRange, GetResult, ListResult, MultipartUpload, ObjectMeta, | ||
| ObjectStore, PutMultipartOptions, PutOptions, PutPayload, PutResult, Result, | ||
| path::Path, | ||
| CopyOptions, GetOptions, GetRange, GetResult, ListResult, MultipartUpload, | ||
| ObjectMeta, ObjectStore, ObjectStoreExt, PutMultipartOptions, PutOptions, PutPayload, | ||
| PutResult, Result, path::Path, | ||
| }; | ||
| use parking_lot::{Mutex, RwLock}; | ||
| use url::Url; | ||
| @@ -230,40 +231,57 @@ impl InstrumentedObjectStore { | ||
| let timestamp = Utc::now(); | ||
| let range = options.range.clone(); | ||
| let head = options.head; | ||
alamb marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A substantial amount of the changes in this PR are due to the upgrade to object_store 0.13 where several of the trait methods are consolidated (e.g. You can see the upgrade guide here: https://docs.rs/object_store/latest/object_store/trait.ObjectStore.html#upgrade-guide-for-0130 | ||
| let start = Instant::now(); | ||
| let ret = self.inner.get_opts(location, options).await?; | ||
| let elapsed = start.elapsed(); | ||
| let (op, size) = if head { | ||
| (Operation::Head, None) | ||
| } else { | ||
| ( | ||
| Operation::Get, | ||
| Some((ret.range.end - ret.range.start) as usize), | ||
| ) | ||
| }; | ||
| self.requests.lock().push(RequestDetails { | ||
| op: Operation::Get, | ||
| op, | ||
| path: location.clone(), | ||
| timestamp, | ||
| duration: Some(elapsed), | ||
| size: Some((ret.range.end - ret.range.start) as usize), | ||
| size, | ||
| range, | ||
| extra_display: None, | ||
| }); | ||
| Ok(ret) | ||
| } | ||
| async fn instrumented_delete(&self, location: &Path) -> Result<()> { | ||
| fn instrumented_delete_stream( | ||
| &self, | ||
| locations: BoxStream<'static, Result<Path>>, | ||
| ) -> BoxStream<'static, Result<Path>> { | ||
| let requests_captured = Arc::clone(&self.requests); | ||
| let timestamp = Utc::now(); | ||
| let start = Instant::now(); | ||
| self.inner.delete(location).await?; | ||
| let elapsed = start.elapsed(); | ||
| self.requests.lock().push(RequestDetails { | ||
| op: Operation::Delete, | ||
| path: location.clone(), | ||
| timestamp, | ||
| duration: Some(elapsed), | ||
| size: None, | ||
| range: None, | ||
| extra_display: None, | ||
| }); | ||
| Ok(()) | ||
| self.inner | ||
| .delete_stream(locations) | ||
| .and_then(move |location| { | ||
| let elapsed = start.elapsed(); | ||
| requests_captured.lock().push(RequestDetails { | ||
| op: Operation::Delete, | ||
| path: location.clone(), | ||
| timestamp, | ||
| duration: Some(elapsed), | ||
| size: None, | ||
| range: None, | ||
| extra_display: None, | ||
| }); | ||
| futures::future::ok(location) | ||
| }) | ||
| .boxed() | ||
| } | ||
| fn instrumented_list( | ||
| @@ -361,25 +379,6 @@ impl InstrumentedObjectStore { | ||
| Ok(()) | ||
| } | ||
| async fn instrumented_head(&self, location: &Path) -> Result<ObjectMeta> { | ||
| let timestamp = Utc::now(); | ||
| let start = Instant::now(); | ||
| let ret = self.inner.head(location).await?; | ||
| let elapsed = start.elapsed(); | ||
| self.requests.lock().push(RequestDetails { | ||
| op: Operation::Head, | ||
| path: location.clone(), | ||
| timestamp, | ||
| duration: Some(elapsed), | ||
| size: None, | ||
| range: None, | ||
| extra_display: None, | ||
| }); | ||
| Ok(ret) | ||
| } | ||
| } | ||
| impl fmt::Display for InstrumentedObjectStore { | ||
| @@ -429,12 +428,15 @@ impl ObjectStore for InstrumentedObjectStore { | ||
| self.inner.get_opts(location, options).await | ||
| } | ||
| async fn delete(&self, location: &Path) -> Result<()> { | ||
| fn delete_stream( | ||
| &self, | ||
| locations: BoxStream<'static, Result<Path>>, | ||
| ) -> BoxStream<'static, Result<Path>> { | ||
| if self.enabled() { | ||
| return self.instrumented_delete(location).await; | ||
| return self.instrumented_delete_stream(locations); | ||
| } | ||
| self.inner.delete(location).await | ||
| self.inner.delete_stream(locations) | ||
| } | ||
| fn list(&self, prefix: Option<&Path>) -> BoxStream<'static, Result<ObjectMeta>> { | ||
| @@ -453,28 +455,24 @@ impl ObjectStore for InstrumentedObjectStore { | ||
| self.inner.list_with_delimiter(prefix).await | ||
| } | ||
| async fn copy(&self, from: &Path, to: &Path) -> Result<()> { | ||
ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. copy and copy_if_not_exists were consolidated | ||
| if self.enabled() { | ||
| return self.instrumented_copy(from, to).await; | ||
| } | ||
| self.inner.copy(from, to).await | ||
| } | ||
| async fn copy_if_not_exists(&self, from: &Path, to: &Path) -> Result<()> { | ||
| if self.enabled() { | ||
| return self.instrumented_copy_if_not_exists(from, to).await; | ||
| } | ||
| self.inner.copy_if_not_exists(from, to).await | ||
| } | ||
| async fn head(&self, location: &Path) -> Result<ObjectMeta> { | ||
| async fn copy_opts( | ||
| &self, | ||
| from: &Path, | ||
| to: &Path, | ||
| options: CopyOptions, | ||
| ) -> Result<()> { | ||
| if self.enabled() { | ||
| return self.instrumented_head(location).await; | ||
| return match options.mode { | ||
| object_store::CopyMode::Create => { | ||
| self.instrumented_copy_if_not_exists(from, to).await | ||
| } | ||
| object_store::CopyMode::Overwrite => { | ||
| self.instrumented_copy(from, to).await | ||
| } | ||
| }; | ||
| } | ||
| self.inner.head(location).await | ||
| self.inner.copy_opts(from, to, options).await | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -48,7 +48,7 @@ use datafusion_physical_expr_adapter::{ | ||
| use futures::StreamExt; | ||
| use object_store::memory::InMemory; | ||
| use object_store::path::Path; | ||
| use object_store::{ObjectStore, PutPayload}; | ||
| use object_store::{ObjectStore, ObjectStoreExt, PutPayload}; | ||
| // Metadata key for storing default values in field metadata | ||
| const DEFAULT_VALUE_METADATA_KEY: &str = "example.default_value"; | ||
| @@ -79,7 +79,7 @@ pub async fn default_column_values() -> Result<()> { | ||
| let mut buf = vec![]; | ||
| let props = WriterProperties::builder() | ||
| .set_max_row_group_size(2) | ||
| .set_max_row_group_row_count(Some(2)) | ||
ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This configuration was renamed in | ||
| .build(); | ||
| let mut writer = | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -43,7 +43,7 @@ use datafusion::parquet::arrow::arrow_reader::{ | ||
| ArrowReaderOptions, ParquetRecordBatchReaderBuilder, RowSelection, RowSelector, | ||
| }; | ||
| use datafusion::parquet::arrow::async_reader::{AsyncFileReader, ParquetObjectReader}; | ||
| use datafusion::parquet::file::metadata::ParquetMetaData; | ||
| use datafusion::parquet::file::metadata::{PageIndexPolicy, ParquetMetaData}; | ||
| use datafusion::parquet::file::properties::{EnabledStatistics, WriterProperties}; | ||
| use datafusion::parquet::schema::types::ColumnPath; | ||
| use datafusion::physical_expr::PhysicalExpr; | ||
| @@ -410,7 +410,7 @@ impl IndexedFile { | ||
| let options = ArrowReaderOptions::new() | ||
| // Load the page index when reading metadata to cache | ||
| // so it is available to interpret row selections | ||
| .with_page_index(true); | ||
| .with_page_index_policy(PageIndexPolicy::Required); | ||
| let reader = | ||
| ParquetRecordBatchReaderBuilder::try_new_with_options(file, options)?; | ||
| let metadata = reader.metadata().clone(); | ||
| @@ -567,7 +567,7 @@ impl ParquetFileReaderFactory for CachedParquetFileReaderFactory { | ||
| .object_meta | ||
| .location | ||
| .parts() | ||
| .last() | ||
| .next_back() | ||
ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. clippy told me to do this -- I am not sure why it doesn't do so on main | ||
| .expect("No path in location") | ||
| .as_ref() | ||
| .to_string(); | ||
| @@ -659,7 +659,7 @@ fn make_demo_file(path: impl AsRef<Path>, value_range: Range<i32>) -> Result<()> | ||
| // enable page statistics for the tag column, | ||
| // for everything else. | ||
| let props = WriterProperties::builder() | ||
| .set_max_row_group_size(100) | ||
| .set_max_row_group_row_count(Some(100)) | ||
| // compute column chunk (per row group) statistics by default | ||
| .set_statistics_enabled(EnabledStatistics::Chunk) | ||
| // compute column page statistics for the tag column | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this reduction in metadata size is a direct consequence of @WaterWhisperer's PR to improve PageEncoding representation
PageEncodingStatsto bitmask arrow-rs#9051