feat!: Make expression and predicate evaluator constructors fallible #1452
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1452 +/- ##
==========================================
- Coverage 85.00% 84.97% -0.04%
==========================================
Files 120 120
Lines 31620 31641 +21
Branches 31620 31641 +21
==========================================
+ Hits 26879 26886 +7
+ Misses 3446 3445 -1
- Partials 1295 1310 +15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
github-actions
bot
added
the
breaking-change
sanujbasu
force-pushed
the
fallible-evaluator-constructors
branch
from
aecdadf to
9155ff7
Compare
sanujbasu
force-pushed
the
fallible-evaluator-constructors
branch
2 times, most recently
from
9df0890 to
ad74958
Compare
OussamaSaoudi
approved these changes
Nov 4, 2025
scovich
approved these changes
Nov 4, 2025
Collaborator
scovich
left a comment
scovich
left a comment
There was a problem hiding this comment.
LGTM, couple questions that probably don't block merge
kernel/src/scan/data_skipping.rs
Outdated
Comment on lines
147
to
148
| .map_err(|e| error!("Failed to create select stats evaluator: {e}")) | ||
| .ok()?; |
Collaborator
There was a problem hiding this comment.
Interesting... this turns Result<T, Error> into Result<T, ()> and then returns None in case of Result::Err? That took a minute to grok.
Collaborator
Author
There was a problem hiding this comment.
I'll change these to inspect error which makes the intent clearer I guess
Comment on lines
163
to
167
| .new_predicate_evaluator(stats_schema.clone(), FILTER_PRED.clone()) | ||
| .ok()?; |
Collaborator
There was a problem hiding this comment.
Why not log the error! here as well?
sanujbasu
force-pushed
the
fallible-evaluator-constructors
branch
from
ad74958 to
da506b3
Compare
This PR refactors the `new_expression_evaluator` and `new_predicate_evaluator` methods in the `EvaluationHandler` trait to return `DeltaResult` instead of being infallible. This is a breaking API change that enables: - Better error reporting at evaluator construction time rather than evaluation - Early validation of expression/predicate compatibility with input schemas - More idiomatic Rust error handling patterns **Changes:** - Updated `EvaluationHandler` trait signatures to return `DeltaResult<Arc<dyn Evaluator>>` - Updated `ArrowEvaluationHandler` implementation to wrap returns in `Ok(...)` - Made `ScanLogReplayProcessor::new` fallible - Updated `scan_action_iter` to return `DeltaResult<impl Iterator<...>>` - Updated 12 callsites across the codebase to propagate errors with `?` operator - Updated FFI layer to handle errors through ExternResult - All 630 existing kernel tests pass - All 18 FFI tests pass - Verified compilation with `cargo check --all-features` - No functional changes - purely refactoring error handling patterns
sanujbasu
force-pushed
the
fallible-evaluator-constructors
branch
from
da506b3 to
d64cee7
Compare
aleksandarskrbic
pushed a commit
to aleksandarskrbic/delta-kernel-rs
that referenced
this pull request
Nov 10, 2025
…elta-io#1452) This PR refactors the `new_expression_evaluator` and `new_predicate_evaluator` methods in the `EvaluationHandler` trait to return `DeltaResult` instead of being infallible. Addresses delta-io#566. ## What changes are proposed in this pull request? This is a breaking API change that enables: - Better error reporting at evaluator construction time rather than evaluation - Early validation of expression/predicate compatibility with input schemas - More idiomatic Rust error handling patterns ### This PR affects the following public APIs `new_expression_evaluator` and `new_predicate_evaluator` in `EvaluationHandler` **Changes:** - Updated `EvaluationHandler` trait signatures to return `DeltaResult<Arc<dyn Evaluator>>` - Updated `ArrowEvaluationHandler` implementation to wrap returns in `Ok(...)` - Made `ScanLogReplayProcessor::new` fallible - Updated `scan_action_iter` to return `DeltaResult<impl Iterator<...>>` - Updated 12 callsites across the codebase to propagate errors with `?` operator - Updated FFI layer to handle errors using ExternResult ## How was this change tested? - All 630 existing kernel tests pass - All 18 FFI tests pass - Verified compilation with `cargo check --all-features` - No functional changes - purely refactoring error handling patterns
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR refactors the
new_expression_evaluatorandnew_predicate_evaluatormethods in theEvaluationHandlertrait to returnDeltaResultinstead of being infallible. Addresses #566.What changes are proposed in this pull request?
This is a breaking API change that enables:
This PR affects the following public APIs
new_expression_evaluatorandnew_predicate_evaluatorinEvaluationHandlerChanges:
EvaluationHandlertrait signatures to returnDeltaResult<Arc<dyn Evaluator>>ArrowEvaluationHandlerimplementation to wrap returns inOk(...)ScanLogReplayProcessor::newfalliblescan_action_iterto returnDeltaResult<impl Iterator<...>>?operatorHow was this change tested?
cargo check --all-features