Kotlin / Kotlin/dataframe

`indices()` on `BaseColumn` is ambiguous for a `ColumnGroup`

Open
#2,067 2 comments 0 reactions 1 assignee Claimed by @zaleslaw View on GitHub
bug
Dominant language
Kotlin
Stars
1.1k
Forks
83
Avg merge
4d 12h
Merged PRs (30d)
30

Description

### `indices()` on `BaseColumn` is ambiguous for a `ColumnGroup`

`indices()` exists on both `BaseColumn` and `DataFrame`:

| Declaration | Location |
|---|---|
| `public fun AnyBaseCol.indices(): IntRange = 0 until size()` | `core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/DataColumn.kt:325` |
| `public fun DataFrame<*>.indices(): IntRange = 0 until rowsCount()` | `core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/indices.kt:38` |

`ColumnGroup` derives from both (`core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/columns/ColumnGroup.kt`):

```kotlin
public interface ColumnGroup :
BaseColumn>,
DataFrame {
```

Neither candidate is more specific for a column group, so calling `indices()` on one does not compile.

#### Reproducer

Put this file at
`samples/src/test/kotlin/org/jetbrains/kotlinx/dataframe/samples/api/IndicesAmbiguityRepro.kt`:

```kotlin
package org.jetbrains.kotlinx.dataframe.samples.api

import org.jetbrains.kotlinx.dataframe.api.indices
import org.jetbrains.kotlinx.dataframe.indices
import org.junit.Test

class IndicesAmbiguityRepro : TestBase {
val df = peopleDf

@Test
fun repro() {
df.name.indices()
}
}
```

`df.name` is a `ColumnGroup` — `peopleDf` groups `firstName` and `lastName` into `name`.
Line 12 is `df.name.indices()`, column 17 is the `indices` token.

Run:

```
./gradlew :samples:compileTestKotlin
```

Output:

```
> Task :samples:compileTestKotlin FAILED
e: file:///.../samples/src/test/kotlin/org/jetbrains/kotlinx/dataframe/samples/api/IndicesAmbiguityRepro.kt:12:17 Overload resolution ambiguity between candidates:
fun DataFrame<*>.indices(): IntRange
fun BaseColumn<*>.indices(): IntRange

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':samples:compileTestKotlin' (registered by plugin 'org.jetbrains.kotlin.jvm').
> A failure occurred while executing org.jetbrains.kotlin.compilerRunner.btapi.BuildToolsApiCompilationWork
> Compilation error. See log for more details
```

#### Do not check this from inside `:core`

The same call **compiles** in `:core` when the calling file sits in package
`org.jetbrains.kotlinx.dataframe.api`. There `DataFrame.indices()` comes from the same package
and loses to the explicit `import org.jetbrains.kotlinx.dataframe.indices`, so only one candidate
is considered and the ambiguity never appears. A probe in `:core` returns `0..2` for a
three-row column group and looks perfectly healthy. `:samples` is the module that reflects a
user's import set.

#### The property form is not affected

`public val AnyCol.indices: IntRange` (`DataColumn.kt:317`) is declared on `AnyCol`, which is
`DataColumn<*>`. `ColumnGroup` does not derive from `DataColumn` — that is deliberate, per its
own KDoc: "It derives not from `DataColumn`, but from `BaseColumn` to avoid API clashes between
`DataFrame` and `DataColumn`." So the property simply does not apply to a column group, and
there is nothing to be ambiguous about. Only the function form is broken.

#### Why it happens

The placement rule is written in the types themselves.

`BaseColumn`:

> Column operations that doesn't clash by signature with `DataFrame` operations can be defined
> for `BaseColumn`

`DataColumn`:

> Column operations that have signature clash with `DataFrame` API (`filter`, `take`, `map` etc.)
> are defined for `DataColumn` and not for `BaseColumn`.

`indices()` does clash with `DataFrame.indices()`, so by that rule it belongs on `DataColumn`,
next to `filter`/`take`/`map`. Putting it on `BaseColumn` looks like an oversight.

#### Suggested fix

Narrow the receiver of `indices()` from `AnyBaseCol` to `AnyCol`. The function and the `indices`
property then agree on the receiver, and a column group resolves to `DataFrame.indices()` — which
returns the same range anyway, because `ColumnGroupImpl.size()` is `rowsCount()`
(`core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/columns/ColumnGroupImpl.kt`):

```kotlin
override fun size() = rowsCount()
```

Impact:

- `ValueColumn` and `FrameColumn` are `DataColumn`, so they are unaffected.
- The only in-repo use of `indices` on a `ColumnGroup` receiver is
`core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/reverse.kt:91`:
```kotlin
public fun ColumnGroup.reverse(): ColumnGroup = get(indices.reversed())
```
It uses the *property*. There are exactly two `indices` properties — `AnyCol.indices` and the
`internal val AnyFrame.indices` (`DataFrame.kt:182`) — and the first does not apply to a
column group, so this line already goes through the `DataFrame` side. No internal caller
depends on the function form for column groups.
- Narrowing the receiver of a public extension is source-breaking for any caller holding a
`BaseColumn`, so this needs an `apiDump` and probably a deprecation cycle rather than a
straight edit.

#### Current behaviour, for reference

Measured on a three-element column, and on a column group and a frame column built from a
three-row dataframe:

| Call | Result |
|---|---|
| `col.indices` | `0..2` |
| `col.indices()` | `0..2` |
| `base.indices()`, where `val base: AnyBaseCol = col` | `0..2` |
| `group.indices()` (from `:core`, where it compiles) | `0..2` |
| `group.size()` / `group.rowsCount()` | `3` / `3` |
| `frameCol.indices` / `frameCol.indices()` | `0..2` |
| `frameCol.size()` | `3` |

`IndicesTests` (`core/src/test/kotlin/org/jetbrains/kotlinx/dataframe/api/indices.kt`, region
`on a column`) pins this down. The column-group case goes through a receiver explicitly typed as
`AnyBaseCol`, because that is the only way to call it today without hitting the ambiguity.

Tests are here: https://github.com/Kotlin/dataframe/pull/2068

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.