Repository navigation
fix(ffi): preserve TableProvider DML overrides - #23906
Conversation
github-actions
Bot
added
ffi
ca8af01 to
5568899
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #23906 +/- ##
========================================
Coverage 81.29% 81.29%
========================================
Files 1110 1110
Lines 385336 385553 +217
Branches 385336 385553 +217
========================================
+ Hits 313261 313449 +188
Misses 53594 53594
- Partials 18481 18510 +29 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
@Amogh-2404
Thanks for working on this!
This looks good to me overall. I just have one small suggestion that could help strengthen the test coverage.
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Preserves TableProvider DML overrides (delete/update/truncate) across the FFI boundary so foreign table providers remain writable rather than falling back to default read-only behavior.
Changes:
- Adds FFI forwarding for
delete_from,update, andtruncateand ensures ordered filters/assignments survive serialization. - Refactors expression (de)serialization into helpers used by scan/pushdown/DML.
- Adds integration + round-trip tests, including malformed update assignment payload coverage.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| datafusion/ffi/tests/ffi_integration.rs | Adds cross-library integration test verifying DML plans are produced via FFI provider. |
| datafusion/ffi/src/tests/mod.rs | Extends test provider to implement DML overrides and validate received filters/assignments. |
| datafusion/ffi/src/table_provider.rs | Extends FFI ABI to include DML function pointers; adds wrappers, serialization helpers, and unit tests. |
Suppressed comments (1)
datafusion/ffi/src/table_provider.rs:1
- This changes the
FFI_TableProviderABI/layout by adding new function pointers. If an older foreign library is loaded at runtime, reading/calling these new fields can become UB (crash) rather than a clean error. Recommendation (mandatory): bump/extend the FFI ABI versioning and enforce it before constructing/usingForeignTableProviderDML methods (e.g., reject older providers early with a clear error), or gate calls behind an explicit version/feature check with safe fallbacks.
// Licensed to the Apache Software Foundation (ASF) under one
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
timsaucer
left a comment
timsaucer
left a comment
There was a problem hiding this comment.
Thank you for the PR!
Overall this looks very nice. I have a couple of organizational comments, but the general flow looks good to me.
64923e2 to
8c72608
Compare
|
thanks @Amogh-2404 |
Which issue does this PR close?
Part of #22328.
Rationale for this change
ForeignTableProviderinherits the default DML methods, so producer overrides are lost across the FFI boundary and writable tables appear read-only.What changes are included in this PR?
delete_from,update, andtruncatethroughFFI_TableProvider.datafusion-protohelpers.Are these changes tested?
cargo test -p datafusion-ffi --features integration-testscargo clippy --all-targets --all-features -- -D warningsAre there any user-facing changes?
The FFI ABI changes. Foreign libraries must rebuild against the new DataFusion version.