feat!(ffi): Make get_partition_column* work on a snapshot. #697
Conversation
Also rename the old ones to get_scan_partition_column*
github-actions
bot
added
the
breaking-change
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #697 +/- ##
==========================================
+ Coverage 84.19% 84.42% +0.22%
==========================================
Files 77 77
Lines 17960 18004 +44
Branches 17960 18004 +44
==========================================
+ Hits 15122 15199 +77
+ Misses 2121 2087 -34
- Partials 717 718 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
It's anyway a breaking change; should we just delete this redundant API to have one right way of doing things?
There was a problem hiding this comment.
Given it's a breaking change, pls make the conventional commit a feat!
| /// Get an iterator of the list of partition columns for this snapshot. | ||
| /// | ||
| /// # Safety | ||
| /// Caller is responsible for passing a valid snapshot handle. |
There was a problem hiding this comment.
Should the caller also be freeing the strings? Mention it in the docs if it's worth mentioning.
There was a problem hiding this comment.
Should the caller also be freeing the strings?
No. String slices by definition are not owned.
In this specific case, the strings will be freed by the iterator itself when it gets dropped.
There was a problem hiding this comment.
Given it's a breaking change, pls make the conventional commit a feat!
There was a problem hiding this comment.
looks great :) seems similar to my schema changes in #709 - glad we are improving these!
| pub enum TestAction { | ||
| Add(String), | ||
| Remove(String), | ||
| Metadata, | ||
| } | ||
|
|
||
| /// Convert a vector of actions into a newline delimited json string | ||
| // TODO: We need a better way to mock tables :) |
There was a problem hiding this comment.
agree - should we track an issue and try to size/prioritize it?
zachschuermann
changed the title
zachschuermann
changed the title
| /// Get an iterator of the list of partition columns for this snapshot. | ||
| /// | ||
| /// # Safety | ||
| /// Caller is responsible for passing a valid snapshot handle. |
There was a problem hiding this comment.
Should the caller also be freeing the strings?
No. String slices by definition are not owned.
In this specific case, the strings will be freed by the iterator itself when it gets dropped.
What changes are proposed in this pull request?
Make
get_partition_column_countandget_partition_columnstake a snapshot so engines can work this out at planning time without creating a scan.The previous methods to get this info out of a scan have been removed.
Yes, the old functions have had
_scan_added to the name, so this is a breaking change. (but I don't know of anyone who was using the old ones other thanread_table.c.How was this change tested?
New unit test