Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Arrow writing #169

Merged
merged 21 commits into from
Dec 2, 2022
Merged

Arrow writing #169

merged 21 commits into from
Dec 2, 2022

Conversation

Kopilov
Copy link
Contributor

@Kopilov Kopilov commented Sep 17, 2022

#162 duplicate

@Kopilov Kopilov mentioned this pull request Sep 17, 2022
Merged
@Kopilov
Copy link
Contributor Author

Kopilov commented Sep 20, 2022

Hello @koperagen,

right now i seem to lack general understanding, so i'll review it again a bit later.

You cal look similar module in krangl. And ask me about anything if you want.

What do you think about making possible warnings a sealed class with overloaded toString for default behaviour? So that you can access actual exception in conversion or DataColumn and KType, etc.

Sorry, I still do not have enough time to elaborate this. Working on another part of the customer's project currently…

@Kopilov
Copy link
Contributor Author

Kopilov commented Nov 16, 2022

possible warnings a sealed class

Started working on this

@Kopilov Kopilov mentioned this pull request Nov 22, 2022
Merged
@Jolanrensen Jolanrensen added this to the 0.9.0 milestone Nov 28, 2022
@Jolanrensen Jolanrensen added the enhancement New feature or request label Nov 28, 2022
@Kopilov
Copy link
Contributor Author

Kopilov commented Nov 29, 2022

@koperagen @Jolanrensen @nikitinas
Hi,
this branch is upgraded. Evaluate please :)

@Jolanrensen Jolanrensen self-requested a review November 30, 2022 14:25
Jolanrensen
Jolanrensen requested changes Nov 30, 2022
View reviewed changes
docs/StardustDocs/topics/write.md Outdated Show resolved Hide resolved
docs/StardustDocs/topics/write.md Outdated Show resolved Hide resolved
docs/StardustDocs/topics/write.md Outdated Show resolved Hide resolved
docs/StardustDocs/topics/write.md Outdated Show resolved Hide resolved
core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/convert.kt Show resolved Hide resolved
dataframe-arrow/src/main/kotlin/org/jetbrains/kotlinx/dataframe/io/arrowReading.kt Show resolved Hide resolved
dataframe-arrow/src/main/kotlin/org/jetbrains/kotlinx/dataframe/io/ArrowWriter.kt Outdated Show resolved Hide resolved
dataframe-arrow/src/main/kotlin/org/jetbrains/kotlinx/dataframe/io/ArrowWriterImpl.kt Outdated Show resolved Hide resolved
dataframe-arrow/src/main/kotlin/org/jetbrains/kotlinx/dataframe/io/ArrowWriterImpl.kt Outdated Show resolved Hide resolved
dataframe-arrow/src/main/kotlin/org/jetbrains/kotlinx/dataframe/io/ArrowWriterImpl.kt Outdated Show resolved Hide resolved
@Jolanrensen
Copy link
Collaborator

@Kopilov updated the comments with my thoughts :) the rest looks good!

@Jolanrensen Jolanrensen merged commit 3583c91 into Kotlin:master Dec 2, 2022
@Kopilov Kopilov deleted the arrow branch December 2, 2022 14:37
@Jolanrensen
Copy link
Collaborator

I'm gonna change ArrowWriter.Companion.Mode -> ArrowWriter.Mode. It is less typing, cleaner, and Companion is always avoided in Kotlin unless explicitly needed.

@Kopilov
Copy link
Contributor Author

Kopilov commented Dec 19, 2022

Hello @Jolanrensen,
OK, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jolanrensen Jolanrensen Jolanrensen approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
0.9.1
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Kopilov @Jolanrensen