Repository navigation
Conversation
…ataFrame creation methods
There was a problem hiding this comment.
Thanks for this thorough work! One blocking doc inaccuracy needs fixing before merge.
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=a4821ed4-d9f7-4e51-8ae2-83dad7a9e897
|
|
||
| /** | ||
| * Creates a [DataFrame] where the values of each column are produced by [valuesBuilder] | ||
| * for that column's name; the column type comes from [T]. |
There was a problem hiding this comment.
Blocking: "the column type comes from [T]" is wrong — the implementation uses TypeSuggestion.InferWithUpperbound(typeOf<T>()), the same upper-bound semantics this PR documents for columnOf (Problem 2).
Failure: dataFrameOf("a").invoke<Number> { listOf(1, 2) } yields an Int column, not Number. The top-level dataFrameOf(vararg header, fill) delegates here, and dataFrameOf(header: Iterable<T>, fill) behaves identically, so both inherit the gap.
Fix: say the type is inferred from the values with [T] as an upper bound (reuse the columnOf wording, ideally as a shared ConstructorsDocs fragment), apply it to both fill-lambda dataFrameOf overloads too, and pin it with a test.
| @TransformDataFrameExpressions | ||
| fun createComputedColumnReference_strings() { | ||
| // SampleStart | ||
| val isAdult = column<Boolean>("isAdult") { "age"<Int>() > 18 } |
There was a problem hiding this comment.
The new _strings website sample uses the Invoked String API ("age"<Int>()). DOCUMENTATION_GUIDELINES.md and the operation-website-documentation skill ban it explicitly as outdated.
Failure: the createColumn page now teaches the not-recommended style in a tab meant to show the String API.
Fix: column<Boolean>("isAdult") { getValue<Int>("age") > 18 }. Using the same df as the _properties tab would also keep the two tabs equivalent.
Closes #1954, closes #1955
About 50 public declarations in
api/constructors.kthad no KDoc, and the constructor families hadminimal test coverage. This PR documents every non-deprecated public API there and pins its behaviour
with tests: name generation, row-major fill, type-inference bounds and the thrown exceptions.
Files
core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/constructors.ktcolumn/valueColumn/columnGroup/frameColumnfamilies,ColumnDelegate,columnOf/column, the previously undocumenteddataFrameOfoverloads,DataFrameBuildermembers,pathOfcore/src/main/kotlin/org/jetbrains/kotlinx/dataframe/documentation/ConstructorsDocs.kt(new)dataFrameOfcontract with@throwscore/src/main/kotlin/org/jetbrains/kotlinx/dataframe/documentation/DocumentationUrls.ktPathOfalias →columnselectors.html#column-resolverscore/src/test/kotlin/org/jetbrains/kotlinx/dataframe/api/constructors.ktcore/src/test/kotlin/org/jetbrains/kotlinx/dataframe/api/columnAccessors.kt(new)ColumnAccessorsTests: the accessor familiescore/src/test/kotlin/org/jetbrains/kotlinx/dataframe/samples/api/Create.ktdocs/StardustDocs/topics/createColumn.md,createDataFrame.md,ColumnSelectors.md### columnsection,columnOftype nuances,DataFrameBuilderfill/random,pathOfin Column Resolvers;columnadded to the API-reference lineProblem 1:
random*range bounds are asymmetricdataFrameOf("x").randomLong(100, 1L..2L)never yields2L, whilerandomInt(100, 1..2)yields both
1and2: theClosedRangeoverloads passendInclusivetoRandom.nextLong(from, until),where it is excluded, but
randomInt(IntRange)includes both bounds.endInclusive(a behaviour change beyond this PR).
@param [range]docs state the bounds per function; behaviour is unchanged.builder random columns have declared types and stay in rangepinsrandomLong(100, 1L..2L)→ all1LandrandomInt(100, 1..2)→ both values.Problem 2:
columnOf's reified argument is an upper bound, not the column typecolumnOf<Number>(1, 2)produces a column of typeInt, notNumber— the type isinferred from the values within the bound;
Numberonly results from mixed values(
columnOf<Number>(1, 2.5)).columnOfuseTas declared —that would change behaviour and duplicate
column(Iterable), which already does exactly that.columnOf infers a narrower type from homogeneous values(Int),columnOf of mixed values has the upper bound type(Number).Problem 3: empty header always throws
dataFrameOf(emptyList<String>(), emptyList())throwsIllegalArgumentException("Number of values 0 is not divisible by number of columns 0"), while anon-empty header with zero values creates a 0-row frame.
DataFrameBuilder.invokeand pinned; behaviour unchanged.dataFrameOf with empty header throws even without values.Problem 4: the empty path has no name
ColumnPath.EMPTY.name()throwsNoSuchElementException("List is empty").null(a public behaviour change); or document and pin.pathOfand in the Column Resolvers section; behaviour unchanged.empty path has no name.Tests
Before,
ConstructorsTestscovered ~12 scenarios; the accessor families, mostdataFrameOfoverloadsand all
DataFrameBuildermembers were untested. Expected values come from probe runs; static types areasserted with explicit type annotations.
ColumnAccessorsTests(14)@ColumnNameoverrides, access by name/path, typed and untyped overloads, nesting via a receiver,named, the computed reference inselect/add/update, the deprecatedKPropertyoverloadcolumnOf/column(10)Any?, declared type ofcolumn(Iterable), empty column, column groups and frame columnsdataFrameOf(14)untitled, row-major fill, indivisible values and the empty header with exact messages, header variants, deprecated-overload equivalenceDataFrameBuilder(8)fill/nullsnullability,random*types and bounds,Sequenceinvoke, per-column inferenceemptyDataFrame/pathOf(6)Unit→ 0×0, a class without@DataSchemagets columns from properties, vsDataFrame.empty; empty path, last-segment nameKDoc example …(2), samples (7)dataFrameOfpairs example was unbacked before)Note for the reviewer
Beyond the ticket
createcolumn.html,createdataframe.htmlandcolumnselectors.html, and those pages had no sections describing thedocumented APIs.
Conventions
@samplecannot reference coretest sources); each is backed by a test named
KDoc example ….Create.kt; the:samplesmigration (Move Writerside Korro samples out of:coreand migrate them toSampleHelper#898) is a separate ticket.The computed-column sample is rendered as Properties/Strings tabs.
Open question
randomLong/randomDoubletreatClosedRange.endInclusiveas excluded whilerandomInt(IntRange)includes both bounds. Should the
ClosedRangeoverloads include the upper bound for symmetry?Not changed in this PR.