-
Notifications
You must be signed in to change notification settings - Fork 485
feat: upgrade to DataFusion 47.0.0 #3378
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
Conversation
ACTION NEEDED delta-rs follows the Conventional Commits specification for release automation. The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification. |
b5e7d70
to
ef86d80
Compare
ef86d80
to
ff11e08
Compare
I believe DF47 will resolve:
And maybe this one: #3339? |
I will leave some comments about the changes in this PR about what made them necessary |
8f5f785
to
e430e36
Compare
@alamb - I can see you released DF47 to Crates - and it has been merged in delta-kernel. Can we expect this one to be merged soon? I would love to have v0.12.0 of |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3378 +/- ##
==========================================
+ Coverage 71.95% 72.01% +0.06%
==========================================
Files 148 148
Lines 46095 46082 -13
Branches 46095 46082 -13
==========================================
+ Hits 33167 33187 +20
+ Misses 10817 10791 -26
+ Partials 2111 2104 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
83c651f
to
6195604
Compare
@@ -370,15 +370,15 @@ impl ObjectStore for S3StorageBackend { | |||
self.inner.delete(location).await | |||
} | |||
|
|||
fn list(&self, prefix: Option<&Path>) -> BoxStream<'_, ObjectStoreResult<ObjectMeta>> { | |||
fn list(&self, prefix: Option<&Path>) -> BoxStream<'static, ObjectStoreResult<ObjectMeta>> { |
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.
@@ -358,7 +358,7 @@ impl ObjectStore for S3StorageBackend { | |||
self.inner.get_opts(location, options).await | |||
} | |||
|
|||
async fn get_range(&self, location: &Path, range: Range<usize>) -> ObjectStoreResult<Bytes> { | |||
async fn get_range(&self, location: &Path, range: Range<u64>) -> ObjectStoreResult<Bytes> { |
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.
@@ -50,15 +50,6 @@ impl UserDefinedLogicalNodeCore for MetricObserver { | |||
write!(f, "MetricObserver id={}", self.id) | |||
} | |||
|
|||
fn from_template( |
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.
This is an old api that was removed
@@ -671,7 +673,7 @@ impl<'a> DeltaScanBuilder<'a> { | |||
} | |||
}; | |||
|
|||
let file_scan_config = FileScanConfig::new( | |||
let file_scan_config = FileScanConfigBuilder::new( |
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.
@@ -300,7 +300,8 @@ impl LogSegment { | |||
let store = store.clone(); | |||
let read_schema = read_schema.clone(); | |||
async move { | |||
let mut reader = ParquetObjectReader::new(store, meta); | |||
let mut reader = |
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.
} | ||
|
||
impl SchemaMapper for SchemaMapping { | ||
fn map_batch(&self, batch: RecordBatch) -> datafusion_common::Result<RecordBatch> { | ||
let record_batch = cast_record_batch(&batch, self.projected_schema.clone(), false, true)?; | ||
Ok(record_batch) | ||
} | ||
|
||
fn map_partial_batch(&self, batch: RecordBatch) -> datafusion_common::Result<RecordBatch> { |
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.
Due to @adriangb simplifying the implementation in
@@ -1099,8 +1100,11 @@ pub(super) mod zorder { | |||
Ok(DataType::Binary) | |||
} | |||
|
|||
fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue, DataFusionError> { |
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.
Sorry I forgot to submit some comments on this PR. I have also merged up and removed the cargo patches now that datafusion 47 is released. |
Thanks -- sorry @Nordalf -- I tried to polish this PR up and pushed a new version without any patches |
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.
todo! still needs resolution in the hdfs crate, will take a look
Should be a simple version bump, just let me know if there's any issues |
6195604
to
5f9ff3d
Compare
I just force pushed a change to fix the hdfs thing As @Kimahriman said, it just needed a version update. Very easy |
Signed-off-by: Andrew Lamb <[email protected]>
Signed-off-by: Andrew Lamb <[email protected]>
Signed-off-by: R. Tyler Croy <[email protected]>
Signed-off-by: R. Tyler Croy <[email protected]>
a03c975
to
837ead8
Compare
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.
@alamb thank you so much for taking the time to put this together. Everything looks good to me and I believe CI will now 💚 this change, so let's land it and celebrate
Thank you guys! I owe you a beer sometime. Maybe if you are around the Data and AI summit |
@alamb - no need to apologize to me! I am more than happy this got merged so fast 😄. B-e-autiful job! |
Description
upgrade to DataFusion 47.0.0
Related Issue(s)
47.0.0
(April 2025) apache/datafusion#15072As in the past, I am making a PR to upgrade to DataFusion 47 to both:
Documentation