Repository navigation
Added implode KDocs - #2178
Added implode KDocs#2178AndreiKingsley wants to merge 3 commits into
implode KDocs#2178Conversation
![jetbrains-air[bot]](https://avatars.githubusercontent.com/in/3314082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for the thorough KDocs — just one dropNA caveat to add inline before this can go in!
Produced by Air Automations. Name: AI Review / Run: https://air.jetbrains.cloud/org/05cf1a7f-6ab5-713b-abd3-29d0c8a05e2d/automations/8daf2412-0b6c-4433-90c0-b40ab5aebcdb?run=9639e615-fa9a-401f-948e-cb852fd397de
| @ExcludeFromSources | ||
| internal interface ImplodeDocs { | ||
| /** | ||
| * @param [\dropNA] If `true`, removes {@include [NALink]} values (`null`, [Double.NaN] or [Float.NaN]) |
There was a problem hiding this comment.
Blocking: dropNA drops NA values per column, independently, so when several columns are imploded in one call their lists stop lining up. This KDoc (and the new implodeDropNA_properties sample that illustrates it, which implodes city and year together) presents it as a harmless filter.
Failure scenario (verified on this branch): rows ("Alice", null, 2020), ("Alice", "Milan", 2021); df.implode(dropNA = true) { city and year } yields city = [Milan], year = [2020, 2021]. Calling the documented reverse operation, explode { city and year }, gives (Milan, 2020) and (null, 2021) — Milan is silently re-paired with the wrong year, no error. A user following this doc corrupts cross-column row correspondence.
Suggested fix: add a sentence to DropNASnippet stating that values are removed from each imploded column independently, so lists in different imploded columns may lose their positional correspondence and the explode roundtrip is then not value-preserving; and implode a single column in the dropNA sample so the example doesn't teach the multi-column form.
There was a problem hiding this comment.
Also restructure the sentence a bit :) "removes NA values", who removes them? the function? You can also write "are removed" if that makes sense
There was a problem hiding this comment.
Looks great — the KDocs match the implementation and the website page, and all four samples pass. 🎉
Produced by Air Automations. Name: AI Review / Run: https://air.jetbrains.cloud/org/05cf1a7f-6ab5-713b-abd3-29d0c8a05e2d/automations/8daf2412-0b6c-4433-90c0-b40ab5aebcdb?run=fb0fc889-be0e-4f3f-8b28-1abb7e96b087
There was a problem hiding this comment.
Correcting my earlier approval — the blocking dropNA finding already posted inline does need addressing first, thanks!
Produced by Air Automations. Name: AI Review / Run: https://air.jetbrains.cloud/org/05cf1a7f-6ab5-713b-abd3-29d0c8a05e2d/automations/8daf2412-0b6c-4433-90c0-b40ab5aebcdb?run=fb0fc889-be0e-4f3f-8b28-1abb7e96b087
| @Test | ||
| fun implodeDropNA_properties() { | ||
| // SampleStart | ||
| // Same as above, but `null` cities are removed from the lists |
There was a problem hiding this comment.
The user reading the KDoc won't know that there are no null years, but that's the price of unification of samples :)
There was a problem hiding this comment.
Added tables
|
@Allex-Nik added KDocs tables |
There was a problem hiding this comment.
Thanks for the fixes — one more dropNA caveat to add inline, then this is good to go!
Produced by Air Automations. Name: AI Review / Run: https://air.jetbrains.cloud/org/05cf1a7f-6ab5-713b-abd3-29d0c8a05e2d/automations/8daf2412-0b6c-4433-90c0-b40ab5aebcdb?run=ba455033-b2f2-4d24-bff0-3d571c73858f
| internal interface ImplodeDocs { | ||
| /** | ||
| * @param [\dropNA] If `true`, removes {@include [NALink]} values (`null`, [Double.NaN] or [Float.NaN]) | ||
| * from the merged lists. `false` by default. |
There was a problem hiding this comment.
Blocking: dropNA only applies to value columns. implodeImpl filters just ColumnKind.Value; ColumnKind.Group goes through extractDataFrame() and ColumnKind.Frame through concat(), both unfiltered. This KDoc (and the one at line 30) states unconditionally that NA values are removed.
Failure scenario (verified on this branch): dataFrameOf("key" to listOf("a", "a"), "city" to listOf("London", null), "weight" to listOf(54, null)).group { city and weight }.into("info").implode(dropNA = true) { info } returns a nested frame with 2 rows — the all-NA row is still there. The linked website page's own example implodes a column group (df.implode { name and age and weight and isHappy }), so a user adding dropNA = true to it gets weight cleaned and name untouched, with no error and nothing in the docs explaining why.
Suggested fix: add to DropNASnippet (and the overview sentence) that dropNA affects only value columns — imploded column groups and frame columns keep all their rows regardless.
| * | ||
| * Imploded columns will change their types: | ||
| * - `T` to `List<T>`; | ||
| * - [DataRow] to [DataFrame] — an imploded [ColumnGroup] is converted into a [FrameColumn]. |
There was a problem hiding this comment.
shouldn't that last one get a new line?
| @ExcludeFromSources | ||
| internal interface ImplodeDocs { | ||
| /** | ||
| * @param [\dropNA] If `true`, removes {@include [NALink]} values (`null`, [Double.NaN] or [Float.NaN]) |
There was a problem hiding this comment.
Also restructure the sentence a bit :) "removes NA values", who removes them? the function? You can also write "are removed" if that makes sense
|
|
||
| /** | ||
| * @return A new [DataFrame] with one row per distinct combination of values in the other columns, | ||
| * where the [\columns] contain merged lists of values. |
There was a problem hiding this comment.
the *specified columns
| * | ||
| * If [dropNA] is `true`, {@include [NALink]} values are removed from the merged lists. | ||
| * | ||
| * It is the same as `df.implode(dropNA) { all() }[0]`. |
There was a problem hiding this comment.
.single() might be better, actually. It signalizes implode turns the df into a single row (and it's safer)
Closes #1385