refactor: Zero-Field Structs and DataFrame with Height Property #19123
+1,155
−649
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
github-actions
bot
added
internal
coastalwhite
force-pushed
the
refactor/zero-field-struct
branch
from
2445782
to
11dbc10
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #19123 +/- ##
==========================================
+ Coverage 79.78% 79.79% +0.01%
==========================================
Files 1531 1532 +1
Lines 208445 208649 +204
Branches 2913 2913
==========================================
+ Hits 166301 166498 +197
- Misses 41593 41601 +8
+ Partials 551 550 -1 ☔ View full report in Codecov by Sentry. |
coastalwhite
commented
Oct 9, 2024
Comment on lines
-110
to
-111
| unsafe fn _mmap_unchecked<T: AsRef<[u8]>>( | ||
| fields: &ArrowSchema, |
There was a problem hiding this comment.
drive-by: this function was unused
| let mut out = a | ||
| .fields_as_series() | ||
| .iter() | ||
| .zip(b.fields_as_series().iter()) | ||
| .map(|(l, r)| op(l, r)) | ||
| .reduce(reduce) | ||
| .unwrap(); | ||
| .unwrap_or_else(|| BooleanChunked::full(PlSmallStr::EMPTY, !value, a.len())); |
There was a problem hiding this comment.
if both structs are ZFSs it is always eq / not ne
| @@ -192,10 +192,7 @@ impl DataFrame { | |||
|
|
|||
| match n.get(0) { | |||
| Some(n) => self.sample_n_literal(n as usize, with_replacement, shuffle, seed), | |||
| None => { | |||
There was a problem hiding this comment.
drive-by: this should be the same
| @@ -237,10 +234,7 @@ impl DataFrame { | |||
| let n = (self.height() as f64 * frac) as usize; | |||
| self.sample_n_literal(n, with_replacement, shuffle, seed) | |||
| }, | |||
| None => { | |||
There was a problem hiding this comment.
drive-by: this should be the same
| @@ -32,6 +32,15 @@ impl DataFrame { | |||
| /// - the length of all [`Column`] is equal to the height of this [`DataFrame`] | |||
| /// - the columns names are unique | |||
| pub unsafe fn hstack_mut_unchecked(&mut self, columns: &[Column]) -> &mut Self { | |||
| // If we don't have any columns yet, copy the height from the given columns. | |||
| if let Some(fst) = columns.first() { | |||
| if self.width() == 0 { | |||
There was a problem hiding this comment.
maybe this should have a self.height() == 0 check as well, but I want to leave that as future correctness work
| @@ -1232,6 +1292,10 @@ impl DataFrame { | |||
| if let Some(idx) = self.get_column_index(column.name().as_str()) { | |||
| self.replace_column(idx, column)?; | |||
| } else { | |||
| if self.width() == 0 { | |||
| @@ -1274,7 +1338,13 @@ impl DataFrame { | |||
| debug_assert!(self.width() == 0 || self.height() == column.len()); | |||
| debug_assert!(self.get_column_index(column.name().as_str()).is_none()); | |||
|
|
|||
| // SAFETY: Invariant of function guarantees for case `width` > 0. We set the height | |||
| // properly for `width` == 0. | |||
| if self.width() == 0 { | |||
| @@ -1288,6 +1358,10 @@ impl DataFrame { | |||
| self.replace_column(idx, c)?; | |||
| } | |||
| } else { | |||
| if self.width() == 0 { | |||
| @@ -657,13 +657,6 @@ fn any_values_to_list( | |||
| DataType::Categorical(Some(Arc::new(RevMapping::default())), *ordering) | |||
| }, | |||
|
|
|||
| // Structs don't support empty fields yet. | |||
There was a problem hiding this comment.
It is very nice that we can remove these here.
| .iter() | ||
| .map(|s| s.new_from_index(0, num_rows).into()); |
There was a problem hiding this comment.
drive-by: just convert into scalar column
coastalwhite
requested review from
ritchie46,
c-peters,
alexander-beedie,
MarcoGorelli,
reswqa and
orlp
as code owners
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.
This PR refactors a large part of the code to allow for:
This required quite a bit of changes all over the place to be able to support.