Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4090 +/- ##
==========================================
+ Coverage 75.99% 76.01% +0.02%
==========================================
Files 164 164
Lines 44502 44579 +77
Branches 44502 44579 +77
==========================================
+ Hits 33818 33887 +69
- Misses 9016 9022 +6
- Partials 1668 1670 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
f808c1a to
60dc572
Compare
When I was working on my branch, this was also a problem. I think the right way to solve it would be to use a session at ingestion time to convert the SQL -> logical plan and store that. That, of course, could lead to inconsistencies between the functions that the generated columns had access to and what we use in planning. |
Signed-off-by: Robert Pack <[email protected]>
60dc572 to
dbde71f
Compare
|
Yes @abhiaagarwal! In general we need to defer physical planning as much as possible and see that we have clear APIs and guidelines for managing sessions. |
| let mut actions = this.check_preconditions().await?; | ||
|
|
||
| let Some(mut source) = this.input.take() else { | ||
| return Err(WriteError::MissingData.into()); |
There was a problem hiding this comment.
Check_preconditions already does this check
There was a problem hiding this comment.
yes, saw that when looking into that fn ... but i wanted to cleanly take the input and not have the unwrap ... so purely cosmetic :).
| .table_configuration() | ||
| .is_feature_enabled(&TableFeature::GeneratedColumns) |
There was a problem hiding this comment.
Will this feature be in the snapshot with protocol v4 writer? If not then the behavior is not the same
There was a problem hiding this comment.
good point! and yes, this method will check feature enablement via version and feature lists.
ion-elgreco
left a comment
There was a problem hiding this comment.
Lgtm! Feel free to merge if my comment about gc_is_enabled function maintains previous behavior
Description
It seems we may have found an avenue into moving further into logical planning that does not break stuff 😆. This PR updates the write operation to no longer make use of
DataFrameand use logical plans.Turns out that handling generated columns is a significant part of this. I'll try and remove the the usage of current gc-related function in a follow-up. This touches merge though, so keeping it separate.