Skip to content

Added merge KDocs, fixed notNull() and into(path) bugs - #2210

Draft
zaleslaw wants to merge 4 commits into
masterfrom
issues-1975
Draft

zaleslaw wants to merge 4 commits into
masterfrom
issues-1975

Conversation

@zaleslaw

@zaleslaw zaleslaw commented Nov 3, 2026

Copy link
Copy Markdown
Collaborator

Closes #1975

The public merge APIs had no KDoc at all. This PR adds concise KDoc for all 12 non-deprecated
merge APIs, with every example backed by a runnable test, and rebuilds the merge page of the
documentation website to match. Two bugs found while writing the tests are fixed.

Behaviour change

notNull() now applies in intoList() too: df.merge { a and b }.notNull().by(" ").intoList() skips
null values (before: returned them all). into(path) now creates missing parent column groups:
into(pathOf("person", "fullName")) merges into a new nested group instead of throwing
Column 'fullName' not found among [top-level columns]. Single-segment paths, replacement of existing
targets and the full core suite are unaffected.

Files

File What changed
core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/merge.kt KDoc for the 12 non-deprecated APIs (MergeDocs grammar, shared KoDEx snippets); notNull filter shared by into/intoList; into(path) group selection made path-aware
core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/documentation/DocumentationUrls.kt new Merge website-link entry
samples/src/test/kotlin/org/jetbrains/kotlinx/dataframe/samples/api/MergeSamples.kt 12 tests backing every @sample in the new KDocs and every sample of the rebuilt page, with Sample Helper outputs
core/src/test/kotlin/org/jetbrains/kotlinx/dataframe/api/merge.kt 17 behaviour and claim tests
docs/StardustDocs/topics/merge.md page rebuilt: Writerside summaries, corrected grammar (transform, not merger), full notes incl. notNull/asStrings/into(path), Parameters section, split round-trip example
docs/StardustDocs/topics/_shadow_resources.md, docs/StardustDocs/resources/api/merge/ 10 new resource entries and 10 generated HTML outputs of the page samples (first HTML this page ever had)
samples/build.gradle.kts merge.md registered in the Korro docs config
core/src/test/kotlin/org/jetbrains/kotlinx/dataframe/samples/api/Modify.kt the five legacy merge sample functions deleted — page samples migrated to :samples

Problem 1: notNull() silently ignored in intoList()

  • Problem: df.merge { a and b }.notNull().intoList() returned the unfiltered values, and an
    all-null column-group row stayed, while into("col") filtered both.
  • Options: filter inside intoList; keep unfiltered and document the exception. Rejected the
    second: notNull is the documented contract of the merge pipeline, not an into-only option.
  • Chosen: one shared private filterNotNullAndEmptyRows used by into and both intoList
    implementations. For callers: notNull now affects intoList results.
  • Test: intoList excludes null values when notNull is set,
    intoList excludes an all-null row value from a column group when notNull is set (both failed before).

Problem 2: into(path) threw for any new nested path

  • Problem: df.merge { age }.by{}.into(pathOf("person", "fullName")) threw
    Column 'fullName' not found among [top-level columns] — also for a new sub-path inside an existing
    group. Only single-segment paths and fully existing nested targets worked.
  • Options: fix the selector inside into(path); pre-create parent groups in under. Rejected the
    second: the failure is in the convert selector — getColumnGroup(path) returns an eagerly resolved,
    path-less ColumnGroup that re-resolves by its leaf name only.
  • Chosen: convert { columnGroup(mergePath) } — a path-aware accessor. For callers: missing
    intermediate groups are now created instead of an exception.
  • Test: into a new nested path creates the missing column groups,
    into a new sub-path inside an existing column group (both threw before).

Tests

Before this PR, merge had no KDoc-backed examples; behaviour was covered only by a few
testSets/person2 tests. All expected values come from real runs.

Test What it pins down
selection order; merged-column position; the String overload values follow the selector order; the merged column takes the first selected column's place; merge("a", "b") equals the DSL form
by(separator, ...) join; null rendering all 5 parameters reach joinToString; a null value renders as null
notNull on every final step nulls and all-null group rows are dropped in into and in intoList, with the default and with a custom transform
by { } contract; asStrings the transform receives the source row as receiver and the collected values as a list; asStrings() equals by(", ")
into target handling existing targets are replaced entirely in place; missing parent groups are created
MergeSamples (12) every @sample in the KDocs and every sample of the website page runs and produces the asserted result

Note for the reviewer

  • The page is rebuilt beyond the KDoc the ticket asked for: it had no section the new KDoc links
    could point to, never explained notNull or asStrings, called the lambda merger where the API
    says transform, and had no generated HTML outputs.
  • getColumnGroup(path) returns an eagerly resolved, path-less ColumnGroup; used as a column set
    inside select/convert selectors it re-resolves by its leaf name only and breaks nested paths —
    the path-aware columnGroup(path) is the correct form there. This was the root cause of Problem 2.
  • DataRow.isEmpty() is used deliberately in the shared filter with @Suppress("DEPRECATION"):
    its prescribed replacement values().all { it == null } is not equivalent for row values collected
    from column groups — a probe showed the filtering invert there (DataRowImpl.values() is df-based,
    isEmpty() is owner-based). Should the ReplaceWith itself be fixed? Not changed in this PR.

…e `merge` API, including features like `notNull`, `by`, `asStrings`, `into`, and `intoList`.
…sources**

- Added comprehensive documentation for the `merge` API, including features like `notNull`, `by`, `asStrings`, `into`, and `intoList`.
- Updated tests to demonstrate various usages of `merge`, including handling heterogeneous columns and transforming merged values.
- Included multiple examples for the newly added documentation.
- Introduced new HTML resources supporting the `merge` documentation.
- Removed outdated `merge` tests in `Modify.kt`, replaced with refined tests in the new `MergeSamples` file.
- Updated `build.gradle.kts` to link the new `merge.md` documentation.

@jetbrains-air jetbrains-air Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add KDocs for merge APIs

2 participants