migration from kotlinx.datetime.Instant -> kotlin.time.Instant

[Jolanrensen]

Fixes #1350

• Uses 0.7.1-0.6.x-compat to ease the transition to the new version. Old Instants are still supported in conversions, but not the default anymore.
• Adds support for our notebook tests to run with --opt-in and other compiler options :)

[koperagen]

Check these functions too please:

public fun DataColumn<String?>.convertToInstant(): DataColumn<Instant?> = map { it?.let { Instant.parse(it) } }
public fun <T> Convert<T, String?>.toInstant(): DataFrame<T> = asColumn { it.convertToInstant() }
public fun <T> Convert<T, String>.toInstant(): DataFrame<T> = asColumn { it.convertToInstant() }

What should we do about them?

For transparency, i suppose these things change:

1. Changes in convertTo and to - it shouldn't break anything that was previously working, new code that explicitly uses kotlin.time.Instant will benefit from it
2. parse - in this case things can break, right?
   a) DataFrame.readCsv/parse produces kotlin.time.Instant => compilation fails, person needs to add compiler opt in. This is concerning and affects new users not in a good way, especially in notebooks =(
   b) Old code refers to kotlinx.datetime.Instant from casts/String API/generated accessors, but value in runtime is now kotlin.time.Instant.
3. Our library now exposes newer kotlinx.datetime with deprecated Instant

[Jolanrensen]

3. Our library now exposes newer kotlinx.datetime with deprecated Instant

Nope :) I made it api(version 0.7.1), only implement(version 0.7.1-0.6.x-compat), so we don't expose the deprecated Instant, we only recognize it when a user provides it.

Check these functions too please:

public fun DataColumn<String?>.convertToInstant(): DataColumn<Instant?> = map { it?.let { Instant.parse(it) } }
public fun Convert<T, String?>.toInstant(): DataFrame = asColumn { it.convertToInstant() }
public fun Convert<T, String>.toInstant(): DataFrame = asColumn { it.convertToInstant() }
What should we do about them?

These I replaced with kotlin.time.Instant.
If we want to avoid breakage, the proper way would be to deprecate the original function, introduce convertToStdlibInstant(), remove the original, deprecate convertToStdlibInstant() and introduce a new convertToInstant(). Do you think that would be the best option? (also taking the compiler plugin into account).
This would require us to use api(version 0.7.1-0.6.x-compat), because our functions could return a deprecated kotlinx.datetime.Instant.

1. Changes in convertTo and to - it shouldn't break anything that was previously working, new code that explicitly uses kotlin.time.Instant will benefit from it

Yep :) should be fine

2. parse - in this case things can break, right?
   a) DataFrame.readCsv/parse produces kotlin.time.Instant => compilation fails, person needs to add compiler opt in. This is concerning and affects new users not in a good way, especially in notebooks =(

Mmm yes you're right. I think the --opt-in is fine. Kotlinx DateTime is always considered experimental, so requiring OptIn can be expected. If you try this in notebooks, you will get an exception that you need "--opt-in". I think it's up to the notebook team to provide a small guide for how to do that. It will likely come up moreoften, in other libraries too.

b) Old code refers to kotlinx.datetime.Instant from casts/String API/generated accessors, but value in runtime is now kotlin.time.Instant.

yes... that's true. On the other hand, we need to make the switch at some point right? It could be possible to also do this in a cycle; similar to ParserOptions.parseExperimentalUuid, we could introduce ParserOptions.parseExperimentalInstant, default it to false, then default it to true, then remove it. However, this would require us to use api(version 0.7.1-0.6.x-compat), because our functions could return a deprecated kotlinx.datetime.Instant.

[koperagen]

These I replaced with kotlin.time.Instant.

Got it, didn't notice import changed.

Nope :) I made it api(version 0.7.1), only implement(version 0.7.1-0.6.x-compat)

Ah ok, so we expose api without kotlinx.datetime.Instant? Like, if someone used to have only dataframe dependency and import it, now it becomes red code? I'm asking so we can compile a little list of "what changed"

Mmm yes you're right. I think the --opt-in is fine.

Normally people explicitly use experimental api in their code and see the warning/error. DataFrame produces Instant as a result of non-experimental operation and then codegeneration fails with exception. I think this is what makes difference =(

[Jolanrensen]

Normally people explicitly use experimental api in their code and see the warning/error. DataFrame produces Instant as a result of non-experimental operation and then codegeneration fails with exception. I think this is what makes difference =(

Well yes :/ unfortunately kotlinx.datetime pushes us in that direction. They deprecated the old Instant before the new one is stable... But you're right, we say our API is stable by declaring a beta, so we should probably honor that and go with a safe route, which means:

• using api(version 0.7.1-0.6.x-compat), which exposes kotlinx.datetime.Instant as deprecated, encouraging users to switch to kotlin.time.Instant
• full support of the deprecated and experimental Instant in convertTo<>{}, and convert {}.to<>()
• introduce convertToStdlibInstant(): kotlin.time.Instant, deprecate convertToInstant(): kotlinx.datetime.Instant as warning
• introduce ParserOptions.parseExperimentalInstant = false and let it parse to the deprecated instant by default for the current release (for CSV/Arrow as well)

would this approach be better? :)

[AndreiKingsley]

I don't like convertToStdlibInstant(). Looks weird

[Jolanrensen]

I don't like convertToStdlibInstant(). Looks weird

@AndreiKingsley
Kotlinx datetime itself uses .toStdlibInstant() and .toDeprecatedInstant(), best to follow it:
https://github.com/Kotlin/kotlinx-datetime?tab=readme-ov-file#deprecation-of-instant

[AndreiKingsley]

Ok, just a bit of a strange transition scheme (on their part).

[Jolanrensen]

Ok, just a bit of a strange transition scheme (on their part).

That's true, but then again, how often does it happen you need to deprecate something because it's added to the standard library? Compatibility nightmare XD Better to be explicit about it. Hopefully the path will be easier whenever LocalDate(Time) gets added to the stdlib at some point :)

[Jolanrensen]

Alright I did:

• using api(version 0.7.1-0.6.x-compat), which exposes kotlinx.datetime.Instant as deprecated, encouraging users to switch to kotlin.time.Instant
• full support of the deprecated and experimental Instant in convertTo<>{}, and convert {}.to<>()
• introduce convertToStdlibInstant(): kotlin.time.Instant, deprecate convertToInstant(): kotlinx.datetime.Instant as warning
• introduce ParserOptions.parseExperimentalInstant = false and let it parse to the deprecated instant by default for the current release (for CSV/Arrow as well)

Specifically I made the convertToInstant()-like overloads ERROR, pointing users to convertToDeprecatedInstant().
convertToDeprecatedInstant() I made WARNING, same level as kotlinx.datetime.Instant, pointing users to kotlin.time.Instant, but more gently ;P. This allows us to change the behavior of convertToInstant() in DF 1.1 already :)

I also added some helper-conversions from the deprecated instant to stdlib instant to ease the transition a bit.

A similar thing was done for the ColTypes, used by readCsv(), and the docs were updated.

[Jolanrensen]

convert {}.toStdlibInstant() (both overloads) and convert {}.toDeprecatedInstant() seem to work fine with the compiler plugin as well, tested with nightly and Kotlin 2.2.20-Beta2

[zaleslaw]

better to have a full reference to issue

[Jolanrensen]

Issue numbers are clickable now, but I'll add some text as well