paulk-asert commented on PR #3155: URL: https://github.com/apache/groovy/pull/3155#issuecomment-6601636412
I've been through this with 7.0.0-alpha-1 in mind and I think it can go in as is, as an incubating optional module, with the smaller items handled in later alphas. Checked: `:groovy-ginq:test` (665) and `:groovy-ginq-sql:test` (207, on the #3167 tip which includes this) are green locally; the Copilot findings and Jochen's two comments are addressed at the tip; jOOQ stays `compileOnly`, H2 test-only, nothing new is bundled. The additive `groovy-ginq` changes (`provider`/`dataSource`, `GinqProvider`, the two `GinqAstVisitor` defaults) are all under the incubating provider extension point, which is the right place for them. CI: the macOS red is the known 60-minute stall. Please rebase onto master before merging: the branch is ~40 commits behind and lacks 71552661eb (the cache-scrub fix), which is exactly what failed on Windows for #3167 after a successful Gradle run. For later alphas (none blocking): - `MixedPlan` and `Untranslatable` are the only public types without `@Incubating`. - A note in the guide that SQL result types can differ from in-memory ones (e.g. `avg` is `double` from H2, `BigDecimal` in memory). - GEP-26 should be updated to say mixed mode is implemented. While comparing providers I also hit two pre-existing collection-provider bugs (`denseRank() over(orderby e.prop)` without `partitionby` NPEs, and a `range` frame NPEs on a null key); I'll raise those separately, they're unrelated to this PR. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
