Add Warnings whenever a floating-point value is used as a Dictionary key #11417
Conversation
GregoryTravis
added
the
CI: No changelog needed
GregoryTravis
requested review from
jdunkerley,
radeusgd,
AdRiley and
marthasharkey
as code owners
| group_builder.specify "should attach a warning when a Float is used as a key" <| | ||
| m = Dictionary.empty . insert 1.2 3 | ||
| m.at 1.2 . should_equal 3 | ||
| Problems.expect_only_warning Float_Used_As_Dictionary_Key m |
There was a problem hiding this comment.
Please also add a test that checks if we have the same behaviour when we construct the dictionary using Dictionary.from_vector.
|
I am still evaluating benchmarks for this change. |
| ## Indicates that a Float was used as a Dicionary key. | ||
| type Float_Used_As_Dictionary_Key |
There was a problem hiding this comment.
This type feels slightly redundant with already existing Floating_Point_Equality.
However, I do like that we get a more precise message ('Float used as dictionary key'). What about merging this into the Floating_Point_Equality error, perhaps by distinguishing different constructors (we already have multi-constructor errors, e.g. Invalid_Value_Type).
There was a problem hiding this comment.
Done -- moved Floating_Point_Equality into Base.
| # Scenario similar to what is done in distinct | ||
| # 'Dense' ints have more duplicates and cause fewer inserts to happen. |
There was a problem hiding this comment.
I'd add
| # Scenario similar to what is done in distinct | |
| # 'Dense' ints have more duplicates and cause fewer inserts to happen. | |
| # Scenario similar to what is done in distinct | |
| # 'Dense' ints have more duplicates and cause fewer inserts, but more key updates to happen. |
(assuming it's true, but it feels it should be)
|
Benchmarks look good. |
| @@ -4,6 +4,7 @@ import Standard.Base.Data.Filter_Condition as Filter_Condition_Module | |||
| import Standard.Base.Data.Time.Errors.Date_Time_Format_Parse_Error | |||
| import Standard.Base.Data.Vector.Builder | |||
| import Standard.Base.Errors.Common.Additional_Warnings | |||
| import Standard.Base.Errors.Common.Floating_Point_Equality | |||
There was a problem hiding this comment.
Is this needed? Looks like only change to this file.
There was a problem hiding this comment.
Floating_Point_Equality is used in this file, and was previously imported by from Standard.Table.Errors import all, but not anymore because it has moved to Common.
GregoryTravis
added
the
CI: Ready to merge
GregoryTravis
added
the
CI: Clean build required
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
or the Snowflake database integration, a run of the Extra Tests has been scheduled.