Frank, thanks for raising this. I'm with Julian regarding "I am afraid someone could merge the changes unreviewed".
"10 PRs created per minute" does not bother me, and I do not think we should block contributions based on the rate alone. I can easily understand if Frank spent noticeable time trying various approaches to fix Druid/Calcite, and then created all findings at once. It would be the right approach for performance-related changes. I did check a few PRs. >From my point of view the changes themselves look reasonable, except two >things: 1) The tests are either missing or incomplete. I expect the PR to cover its changes (functional, performance). It is good you have a Druid-level benchmark, however, we need a test in Calcite. Otherwise the next innocent change in Calcite could introduce the same regression. 2) Commit messages seem to be empty. The commit message is empty, and the PR description is like a story to tell. This creates a burden on a committer that would have to come up with a commit message at the merge time. With LLMs, I suggest we invest more time into testing things. I do not suggest creating more tests for the sake of tests. I mean use old-school test design techniques like equivalence partitioning, boundary value analysis, and so on. Let us take an example: https://github.com/apache/calcite/pull/5281 The change adds a fast path for "same collation and charset". Nice catch. It properly uses "==" for collation, however, it does not clarify the reason .equals is wrong there. The PR adds no tests to distinguish collation== vs collation.equals in the newly added code. Even though the PR code is valid, it might easily become invalid should someone "use .equals instead of identity comparison". Calcite does have junit for functional tests, and jmh for performance tests. I expect contributions to update those. PS. Frank, I suggest you add the following to your ~/.../AGENTS.md,CLAUDE.md: "When preparing text for GitHub or JIRA (PR or issue description, comment, discussion), don't hard-wrap — keep each paragraph and list item on one line; GitHub, Gitlab, JIRA re-flow it" PPS. I'm not fond of creating JIRA for every small change. Frank's umbrella JIRA approach looked good enough to me. If the community wants "jira for every individual PR" I suggest we add that to AGENTS.md (that should better be discussed separately) Vladimir --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
