Fixes nulls in framecols and improves column creation situation #925

Jolanrensen · 2024-10-17T17:57:39Z

Fixes #593

This PR has turned into a collection of a couple smaller fixes:

a debug=true check has been added to FrameColumnImpl to catch cases where null leaks in the values like before.
DataColumn.Companion cleaning
- All functions gained KDocs explaining these functions do no conversions or checks and just instantiate the columns
- create() was renamed to createUnsafe() for the same reason. Together with .toColumnKind(), giving the type AnyFrame? will now instantiate a value column instead of a frame column.
- createFrameColumn() with startIndices has been deprecated. This is not the "normal" way to instantiate a FrameColumn and thus belongs somewhere else, in this case, in chunked.kt
createColumn() and guessColumnType() have been merged into createColumnGuessingType()
- All cases of createColumn were already covered by guessColumnType() aside from Iterable<DataColumn> -> ColumnGroup
- allColsMakesColGroup argument was added to createColumnGuessingType() to keep this behavior optionally. It is used by columnOf for instance, but doesn't make sense everywhere. guessValueType() was modified to handle this too.
- We now have just one place where type guessing/unifying happens
- I added tests to check the behavior is unchanged.
dataFrameOf constructors had some fixes to align behavior with other column creation functions, added tests

TODO: check whether other uses of DataColumn.create(Unsafe) need to be swapped to createWithTypeInference/createColumnGuessingType

…KType.toColumnKind()

… what it does

…ing KDocs for that suite of functions to explain what they're for

…o `chunked`

…from before the PR. Added allColsMakesColGroup argument for createColumnGuessingType() and guessValueType() so the old behavior of createColumn() is now controlled in the same place as all other conversions

core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/chunked.kt

…or CSV and other parsing usages

# Conflicts: # core/src/test/kotlin/org/jetbrains/kotlinx/dataframe/api/constructors.kt # plugins/kotlin-dataframe/src/org/jetbrains/kotlinx/dataframe/plugin/impl/DataFrameAdapter.kt

github-actions · 2024-10-22T12:52:36Z

Generated sources will be updated after merging this PR.
Please inspect the changes in here.

core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/DataColumn.kt

core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/constructors.kt

core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/TypeUtils.kt

core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/api/parse.kt

core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/columns/constructors.kt

core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/io/readJson.kt

…Suggestion, working on feedback

…e check from guessing type. May be unexpected behavior. Fixed tests

zaleslaw · 2024-10-30T12:29:30Z

core/src/test/kotlin/org/jetbrains/kotlinx/dataframe/types/UtilTests.kt

+            sequenceOf(columnOf("a"), columnOf(1)),
+            allColsMakesRow = true,
+        ) shouldBe typeOf<DataRow<*>>()
+        guessValueType(


split two guessValueType?

then it would need to be split in 4 because we also have listifyValues: Boolean. I think in this case it's good to have all logic of values -> column type of the entire library handled by one function guessValueType(). This makes it easier to debug and reason about.

core/src/test/kotlin/org/jetbrains/kotlinx/dataframe/io/ParserTests.kt

core/src/test/kotlin/org/jetbrains/kotlinx/dataframe/columns/DataColumns.kt

zaleslaw · 2024-10-30T12:34:33Z

core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/columns/FrameColumnImpl.kt

+        // Checks for nulls in the `values` list.
+        // This only runs with `kotlin.dataframe.debug=true` in gradle.properties.
+        if (BuildConfig.DEBUG) {
+            require(!values.anyNull()) { "FrameColumn cannot null values." }


What will happen if we apply this check not only during DEBUG mode?

Then each time we create a frame column, we'll have to check all values for nulls, which at worst is O(n). For efficiency it may be best to at least have a zero-checks path available for library calls.

zaleslaw · 2024-10-30T12:36:45Z

core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/TypeUtils.kt

- * Returns the value type of the given [values] sequence.
+ * Returns the guessed value type of the given [values] sequence.
+ *
+ * This function analyzes all [values] once and returns the expected column type.


Did we cover in tests the situation when one column in file or in List has different types?

We do have a lot of tests of this specific function: https://github.com/Kotlin/dataframe/blob/nulls-in-framecols/core/src/test/kotlin/org/jetbrains/kotlinx/dataframe/types/UtilTests.kt.
These cases are probably covered by other column creation tests, like in https://github.com/Kotlin/dataframe/blob/nulls-in-framecols/core/src/test/kotlin/org/jetbrains/kotlinx/dataframe/api/constructors.kt

zaleslaw · 2024-10-30T12:41:13Z

core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/DataColumn.kt

+         *
+         * This is generally safe, as [T] can be inferred, and more efficient than [createWithTypeInference].
+         *
+         * Be careful when casting occurs; Values in [values] are NOT checked to adhere to the given/inferred type [T],


It is unsafe because of type? For me createUnsafe creates weird reference to something like sun.misc.Unsafe. Could we rename it somehow to the createWithoutTypeChecking or createAndFailFast

createAndFailFast

haha, another title could be createRaw, but createWithoutTypeChecking might also be clear. It's unsafe in the sense that it solely uses the given type T to decide which type of column to instantiate. It doesn't check any of the values, which makes it fast, but, well, unsafe.

Hmm, on the other hand, then we can have DataColumn.createWithoutTypeInference(..., infer = Infer.Type). That's gonna be confusing haha. We need a title that says it will choose createValueColumn, createColumnGroup, or createFrameColumn based on the type T

Alright, I think I'm gonna go with:

createByInference

createByType

core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/DataColumn.kt

zaleslaw

I've suggested just rename or answer some my questions before merging

Jolanrensen · 2024-10-30T12:51:32Z

@zaleslaw Thanks! I'll have a look. I'll also check the binary compatibility now that plugin is merged. I suspect I broke some stuff.

Jolanrensen · 2024-10-31T14:00:41Z

Alright, thanks for the reviews :) they were most helpful! I think I covered all now, so I'll merge so we won't get further out of sync with other branches. Let me know if I missed something major, then I'll fix it :)

Jolanrensen added 5 commits October 17, 2024 14:38

adding debug mode check for nulls in FrameColumnImpl and simplifying …

a7e4874

…KType.toColumnKind()

renamed guessColumnType -> createColumnGuessingType to better reflect…

e29991c

… what it does

small kdoc enhancements to clarify DataColumn.createX functions

6f73354

renamed DataColumn.create to DataColumn.crateUnsafe and added clarify…

798edb0

…ing KDocs for that suite of functions to explain what they're for

deprecated DataColumn.createFrameColumn with startIndices, moved it t…

2535378

…o `chunked`

Jolanrensen force-pushed the nulls-in-framecols branch 2 times, most recently from 115033a to 48fc9ff Compare October 18, 2024 15:16

added some tests to confirm behavior from createColumnGuessingType() …

32bee07

…from before the PR. Added allColsMakesColGroup argument for createColumnGuessingType() and guessValueType() so the old behavior of createColumn() is now controlled in the same place as all other conversions

Jolanrensen force-pushed the nulls-in-framecols branch from 48fc9ff to 32bee07 Compare October 18, 2024 17:41

Jolanrensen requested review from koperagen and zaleslaw October 18, 2024 17:43

koperagen reviewed Oct 18, 2024

View reviewed changes

core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/chunked.kt Outdated Show resolved Hide resolved

Changed parsing to use createColumnGuessingType() to fix issue #593 f…

64d1f99

…or CSV and other parsing usages

Jolanrensen force-pushed the nulls-in-framecols branch from f80fd18 to 6c68f18 Compare October 21, 2024 09:46

moved to chunkImpl

88ee08d

Jolanrensen force-pushed the nulls-in-framecols branch from 6c68f18 to 88ee08d Compare October 21, 2024 09:49

added tests and some small fixes for dataFrameOf constructors

1633e2c

Jolanrensen marked this pull request as ready for review October 21, 2024 12:49

Jolanrensen mentioned this pull request Oct 22, 2024

☂ CSV rework #827

Open

24 tasks

Jolanrensen requested a review from koperagen October 22, 2024 12:41

Merge branch 'master' into nulls-in-framecols

42e2d67

# Conflicts: # core/src/test/kotlin/org/jetbrains/kotlinx/dataframe/api/constructors.kt # plugins/kotlin-dataframe/src/org/jetbrains/kotlinx/dataframe/plugin/impl/DataFrameAdapter.kt

koperagen requested changes Oct 24, 2024

View reviewed changes

Jolanrensen added 2 commits October 25, 2024 17:07

refactored suggestedType+guessTypeWithSuggestedAsUpperbound into Type…

dfaf46e

…Suggestion, working on feedback

forgot defaultValue in createColumnGuessingType(), removed column siz…

222086a

…e check from guessing type. May be unexpected behavior. Fixed tests

Jolanrensen force-pushed the nulls-in-framecols branch from e007cc3 to 222086a Compare October 28, 2024 11:50

clarifying some docs based on feedback

baaa014

Jolanrensen requested review from zaleslaw and koperagen and removed request for zaleslaw October 28, 2024 12:26

zaleslaw reviewed Oct 30, 2024

View reviewed changes

core/src/test/kotlin/org/jetbrains/kotlinx/dataframe/io/ParserTests.kt Outdated Show resolved Hide resolved

zaleslaw reviewed Oct 30, 2024

View reviewed changes

core/src/test/kotlin/org/jetbrains/kotlinx/dataframe/columns/DataColumns.kt Outdated Show resolved Hide resolved

zaleslaw reviewed Oct 30, 2024

View reviewed changes

core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/DataColumn.kt Outdated Show resolved Hide resolved

zaleslaw requested changes Oct 30, 2024

View reviewed changes

Jolanrensen added 3 commits October 30, 2024 14:40

renamed DataColumn.createX() functions and handled other feedback

1181d57

Merge branch 'master' into nulls-in-framecols

33ab8ca

solved binary compatibility issues

8a8650a

Jolanrensen requested a review from zaleslaw October 30, 2024 16:05

Jolanrensen merged commit 2c68cf4 into master Oct 31, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes nulls in framecols and improves column creation situation #925

Fixes nulls in framecols and improves column creation situation #925

Jolanrensen commented Oct 17, 2024 •

edited

Loading

github-actions bot commented Oct 22, 2024

zaleslaw Oct 30, 2024

Jolanrensen Oct 30, 2024

zaleslaw Oct 30, 2024 •

edited

Loading

Jolanrensen Oct 30, 2024

zaleslaw Oct 30, 2024

Jolanrensen Oct 30, 2024

zaleslaw Oct 30, 2024

Jolanrensen Oct 30, 2024

Jolanrensen Oct 30, 2024 •

edited

Loading

Jolanrensen Oct 30, 2024 •

edited

Loading

zaleslaw left a comment

Jolanrensen commented Oct 30, 2024

Jolanrensen commented Oct 31, 2024

Fixes nulls in framecols and improves column creation situation #925

Fixes nulls in framecols and improves column creation situation #925

Conversation

Jolanrensen commented Oct 17, 2024 • edited Loading

github-actions bot commented Oct 22, 2024

zaleslaw Oct 30, 2024

Choose a reason for hiding this comment

Jolanrensen Oct 30, 2024

Choose a reason for hiding this comment

zaleslaw Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Jolanrensen Oct 30, 2024

Choose a reason for hiding this comment

zaleslaw Oct 30, 2024

Choose a reason for hiding this comment

Jolanrensen Oct 30, 2024

Choose a reason for hiding this comment

zaleslaw Oct 30, 2024

Choose a reason for hiding this comment

Jolanrensen Oct 30, 2024

Choose a reason for hiding this comment

Jolanrensen Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Jolanrensen Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

zaleslaw left a comment

Choose a reason for hiding this comment

Jolanrensen commented Oct 30, 2024

Jolanrensen commented Oct 31, 2024

Jolanrensen commented Oct 17, 2024 •

edited

Loading

zaleslaw Oct 30, 2024 •

edited

Loading

Jolanrensen Oct 30, 2024 •

edited

Loading

Jolanrensen Oct 30, 2024 •

edited

Loading