Documented all four `pivot {}.with {}` overloads by mvanhorn · Pull Request #2177 · Kotlin/dataframe

mvanhorn · 2026-10-21T14:42:04Z

Background

It addresses the behavior reported in issue #1994.

Summary of the fix

This change edits core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/with.kt. Across 1 file the diff adds 45 lines and removes 0.

Tests

The local test run for the dataframe change to core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/with.kt hit its time limit before finishing.

Fixes #1994

AI was used for assistance.

Fixes Kotlin#1994

Jolanrensen · 2026-10-21T15:19:42Z

+ * A single result is used directly; multiple results are collected for the corresponding pivot cell.
+ *
+ * For more information: {@include [DocumentationUrls.Pivot]}
+ *


Some examples would be nice to see in each function. Maybe we already have them ready in :samples if they're used for the website. You can use @sample [fully qualified path to sample in :samples] to include a specific example. KoDEx will insert it when publishing

jetbrains-air

Thanks for taking this on — a couple of accuracy points inline to sort out, 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=333d13f9-071a-4d94-8fe8-b3e225a7b660

jetbrains-air · 2026-10-21T15:30:42Z


+/**
+ * Aggregates this [Pivot] by evaluating the given [row expression][expression] on every row of each pivot group.
+ * A single result is used directly; multiple results are collected for the corresponding pivot cell.


Blocking: "multiple results are collected" hides the actual result type. withExpr ends in yieldOneOrMany (impl/aggregation/aggregations.kt:29-40), which yields null with nullable V for an empty group, the bare value typed V for exactly one row, and a List<V> typed List<V> for two or more. Same wording at line 56 for PivotGroupBy.

Failure scenario: a reader of this KDoc writes typed.pivot { key }.groupBy { name }.with { value } expecting an Int column and gets List<Int> — exactly what PivotTests.kt:152 asserts (data["age"].type() shouldBe typeOf<List<Int>>()). Any downstream convert<Int>() / cast then fails at runtime, and the KDoc gave no warning.

Suggested fix: spell the three cases out, e.g. "For each pivot cell: an empty group yields null, a group of exactly one row yields that single value (type V), and a group of several rows yields a List<V>." Since this applies identically to Pivot.with and PivotGroupBy.with, it is a good candidate for one shared KDoc fragment @included by both, per KDOC_GUIDELINES.md.

jetbrains-air · 2026-10-21T15:30:42Z

+/**
+ * Transforms this [ReducedPivotGroupBy] by evaluating the given [row expression][expression] on the single row
+ * selected by the reducer in each group defined by the grouping and pivot keys.
+ * If [expression] returns a column reference, it is resolved to that column's value in the selected row.


The column-reference note is documented on ReducedPivotGroupBy.with only, but all four overloads do it. PivotGroupBy.with routes through withExpr, which has the identical if (value is AnyColumnReference) value.getValue(it) branch (impl/aggregation/aggregations.kt:95-102); Pivot.with delegates to PivotGroupBy.with and ReducedPivot.with delegates to ReducedPivotGroupBy.with, so they inherit it too.

Failure scenario: documenting it on one of four overloads reads as a deliberate distinction. Someone on Pivot.with or PivotGroupBy.with concludes .with { value } hands back a ColumnReference object and writes a redundant .with { value() } workaround — or, reading the asymmetry the other way, a later maintainer "fixes" the resolution out of withExpr. The behaviour is deliberate and load-bearing: PivotTests.kt:170-175 pins PivotGroupBy.with { value } as a "special case that looks very odd, but kept it for compatibility".

Suggested fix: move the sentence into a shared KDoc fragment and @include it in all four overloads.

@sample

Spell out the pivot cell result types for Pivot.with and PivotGroupBy.with (empty group yields null, one row yields V, several rows yield List<V>) in one shared fragment, and document column-reference resolution once and include it in all four overloads. Add @sample examples from :samples to every overload, with new withOnPivotGroupBy and withOnReducedPivotGroupBy samples, and bracket the @PARAM tags so KoDEx renders them.

mvanhorn · 2026-10-22T08:10:26Z

@Jolanrensen thanks, good call on :samples. d87c1ab adds an @sample to each of the four with overloads, reusing the existing withOnPivot_* and pivotWith_* samples and adding withOnPivotGroupBy and withOnReducedPivotGroupBy for the grouped ones.

It also covers the two accuracy points: the null / V / List<V> cell result is now one shared fragment included by Pivot.with and PivotGroupBy.with, and the column-reference note is a shared fragment included by all four. Locally, ktlint, the PivotSamples tests and ./gradlew clean processKDocsMain korro syncExampleFolders all pass.

Jolanrensen · 2026-10-22T13:04:53Z

 // region Pivot

+/**
+ * Aggregates this [Pivot] by evaluating the given [row expression][expression] on every row of each pivot group.


One small addition might be to link to the Pivot Grammar just before the examples. Other than that I think it's quite complete

Jolanrensen · 2026-10-22T13:06:38Z

 // region ReducedPivot

+/**
+ * Transforms this [ReducedPivot] by evaluating the given [row expression][expression] on the single row


*Aggregates

Jolanrensen · 2026-10-22T13:07:55Z

 // region ReducedPivotGroupBy

+/**
+ * Transforms this [ReducedPivotGroupBy] by evaluating the given [row expression][expression] on the single row


*Aggregates

Fixed in c353080, both reduced overloads now open with "Aggregates". The same commit adds the Pivot Grammar link just before the examples on Pivot.with.

…overloads Pivot.with now points to the Pivot Grammar just before its examples, and the ReducedPivot.with and ReducedPivotGroupBy.with KDocs open with "Aggregates" instead of "Transforms", per review.

jetbrains-air

Nicely done — the accuracy points are all sorted out, happy to approve this!

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=b37f3da2-d357-48fd-aa2b-0cc0e17194bb

jetbrains-air · 2026-10-23T12:16:20Z


+/**
+ * Aggregates this [ReducedPivot] by evaluating the given [row expression][expression] on the single row
+ * selected by the reducer in each pivot group.


Both reduced overloads describe the reducer as selecting the single row, with no mention of the no-row case — while the two non-reduced overloads now spell their result contract out via PivotWithOneOrManyResultSnippet. That asymmetry reads as "reduced always yields a V", but ReducedPivotGroupBy.with is reducer(this)?.let { … } (with.kt:138-140), so a group with no selected row yields null and the cell type widens to V?.

Failure scenario: df.groupBy { city }.pivot { isHappy }.maxBy { age }.with { name.lastName } on a sparse pivot — I ran it, and every key column comes back String? with null in the city/key combinations that have no rows (ReducedPivot.first { … } with no match is worse: Nothing?). A reader following this KDoc treats the cells as non-null String and gets an NPE or a failing cast/convert downstream. Sparse groupBy × pivot is the normal case, so this bites more often than the List<V> one you already documented.

Suggested fix: add a third shared fragment next to the existing two, e.g. "For each pivot cell: a group with no row selected by the reducer yields null (type V?), otherwise the value produced by the expression (type V).", and @include it in ReducedPivot.with and ReducedPivotGroupBy.with.

okay this is a good note, afterwards I believe it's good to merge :)

Added in 6009beb: a shared PivotWithReducedResultSnippet fragment says a pivot cell whose group has no row selected by the reducer is null (type V?), otherwise the expression value (type V), and both ReducedPivot.with and ReducedPivotGroupBy.with include it.

The red "Build Preview of Generated Code" check isn't from this change. Its "Commit and push if changes" step fails with a 403 because the workflow token on a fork PR can't push to Kotlin/dataframe, and it failed the same way on the previous head.

Add a shared KDoc fragment saying a pivot cell whose group has no row selected by the reducer yields null (type V?), otherwise the expression value (type V), and include it in ReducedPivot.with and ReducedPivotGroupBy.with, per review.

Document all four pivot with overloads

7de7359

Fixes Kotlin#1994

Jolanrensen added the KDocs Improvements or additions to KDocs label Oct 21, 2026

Jolanrensen self-requested a review October 21, 2026 15:16

Jolanrensen reviewed Oct 21, 2026

View reviewed changes

Jolanrensen changed the title ~~Document all four pivot with overloads~~ Documented all four pivot {}.with {} overloads Oct 21, 2026

jetbrains-air Bot requested changes Oct 21, 2026

View reviewed changes

Jolanrensen reviewed Oct 22, 2026

View reviewed changes

Link Pivot grammar in Pivot.with KDoc and say Aggregates for reduced …

c353080

…overloads Pivot.with now points to the Pivot Grammar just before its examples, and the ReducedPivot.with and ReducedPivotGroupBy.with KDocs open with "Aggregates" instead of "Transforms", per review.

Jolanrensen self-requested a review October 23, 2026 12:02

Jolanrensen approved these changes Oct 23, 2026

View reviewed changes

jetbrains-air Bot approved these changes Oct 23, 2026

View reviewed changes

Document the null result of reduced pivot with

6009beb

Add a shared KDoc fragment saying a pivot cell whose group has no row selected by the reducer yields null (type V?), otherwise the expression value (type V), and include it in ReducedPivot.with and ReducedPivotGroupBy.with, per review.

Conversation

mvanhorn commented Oct 21, 2026

Background

Summary of the fix

Tests

Uh oh!

Jolanrensen Oct 21, 2026

Choose a reason for hiding this comment

Uh oh!

jetbrains-air Bot left a comment

Choose a reason for hiding this comment

Uh oh!

jetbrains-air Bot Oct 21, 2026

Choose a reason for hiding this comment

Uh oh!

jetbrains-air Bot Oct 21, 2026

Choose a reason for hiding this comment

Uh oh!

mvanhorn commented Oct 22, 2026

Uh oh!

Jolanrensen Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

Jolanrensen Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

Jolanrensen Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

mvanhorn Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

jetbrains-air Bot left a comment

Choose a reason for hiding this comment

Uh oh!

jetbrains-air Bot Oct 23, 2026

Choose a reason for hiding this comment

Uh oh!

Jolanrensen Oct 23, 2026

Choose a reason for hiding this comment

Uh oh!

mvanhorn Oct 24, 2026

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants