Modify reject algorithm for identical elements#8446
Merged
sholderbach merged 2 commits intonushell:mainfrom Mar 14, 2023
Merged
Modify reject algorithm for identical elements#8446sholderbach merged 2 commits intonushell:mainfrom
sholderbach merged 2 commits intonushell:mainfrom
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #8446 +/- ##
==========================================
+ Coverage 68.13% 68.50% +0.37%
==========================================
Files 620 620
Lines 99723 99726 +3
==========================================
+ Hits 67950 68322 +372
+ Misses 31773 31404 -369
|
Contributor
|
Seems reasonable. Thanks! |
sholderbach
approved these changes
Mar 14, 2023
Member
sholderbach
left a comment
There was a problem hiding this comment.
Thanks for stopping the panic! Would you mind adding a test that reproduces the old panic so we can be sure that we don't run into that again?
Contributor
Author
|
@sholderbach Tests are added :) |
Member
|
Awesome! |
sholderbach
added a commit
to sholderbach/nushell
that referenced
this pull request
Oct 29, 2023
For this we make the assumption that the keys are unique. Note: this does not hold universally, yet. Implementations in `nu-protocol` use an all key removal on the basis of retain in certain places. (e.g. used by `reject` under the hood) See nushell#8446 for this defensiveness
sholderbach
added a commit
that referenced
this pull request
Nov 1, 2023
# Description Pretty much all operations/commands in Nushell assume that the column names/keys in a record and thus also in a table (which consists of a list of records) are unique. Access through a string-like cell path should refer to a single column or key/value pair and our output through `table` will only show the last mention of a repeated column name. ```nu [[a a]; [1 2]] ╭─#─┬─a─╮ │ 0 │ 2 │ ╰───┴───╯ ``` While the record parsing already either errors with the `ShellError::ColumnDefinedTwice` or silently overwrites the first occurence with the second occurence, the table literal syntax `[[header columns]; [val1 val2]]` currently still allowed the creation of tables (and internally records with more than one entry with the same name. This is not only confusing, but also breaks some assumptions around how we can efficiently perform operations or in the past lead to outright bugs (e.g. #8431 fixed by #8446). This PR proposes to make this an error. After this change another hole which allowed the construction of records with non-unique column names will be plugged. ## Parts - Fix `SE::ColumnDefinedTwice` error code - Remove previous tests permitting duplicate columns - Deny duplicate column in table literal eval - Deny duplicate column in const eval - Deny duplicate column in `from nuon` # User-Facing Changes `[[a a]; [1 2]]` will now return an error: ``` Error: nu::shell::column_defined_twice × Record field or table column used twice ╭─[entry #2:1:1] 1 │ [[a a]; [1 2]] · ┬ ┬ · │ ╰── field redefined here · ╰── field first defined here ╰──── ``` this may under rare circumstances block code from evaluating. Furthermore this makes some NUON files invalid if they previously contained tables with repeated column names. # Tests + Formatting Added tests for each of the different evaluation paths that materialize tables.
sholderbach
added a commit
to sholderbach/nushell
that referenced
this pull request
Nov 2, 2023
This breaks two tests Using `Record::remove` assumes that the keys are all unique. The current implementation since nushell#8446 uses the less efficient way of going over the whole record with `.retain` to deal with repeated keys. Culprit in the example is the table literal
hardfau1t
pushed a commit
to hardfau1t/nushell
that referenced
this pull request
Dec 14, 2023
# Description Pretty much all operations/commands in Nushell assume that the column names/keys in a record and thus also in a table (which consists of a list of records) are unique. Access through a string-like cell path should refer to a single column or key/value pair and our output through `table` will only show the last mention of a repeated column name. ```nu [[a a]; [1 2]] ╭─#─┬─a─╮ │ 0 │ 2 │ ╰───┴───╯ ``` While the record parsing already either errors with the `ShellError::ColumnDefinedTwice` or silently overwrites the first occurence with the second occurence, the table literal syntax `[[header columns]; [val1 val2]]` currently still allowed the creation of tables (and internally records with more than one entry with the same name. This is not only confusing, but also breaks some assumptions around how we can efficiently perform operations or in the past lead to outright bugs (e.g. nushell#8431 fixed by nushell#8446). This PR proposes to make this an error. After this change another hole which allowed the construction of records with non-unique column names will be plugged. ## Parts - Fix `SE::ColumnDefinedTwice` error code - Remove previous tests permitting duplicate columns - Deny duplicate column in table literal eval - Deny duplicate column in const eval - Deny duplicate column in `from nuon` # User-Facing Changes `[[a a]; [1 2]]` will now return an error: ``` Error: nu::shell::column_defined_twice × Record field or table column used twice ╭─[entry nushell#2:1:1] 1 │ [[a a]; [1 2]] · ┬ ┬ · │ ╰── field redefined here · ╰── field first defined here ╰──── ``` this may under rare circumstances block code from evaluating. Furthermore this makes some NUON files invalid if they previously contained tables with repeated column names. # Tests + Formatting Added tests for each of the different evaluation paths that materialize tables.
dmatos2012
pushed a commit
to dmatos2012/nushell
that referenced
this pull request
Feb 20, 2024
# Description Pretty much all operations/commands in Nushell assume that the column names/keys in a record and thus also in a table (which consists of a list of records) are unique. Access through a string-like cell path should refer to a single column or key/value pair and our output through `table` will only show the last mention of a repeated column name. ```nu [[a a]; [1 2]] ╭─#─┬─a─╮ │ 0 │ 2 │ ╰───┴───╯ ``` While the record parsing already either errors with the `ShellError::ColumnDefinedTwice` or silently overwrites the first occurence with the second occurence, the table literal syntax `[[header columns]; [val1 val2]]` currently still allowed the creation of tables (and internally records with more than one entry with the same name. This is not only confusing, but also breaks some assumptions around how we can efficiently perform operations or in the past lead to outright bugs (e.g. nushell#8431 fixed by nushell#8446). This PR proposes to make this an error. After this change another hole which allowed the construction of records with non-unique column names will be plugged. ## Parts - Fix `SE::ColumnDefinedTwice` error code - Remove previous tests permitting duplicate columns - Deny duplicate column in table literal eval - Deny duplicate column in const eval - Deny duplicate column in `from nuon` # User-Facing Changes `[[a a]; [1 2]]` will now return an error: ``` Error: nu::shell::column_defined_twice × Record field or table column used twice ╭─[entry nushell#2:1:1] 1 │ [[a a]; [1 2]] · ┬ ┬ · │ ╰── field redefined here · ╰── field first defined here ╰──── ``` this may under rare circumstances block code from evaluating. Furthermore this makes some NUON files invalid if they previously contained tables with repeated column names. # Tests + Formatting Added tests for each of the different evaluation paths that materialize tables.
This file contains hidden or 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
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
The correction made here concerns the issue #8431. Indeed, the algorithm initially proposed to remove elements of a
vectorperformed a loop withremoveand an incident therefore appeared when several values were equal because the deletion was done outside the length of the vector:Then,
[[a, a]; [1, 2]] | reject a:gavethread 'main' panicked at 'removal index (is 1) should be < len (is 1)', crates/nu-protocol/src/value/mod.rs:1213:54.The proposed correction is therefore the implementation of the
retain_mututility dedicated to this functionality.