Kotlin / Kotlin/dataframe

Add KDocs for non-deprecated `sort` APIs + possible renaming of function

Open
#1,983 0 comments 0 reactions 0 assignees View on GitHub
API KDocs
Dominant language
Kotlin
Stars
1.1k
Forks
83
Avg merge
4d 12h
Merged PRs (30d)
30

Description

# Add KDocs for non-deprecated `sort` APIs

The file `core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/sort.kt` holds public `sort` / `sortDesc` / `sortWith` / `sortBy` / `sortByDesc` APIs. Most of the non-deprecated `DataColumn`, `DataFrame`, and `GroupBy` overloads currently have no KDocs (only `DataColumn.sortWith` is documented, via `CommonDataColumnSortWithDocs`).

Please add concise KDocs for the current non-deprecated APIs only. Deprecated and binary-compatibility (`@AccessApiOverload`) overloads are out of scope.

### Scope

APIs needing KDocs (in `sort.kt`):

- `SortDsl` (sort DSL interface + its members)
- the `SortDsl` interface itself
- `ColumnSet.desc()`
- `SingleColumn.desc()`
- `String.desc()`
- `ColumnSet.nullsLast(flag: Boolean = true)`
- `SingleColumn.nullsLast(flag: Boolean = true)`
- `String.nullsLast(flag: Boolean = true)`
- `DataColumn.*`
- `sort()`
- `sortDesc()`
- `DataFrame.*`
- `sortBy(columns: SortColumnsSelector)`
- `sortBy(vararg cols: String)`
- `sortByDesc(columns: SortColumnsSelector)`
- `sortByDesc(vararg columns: String)`
- `sortWith(comparator: Comparator>)`
- `sortWith(comparator: (DataRow, DataRow) -> Int)`
- `GroupBy.*`
- `sortBy(vararg cols: String)`
- `sortBy(selector: SortColumnsSelector)`
- `sortByDesc(vararg cols: String)`
- `sortByDesc(selector: SortColumnsSelector)`
- `sortByGroup(nullsLast: Boolean = false, expression: DataFrameExpression)`
- `sortByGroupDesc(nullsLast: Boolean = false, expression: DataFrameExpression)`
- `sortByCountAsc()`
- `sortByCount()`
- `sortByKey(nullsLast: Boolean = false)`
- `sortByKeyDesc(nullsLast: Boolean = false)`

Already documented / out of scope: `DataColumn.sortWith(...)` (both overloads), the `SortColumnsSelector` typealias (already has a KDoc), all deprecated / `@AccessApiOverload` overloads (including the `KProperty` / `ColumnReference` `desc` / `nullsLast` / `sortBy` / `sortByDesc` variants), and the `private` helper `createColumnFromGroupExpression`.

### KDocs should explain

- What each operation returns:
- `DataColumn.sort()` / `sortDesc()` — a new sorted [ValueColumn] (ascending / descending) with the same name and type.
- `DataFrame.sortBy(...)` / `sortByDesc(...)` — a new [DataFrame] with rows reordered by the selected columns (ascending / descending); the original frame is not modified.
- `DataFrame.sortWith(...)` — a new [DataFrame] with rows reordered according to the given [Comparator] over whole [DataRow]s (or the equivalent `(a, b) -> Int` lambda).
- `GroupBy.sortBy(...)` / `sortByDesc(...)` — a new [GroupBy] with rows sorted within each group by the selected columns.
- `GroupBy.sortByGroup(...)` / `sortByGroupDesc(...)` — a new [GroupBy] with the groups themselves reordered by a value computed per group via `expression` (ascending / descending), with `nullsLast` controlling null placement.
- `GroupBy.sortByCountAsc()` / `sortByCount()` — a new [GroupBy] with groups reordered by their row count (`sortByCountAsc` ascending, `sortByCount` descending).
- `GroupBy.sortByKey(...)` / `sortByKeyDesc(...)` — a new [GroupBy] with groups reordered by their key columns (ascending / descending), with `nullsLast` controlling null placement.
- The difference between the variants:
- `sortBy` / `sortByDesc` select columns via the [SortColumnsSelector] DSL (or by column name strings) and sort ascending / descending;
- `sortWith` sorts by an explicit comparator over rows/values rather than by selected columns.
- Column selection semantics for `sortBy` / `sortByDesc`: the string overloads select top-level columns by name; the selector overloads use the sort DSL (where multi-column, `desc()`, and `nullsLast()` can be combined).
- Default null ordering, and that these operations return a new object (immutability — the receiver is not mutated).
- `See also` cross-links between the related variants (`sortBy` ↔ `sortByDesc` ↔ `sortWith`, and `DataColumn.sort` ↔ `sortDesc`).
- For `SortDsl`: what the DSL is (the receiver of the `sortBy` / `sortByDesc` column selector), and what its modifiers do — `desc()` reverses the sort order of the selected column(s), and `nullsLast(flag)` moves `null` values to the end (when `flag` is `true`). Note they are chainable and combine with `and`.

### ⚠️ Note on `sortByCount` / `sortByCountAsc` naming

> **NOTE: The `sortByCount` naming is inconsistent with the rest of this file and is misleading.**
> Everywhere else in `sort.kt` the unsuffixed name means *ascending* and the `Desc` suffix means *descending* (`sortBy`/`sortByDesc`, `sortByGroup`/`sortByGroupDesc`, `sortByKey`/`sortByKeyDesc`). But for count it is inverted: `sortByCount()` (no suffix) sorts **descending** (`sortByGroupDesc { nrow }`), the ascending variant is the oddly suffixed `sortByCountAsc()`, and there is **no** `sortByCountDesc()`. A reader will reasonably assume `sortByCount()` is ascending and be wrong.
>
> **Possible fix within this ticket, without any deprecation:** add a new `sortByCountDesc()` function (a pure, non-breaking addition) delegating to `sortByGroupDesc { nrow }`, so the conventional `...`/`...Desc` pair exists. `sortByCount()` stays as-is (still descending) for source/binary compatibility, and its KDoc documents both the actual behavior and that `sortByCountDesc()` is the preferred, consistently named alias. (Renaming or flipping the behavior of the existing functions would be a breaking change and is out of scope.)

### Acceptance criteria

- Concise KDocs are added to all non-deprecated `sort` / `sortDesc` / `sortWith` / `sortBy` / `sortByDesc` overloads listed in Scope.
- The `SortDsl` interface and its non-deprecated `desc` / `nullsLast` members are documented.
- The `GroupBy` group-wide helpers (`sortByGroup`, `sortByGroupDesc`, `sortByCountAsc`, `sortByCount`, `sortByKey`, `sortByKeyDesc`) are documented, and the KDocs make the sort direction of the confusingly named `sortByCount` (descending) vs `sortByCountAsc` (ascending) explicit.
- A consistently named `sortByCountDesc()` is added as a non-breaking alias for `sortByCount()` (see the note above); `sortByCount()` is kept and documented, no deprecation is introduced.
- Every public, non-deprecated declaration in `sort.kt` has a KDoc (the file is fully covered).
- No documentation is added for deprecated or `@AccessApiOverload` overloads.
- KDocs state the return value, that a new object is returned (no in-place mutation), and the ascending vs descending distinction.
- KDocs distinguish the column-selecting variants (`sortBy` / `sortByDesc`) from the comparator-based variant (`sortWith`).
- Style and `See also` cross-references are consistent with the existing `DataColumn.sortWith` KDoc and with the site docs at `sortBy.html`.

Contributor guide

Open the contributing guide

Research direction

Start with core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/sort.kt and compare the existing DataColumn.sortWith KDoc with the site documentation at sortBy.html. Document the listed non-deprecated declarations, add the consistently named sortByCountDesc alias, and verify every public non-deprecated declaration in sort.kt is covered without documenting deprecated or @AccessApiOverload overloads.

Written by the indexing model from the issue text.

Assessment

Tech stack
kotlin
Domain
documentation
Issue type
Documentation
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
70/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.