feat: make ExpressionHandler::get_evaluator fallible
#577
+66
−44
Conversation
This file contains 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
Make get_evaluator() return DeltaResult<Arc<dyn ExpressionEvaluator>> to properly handle potential errors. Update all call sites to handle the Result type and simplify error handling using the ok()? operator where appropriate.
github-actions
bot
added
the
breaking-change
scovich
reviewed
Dec 10, 2024
| STATS_EXPR.clone(), | ||
| DataType::STRING, | ||
| ) | ||
| .ok()?; |
There was a problem hiding this comment.
Should we have at least a warn! log or similar, to alert when these errors happen? Otherwise I worry it could be very difficult to debug why data skipping does not occur?
There was a problem hiding this comment.
agreed, this is the one part I wasn't too sure about
kernel/src/scan/log_replay.rs
Outdated
| table_schema: &SchemaRef, | ||
| predicate: Option<ExpressionRef>, | ||
| ) -> impl Iterator<Item = DeltaResult<ScanData>> { | ||
| ) -> Box<dyn Iterator<Item = DeltaResult<ScanData>>> { |
There was a problem hiding this comment.
We should consider returning DeltaResult<impl Iterator>, so the error is immediately obvious instead of buried in the iterator itself. Plenty of examples of that elsewhere in the kernel.
…tions - Modified scan_action_iter to return DeltaResult<impl Iterator> - Fixed code formatting in log_replay.rs files - Reordered imports in mod.rs for better readability - Improved error propagation in transaction.rs Co-Authored-By: Calvin Giroud <[email protected]>
Added warning logs when evaluator creation fails in DataSkippingFilter: - Log stats selector evaluator failures - Log skipping evaluator failures - Log filter evaluator failures This improves debuggability when data skipping does not occur as expected. Co-Authored-By: Calvin Giroud <[email protected]>
…olving conflicts Co-Authored-By: Calvin Giroud <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #566 . Makes get_evaluator() return DeltaResult<Arc> to properly handle potential errors. This change is intended to support
get_evaluator()doing eager validation checking, but doesn't do the validation itself.What changes are proposed in this pull request?
Here's a comprehensive summary of all changes made:
How was this change tested?
Tested with
cargo test --all-features --all-targets -- --skip read_table_version_hdfs. Had an issue with getting Java setup but that test doesn't seem relevant to the PR. No new tests need to be written since it's a no-op refactor.Additional Context
This PR was entirely written by Devin with a little bit of review from me. Happy to address any feedback and get this over the finish line.
Original Run: https://preview.devin.ai/sessions/e839a4e9bcb444b69b99205babdcf1af