Skip to content

Replace Klaxon with kotlinx-serialization #603

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Jun 14, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ plugins {
with(libs.plugins) {
alias(kotlin.jvm)
alias(publisher)
alias(serialization)
alias(serialization) apply false
alias(jupyter.api) apply false
alias(dokka)
alias(kover)
Expand Down Expand Up @@ -71,7 +71,7 @@ private fun String.findVersion(): Version {
// these names of outdated dependencies will not show up in the table output
val dependencyUpdateExclusions = listOf(
// 5.6 requires Java 11
libs.klaxon.get().name,
// libs.serialization.get().name,
// TODO Requires more work to be updated to 1.7.0+, https://github.com/Kotlin/dataframe/issues/594
libs.plugins.kover.get().pluginId,
// TODO Updating requires major changes all across the project, https://github.com/Kotlin/dataframe/issues/364
Expand Down
4 changes: 3 additions & 1 deletion core/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,9 @@ dependencies {
implementation(libs.kotlin.stdlib.jdk8)

api(libs.commonsCsv)
implementation(libs.klaxon)
implementation(libs.serialization.core)
implementation(libs.serialization.json)

implementation(libs.fuel)

api(libs.kotlin.datetimeJvm)
Expand Down
227 changes: 122 additions & 105 deletions core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/io/json.kt

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package org.jetbrains.kotlinx.dataframe.jupyter

import com.beust.klaxon.json
import kotlinx.serialization.ExperimentalSerializationApi
import kotlinx.serialization.json.*
import org.jetbrains.kotlinx.dataframe.api.rows
import org.jetbrains.kotlinx.dataframe.api.toDataFrame
import org.jetbrains.kotlinx.dataframe.io.DataFrameHtmlData
Expand Down Expand Up @@ -28,6 +29,7 @@ internal class JupyterHtmlRenderer(
val builder: JupyterIntegration.Builder,
)

@OptIn(ExperimentalSerializationApi::class)
internal inline fun <reified T : Any> JupyterHtmlRenderer.render(
noinline getFooter: (T) -> String,
crossinline modifyConfig: T.(DisplayConfiguration) -> DisplayConfiguration = { it },
Expand Down Expand Up @@ -60,14 +62,12 @@ internal inline fun <reified T : Any> JupyterHtmlRenderer.render(
val staticHtml = df.toStaticHtml(reifiedDisplayConfiguration, DefaultCellRenderer).toJupyterHtmlData()

if (notebook.kernelVersion >= KotlinKernelVersion.from(MIN_KERNEL_VERSION_FOR_NEW_TABLES_UI)!!) {
val jsonEncodedDf = json {
obj(
"nrow" to df.size.nrow,
"ncol" to df.size.ncol,
"columns" to df.columnNames(),
"kotlin_dataframe" to encodeFrame(df.rows().take(limit).toDataFrame()),
)
}.toJsonString()
val jsonEncodedDf = buildJsonObject {
put("nrow", df.size.nrow)
put("ncol", df.size.ncol)
putJsonArray("columns") { addAll(df.columnNames()) }
put("kotlin_dataframe", encodeFrame(df.rows().take(limit).toDataFrame()),)
}.toString()
notebook.renderAsIFrameAsNeeded(html, staticHtml, jsonEncodedDf)
} else {
notebook.renderHtmlAsIFrameIfNeeded(html)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -949,6 +949,7 @@ class JsonTests {
@Test
fun `nulls in columns should be encoded explicitly`() {
val df = dataFrameOf("a", "b")("1", null, "2", 12)
df.toJson(canonical = true) shouldContain "\"b\":null"
df.toJson() shouldContain "\"b\":null"
// df.toJson(canonical = true) shouldContain "\"b\":null"
}
}
Original file line number Diff line number Diff line change
@@ -1,14 +1,12 @@
package org.jetbrains.kotlinx.dataframe.jupyter

import com.beust.klaxon.JsonArray
import com.beust.klaxon.JsonObject
import com.beust.klaxon.Parser
import io.kotest.assertions.throwables.shouldNotThrow
import io.kotest.matchers.comparables.shouldBeGreaterThan
import io.kotest.matchers.comparables.shouldBeLessThan
import io.kotest.matchers.shouldBe
import io.kotest.matchers.string.shouldContain
import io.kotest.matchers.string.shouldNotContain
import kotlinx.serialization.json.*
import org.intellij.lang.annotations.Language
import org.jetbrains.kotlinx.jupyter.api.MimeTypedResult
import org.jetbrains.kotlinx.jupyter.testkit.JupyterReplTestCase
Expand Down Expand Up @@ -94,9 +92,9 @@ class RenderingTests : JupyterReplTestCase() {

assertDataFrameDimensions(json, 30, 1)

val rows = json.array<JsonArray<*>>("kotlin_dataframe")!!
rows.getObj(0).int("id") shouldBe 21
rows.getObj(rows.lastIndex).int("id") shouldBe 50
val rows = json["kotlin_dataframe"]!!.jsonArray
rows.getObj(0)["id"]?.jsonPrimitive?.int shouldBe 21
rows.getObj(rows.lastIndex)["id"]?.jsonPrimitive?.int shouldBe 50
}

/**
Expand All @@ -111,16 +109,15 @@ class RenderingTests : JupyterReplTestCase() {
}

private fun assertDataFrameDimensions(json: JsonObject, expectedRows: Int, expectedColumns: Int) {
json.int("nrow") shouldBe expectedRows
json.int("ncol") shouldBe expectedColumns
json["nrow"]?.jsonPrimitive?.int shouldBe expectedRows
json["ncol"]?.jsonPrimitive?.int shouldBe expectedColumns
}

private fun parseDataframeJson(result: MimeTypedResult): JsonObject {
val parser = Parser.default()
return parser.parse(StringBuilder(result["application/kotlindataframe+json"]!!)) as JsonObject
return Json.decodeFromString<JsonObject>(result["application/kotlindataframe+json"]!!)
}

private fun JsonArray<*>.getObj(index: Int) = this.get(index) as JsonObject
private fun JsonArray.getObj(index: Int) = this[index] as JsonObject

@Test
fun `test kotlin notebook plugin utils sort by one column asc`() {
Expand All @@ -138,10 +135,10 @@ class RenderingTests : JupyterReplTestCase() {

@Suppress("UNCHECKED_CAST")
private fun assertSortedById(json: JsonObject, desc: Boolean) {
val rows = json["kotlin_dataframe"] as JsonArray<JsonObject>
val rows = json["kotlin_dataframe"]!!.jsonArray as List<JsonObject>
var previousId = if (desc) 101 else 0
rows.forEach { row ->
val currentId = row.int("id")!!
rows.forEach { row: JsonObject ->
val currentId = row["id"]!!.jsonPrimitive.int
if (desc) currentId shouldBeLessThan previousId else currentId shouldBeGreaterThan previousId
previousId = currentId
}
Expand Down Expand Up @@ -177,25 +174,25 @@ class RenderingTests : JupyterReplTestCase() {

assertDataFrameDimensions(json, 100, 2)

val rows = json["kotlin_dataframe"] as JsonArray<JsonObject>
val rows = json["kotlin_dataframe"]!!.jsonArray as List<JsonObject>
assertSortedByCategory(rows)
assertSortedById(rows)
}

private fun assertSortedByCategory(rows: JsonArray<JsonObject>) {
private fun assertSortedByCategory(rows: List<JsonObject>) {
rows.forEachIndexed { i, row ->
val currentCategory = row.string("category")
val currentCategory = row["category"]!!.jsonPrimitive.content
if (i < 50) currentCategory shouldBe "odd"
else currentCategory shouldBe "even"
}
}

private fun assertSortedById(rows: JsonArray<JsonObject>) {
private fun assertSortedById(rows: List<JsonObject>) {
var previousCategory = "odd"
var previousId = 0
for (row in rows) {
val currentCategory = row.string("category")!!
val currentId = row.int("id")!!
val currentCategory = row["category"]!!.jsonPrimitive.content
val currentId = row["id"]!!.jsonPrimitive.int

if (previousCategory == "odd" && currentCategory == "even") {
previousId shouldBeGreaterThan currentId
Expand All @@ -220,9 +217,9 @@ class RenderingTests : JupyterReplTestCase() {

assertDataFrameDimensions(json, 2, 2)

val rows = json.array<JsonArray<*>>("kotlin_dataframe")!!
rows.getObj(0).array<JsonObject>("group1")!!.size shouldBe 50
rows.getObj(1).array<JsonObject>("group1")!!.size shouldBe 50
val rows = json["kotlin_dataframe"]!!.jsonArray
rows.getObj(0).get("group1")!!.jsonArray.size shouldBe 50
rows.getObj(1).get("group1")!!.jsonArray.size shouldBe 50
}

// Regression KTNB-424
Expand Down
7 changes: 5 additions & 2 deletions gradle/libs.versions.toml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ kover = "0.6.1"

commonsCsv = "1.10.0"
commonsCompress = "1.26.0"
klaxon = "5.5" # 5.6 requires Java 11
serialization = "1.6.2"
fuel = "2.3.1"
poi = "5.2.5"
mariadb = "3.3.2"
Expand Down Expand Up @@ -69,7 +69,10 @@ kotlin-reflect = { group = "org.jetbrains.kotlin", name = "kotlin-reflect", vers
kotlin-scriptingJvm = { group = "org.jetbrains.kotlin", name = "kotlin-scripting-jvm", version.ref = "kotlin" }
commonsCsv = { group = "org.apache.commons", name = "commons-csv", version.ref = "commonsCsv" }
commonsCompress = { group = "org.apache.commons", name = "commons-compress", version.ref = "commonsCompress" }
klaxon = { group = "com.beust", name = "klaxon", version.ref = "klaxon" }
# Serialization
serialization-core = { group = "org.jetbrains.kotlinx", name = "kotlinx-serialization-core", version.ref = "serialization" }
serialization-json = { group = "org.jetbrains.kotlinx", name = "kotlinx-serialization-json", version.ref = "serialization" }

fuel = { group = "com.github.kittinunf.fuel", name = "fuel", version.ref = "fuel" }
poi = { group = "org.apache.poi", name = "poi", version.ref = "poi" }
mariadb = { group = "org.mariadb.jdbc", name = "mariadb-java-client", version.ref = "mariadb" }
Expand Down
3 changes: 2 additions & 1 deletion plugins/dataframe-gradle-plugin/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ dependencies {

implementation(libs.kotlin.gradle.plugin.api)
implementation(libs.kotlin.gradle.plugin)
implementation(libs.klaxon)
implementation(libs.serialization.core)
implementation(libs.serialization.json)
implementation(libs.ksp.gradle)
implementation(libs.ksp.api)

Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
package org.jetbrains.dataframe.gradle

import com.beust.klaxon.KlaxonException
import io.kotest.assertions.asClue
import io.kotest.assertions.throwables.shouldNotThrowAny
import io.kotest.assertions.throwables.shouldThrow
import io.kotest.assertions.throwables.shouldThrowAny
import io.kotest.matchers.shouldBe
import kotlinx.serialization.SerializationException
import org.jetbrains.kotlinx.dataframe.DataFrame
import org.jetbrains.kotlinx.dataframe.io.read
import org.jetbrains.kotlinx.dataframe.io.readSqlTable
Expand Down Expand Up @@ -33,7 +33,7 @@ class DataFrameReadTest {
fun `file with invalid json`() {
val temp = Files.createTempDirectory("").toFile()
val invalidJson = File(temp, "test.json").also { it.writeText(".") }
shouldThrow<KlaxonException> {
shouldThrow<IllegalArgumentException> {
DataFrame.read(invalidJson)
}
}
Expand Down Expand Up @@ -74,7 +74,7 @@ class DataFrameReadTest {
@Test
fun `URL with invalid JSON`() {
useHostedJson("<invalid json>") { url ->
shouldThrow<KlaxonException> {
shouldThrow<SerializationException> {
DataFrame.read(url).also { println(it) }
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,16 +60,16 @@ class Write : TestBase() {
val jsonStr = df.toJson(prettyPrint = true)
// SampleEnd
jsonStr shouldStartWith """
[{
"name": {
"firstName": "Alice",
"lastName": "Cooper"
},
"age": 15,
"city": "London",
"weight": 54,
"isHappy": true
}
[
{
"name": {
"firstName": "Alice",
"lastName": "Cooper"
},
"age": 15,
"city": "London",
"weight": 54,
"isHappy": true
""".rejoinWithSystemLineSeparator()
}

Expand Down