Enable plugin evaluation of String API overloads - #2057
Conversation
…ith interpreter metadata
There was a problem hiding this comment.
Really like where this is heading — two things to sort out before merge, then this is good to go.
Produced by AIR Automations. Name: AI Review / Run: https://air.jetbrains.cloud/org/05cf1a7f-6ab5-713b-abd3-29d0c8a05e2d/automations/8daf2412-0b6c-4433-90c0-b40ab5aebcdb?run=72cea30b-966a-4e2e-8d35-928efdc25920
| import org.jetbrains.kotlinx.dataframe.annotations.HasSchema | ||
| import org.jetbrains.kotlinx.dataframe.annotations.Interpretable | ||
| import org.jetbrains.kotlinx.dataframe.annotations.Refine | ||
| import org.jetbrains.kotlinx.dataframe.annotations.StringApiInterpretable |
There was a problem hiding this comment.
Blocking: unused import.
Problem: StringApiInterpretable is imported here and in parse.kt:9, but neither file ever uses it — consistent with lint.kt, which lists Convert0 and Parse as "own interpreter". These are leftovers from the annotation sweep.
Failure scenario: standard:no-unused-imports is active (ktlint_code_style = ktlint_official, and the rule is not among the ktlint_standard_* opt-outs in .editorconfig; core/src/main is not in the ktlint = disabled glob). ./gradlew build fails :core:ktlintMainSourceSetCheck on both files, so CI goes red.
Suggested fix: delete the import line here and at parse.kt:9.
There was a problem hiding this comment.
Thanks for this — requesting changes, since there are blocking findings inline to address before merge.
Produced by AIR Automations. Name: AI Review / Run: https://air.jetbrains.cloud/org/05cf1a7f-6ab5-713b-abd3-29d0c8a05e2d/automations/8daf2412-0b6c-4433-90c0-b40ab5aebcdb?run=4ced62f0-a944-4704-baf8-2aada6bacc4e
| * @return A new [GroupBy] containing the unique combinations of values from the provided [key columns][cols], | ||
| * together with their corresponding groups of rows. | ||
| */ | ||
| @Refine |
There was a problem hiding this comment.
Blocking: bare @Refine is added here and on ~30 other String overloads whose only interpreter metadata is @StringApiInterpretable — an annotation only the 2.4.20+ plugin understands. Compiler-Plugin.md documents 2.2.20 as the minimum supported version, and this is the first @Refine in the tree without a co-located @Interpretable.
Failure scenario: a user on Kotlin 2.2.20–2.4.0 upgrades DataFrame. FunctionCallTransformer.intercept gates only on the Refine annotation, so the call is intercepted and its return type is replaced by a generated token; analyzeRefinedCallShape then finds no @Interpretable, falls back to PluginDataFrameSchema.EMPTY, and df.groupBy("key").toDataFrame().key no longer resolves — code that compiles today stops compiling.
Fix: don't emit bare @Refine; have the new plugin trigger refinement from @StringApiInterpretable itself (or from a distinctly named annotation older plugins ignore) so pre-2.4.20 behaviour is unchanged. If you'd rather keep @Refine, raise the documented minimum plugin version and call the break out in the release notes.
| */ | ||
| @Refine | ||
| @Interpretable("SelectString") | ||
| @StringApiInterpretable(interpreter = "Select0", stringArgument = "columns", targetArgument = "columns") |
There was a problem hiding this comment.
Blocking: this overload already has a purpose-built @Interpretable("SelectString"). It is the only declaration in the tree carrying both annotations, and precedence between them isn't defined anywhere.
Failure scenario: if the plugin reads @StringApiInterpretable first, the most-used String overload silently switches from SelectString to the generic Select0 delegation and SelectString becomes dead code. Neither new test suite can catch it, because both paths are asserted to agree.
Fix: remove @StringApiInterpretable here and add Select0 to the "own interpreter" ignore list in lint.kt, next to Parse and Convert0.
This PR enables plugin interpretation for String overloads that have CS DSL counterparts, see
Plugin.kt:
RawSchemaOperationsResultCorrectnessTest:
User scenario when df has no schema, some string operations are invoked, and as a result we enrich schema
TypedSchemaOperationsResultConsistencyTest:
User scenario when dataframe itself is typed, but user chose to use String overload for simplicity => we do not loose information
I had to update kotlin to 2.4.20-RC2 so that compiler plugin understands StringApiInterpretable, to have these tests in this repo, i think it's better for visibility in this case
I plan to update this PR: clean up lint.kt and add annotations to a few remaining functions.
Check the overall idea, clarity of tests, etc.