Refactor drop columns to fix issues
#10903
Merged
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
sholderbach
reviewed
Nov 1, 2023
Comment on lines
+98
to
+120
| fn disjoint_columns_first_row_becomes_empty() { | ||
| let actual = nu!(pipeline( | ||
| " | ||
| [{a: 1}, {b: 2, c: 3}] | ||
| | drop column | ||
| | columns | ||
| | to nuon | ||
| " | ||
| )); | ||
|
|
||
| assert_eq!(actual.out, "[b, c]"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn disjoint_columns() { | ||
| let actual = nu!(pipeline( | ||
| " | ||
| [{a: 1, b: 2}, {c: 3}] | ||
| | drop column | ||
| | columns | ||
| | to nuon | ||
| " | ||
| )); |
There was a problem hiding this comment.
Gotta admit I find both of those behaviors absolutely cursed based on what the user sees with the default table output.
There was a problem hiding this comment.
Lol, I agree. Perhaps we could:
- drop the first columns? Then it would be
skip columnsinstead ofdrop columns - compute the set of all columns (via
nu_engine::column::get_columns). Unfortunately, this means we would have to collect theListStream.
| .input_output_types(vec![(Type::Table(vec![]), Type::Table(vec![]))]) | ||
| .input_output_types(vec![ | ||
| (Type::Table(vec![]), Type::Table(vec![])), | ||
| (Type::Record(vec![]), Type::Record(vec![])), |
sholderbach
added a commit
to sholderbach/nushell
that referenced
this pull request
Nov 8, 2023
Matches the general behavior of `Vec::drain` or `indexmap::IndexMap::drain`: - Drop the remaining elements (implementing the unstable `keep_rest()` would not be compatible with something like `indexmap`) - No `AsRef<[T]>` or `Drain::as_slice()` behavior as this would make layout assumptions. - `Drain: DoubleEndedIterator` Found useful in nushell#10903
sholderbach
added a commit
that referenced
this pull request
Nov 8, 2023
Matches the general behavior of `Vec::drain` or `indexmap::IndexMap::drain`: - Drop the remaining elements (implementing the unstable `keep_rest()` would not be compatible with something like `indexmap`) - No `AsRef<[T]>` or `Drain::as_slice()` behavior as this would make layout assumptions. - `Drain: DoubleEndedIterator` Found useful in #10903
sholderbach
added a commit
to sholderbach/nushell
that referenced
this pull request
Nov 8, 2023
Compatible with `Vec::truncate` and `indexmap::IndexMap::truncate` Found useful in nushell#10903 for `drop column`
sholderbach
added a commit
that referenced
this pull request
Nov 8, 2023
# Description Compatible with `Vec::truncate` and `indexmap::IndexMap::truncate` Found useful in #10903 for `drop column` # Tests + Formatting Doctest with the relevant edge-cases
sholderbach
approved these changes
Nov 9, 2023
There was a problem hiding this comment.
Rerunning the failed job.
I think I don't want to block this based on the behavior on tables with holes for now.
Great that this PR unlocks records and moves us to the sane API.
We can close #6846 with this and keep #10902 open I think.
I think I would like to bias us to stream by default so we may have to live with the weird behavior. We could consider adding a flag that collects the table so you can get sane truncation for tables with sparse columns.
hardfau1t
pushed a commit
to hardfau1t/nushell
that referenced
this pull request
Dec 14, 2023
Matches the general behavior of `Vec::drain` or `indexmap::IndexMap::drain`: - Drop the remaining elements (implementing the unstable `keep_rest()` would not be compatible with something like `indexmap`) - No `AsRef<[T]>` or `Drain::as_slice()` behavior as this would make layout assumptions. - `Drain: DoubleEndedIterator` Found useful in nushell#10903
hardfau1t
pushed a commit
to hardfau1t/nushell
that referenced
this pull request
Dec 14, 2023
# Description Compatible with `Vec::truncate` and `indexmap::IndexMap::truncate` Found useful in nushell#10903 for `drop column` # Tests + Formatting Doctest with the relevant edge-cases
hardfau1t
pushed a commit
to hardfau1t/nushell
that referenced
this pull request
Dec 14, 2023
# Description This PR refactors `drop columns` and fixes issues nushell#10902 and nushell#6846. Tables with "holes" are now handled consistently, although still somewhat awkwardly. That is, the columns in the first row are used to determine which columns to drop, meaning that the columns displayed all the way to the right by `table` may not be the columns actually being dropped. For example, `[{a: 1}, {b: 2}] | drop column` will drop column `a` instead of `b`. Before, this would give a list of empty records. # User-Facing Changes `drop columns` can now take records as input.
dmatos2012
pushed a commit
to dmatos2012/nushell
that referenced
this pull request
Feb 20, 2024
Matches the general behavior of `Vec::drain` or `indexmap::IndexMap::drain`: - Drop the remaining elements (implementing the unstable `keep_rest()` would not be compatible with something like `indexmap`) - No `AsRef<[T]>` or `Drain::as_slice()` behavior as this would make layout assumptions. - `Drain: DoubleEndedIterator` Found useful in nushell#10903
dmatos2012
pushed a commit
to dmatos2012/nushell
that referenced
this pull request
Feb 20, 2024
# Description Compatible with `Vec::truncate` and `indexmap::IndexMap::truncate` Found useful in nushell#10903 for `drop column` # Tests + Formatting Doctest with the relevant edge-cases
dmatos2012
pushed a commit
to dmatos2012/nushell
that referenced
this pull request
Feb 20, 2024
# Description This PR refactors `drop columns` and fixes issues nushell#10902 and nushell#6846. Tables with "holes" are now handled consistently, although still somewhat awkwardly. That is, the columns in the first row are used to determine which columns to drop, meaning that the columns displayed all the way to the right by `table` may not be the columns actually being dropped. For example, `[{a: 1}, {b: 2}] | drop column` will drop column `a` instead of `b`. Before, this would give a list of empty records. # User-Facing Changes `drop columns` can now take records as input.
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.
Description
This PR refactors
drop columnsand fixes issues #10902 and #6846. Tables with "holes" are now handled consistently, although still somewhat awkwardly. That is, the columns in the first row are used to determine which columns to drop, meaning that the columns displayed all the way to the right bytablemay not be the columns actually being dropped. For example,[{a: 1}, {b: 2}] | drop columnwill drop columnainstead ofb. Before, this would give a list of empty records.User-Facing Changes
drop columnscan now take records as input.