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.

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

[Compiler plugin] Setup call transformer pipeline to handle (...) -> DataRow functions #918

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

koperagen
Copy link
Collaborator

@koperagen koperagen commented Oct 11, 2024

This open up possibility to support DataFrame.aggregate -> DataRow and other IO functions. Implemented DataRow.readJsonStr as an example

@koperagen koperagen added the Compiler plugin Anything related to the DataFrame Compiler Plugin label Oct 11, 2024
Copy link
Contributor

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

Copy link
Collaborator

@Jolanrensen Jolanrensen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool :)


fun box(): String {
val row = DataRow.readJsonStr(text)
row.a
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be so awesome for an ordinary Map :o I wonder if we could have a plugin which just does this for Maps. It's like js/python but safe :P

@@ -156,14 +156,14 @@ class FunctionCallTransformer(
?: call
}

inner class DataFrameCallTransformer : CallTransformer {
inner class DataSchemaLikeCallTransformer(val classId: ClassId) : CallTransformer {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you really gotta start with little KDocs here and there. In the last PRs you often make classes gain variables so they can be used for multiple cases based on the argument given. However without docs this becomes harder and harder to understand the more you add (unless you wrote the code, but even then, do you still understand it in a year or 2?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that code is becoming harder to understand should be addressed, but i have my own understanding on what can be explained in kdoc text and what just shouldn't be
I don't like how it seems that i'm the one who - somehow - resists writing it, and you need to push me. So i suggest, instead of general feedback like "write kdocs", what i perceive as "write better code", just say what specifically you don't like here and i'll try to answer

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the main issue is that module is so complex (aside from the impl/api part, that's nice) that I cannot properly review it because I don't know where to begin to understand it, let alone help you catch bugs. And believe me when I say I want to help, because the compiler plugin is awesome :). While "code should explain itself" is a nice standpoint, that can only hold for small pieces of code, otherwise the cognitive complexity is too large. And for me it is too large, on multiple levels.

I'll give some examples. Not to bash or anything, but to be constructive and show where I have trouble understanding what's going on:

  • On the function scope level:
    • Some functions are large, like 50+ lines and sometimes contain up to 8 nested scopes, while I cannot immediately see from the name and arguments how and where it's used or what it does.
    • For instance FunctionCallTransformer.buildScopeFunctionCall, KotlinTypeFacade.interpret
    • This could be solved by splitting a function up into multiple smaller ones with good names, avoiding let/apply unless it's absolutely needed, avoiding inner functions unless needed, etc. And yes, kdoc which in a couple of sentences describes what the function does so I can just read that instead of look through the complex function.
  • On the class/file level:
    • Many files contain many functions/classes/interfaces, which are probably related, but that's not immediately clear. Plus when I'm looking for a certain class, it's sometimes in a completely unexpected file.
    • For instance DefaultValue inside Interpreter.kt, PluginDataFrameSchema inside SimpleCol.kt, KType.from(): String inside toPluginDataFrameSchema.kt (btw what does KType.from() do? it returns fqName but also throws errors? I don't understand it haha)
    • This can be solved by splitting things into multiple files. It might also make it clearer if we follow our convention where files containing multiple functions/classes are named with a lowercase letter and ones that contain just one class/interface (and maybe some strictly related helper functions) are named after that class/interface. That makes it easier to know what to expect just from the file name.
  • On the file/structure level:
    • I kinda miss a "Where to start looking" guide. This could be a README for instance, describing the structure of the module. Just pointing out where the part is that hooks into the compiler, where the part is that converts API calls, what it does etc. A little reference so I know which collection of files does what and I know where to look to fix something.
    • This can partly be solved by improving package names to be more specific (in contrast to "extensions", "impl") but probably not entirely.

The rest of the DF library also contains some of these cases, especially in the structure part and large nesting/difficult to understand functions. Proof of this is @zaleslaw just discovering what a certain module does after reading my README haha. So hopefully we can improve there too over time :).

Again, I don't want to make you feel like you did a bad job or anything, because you didn't :) You're doing an awesome job! And yes, we're still in the experimental phase. But let's say you suddenly cannot work on the plugin anymore, then someone else needs to be able to pick it up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Compiler plugin Anything related to the DataFrame Compiler Plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants