Some of the Prs show improvements of 1-3%. These I expect are in the measurement noise. In general if you make measurements you have to show some variance bars. I would start with the changes that show a clear improvement.
Mihai ________________________________ From: Frank Chen <[email protected]> Sent: Wednesday, September 23, 2026 8:16 AM To: [email protected] <[email protected]> Cc: [email protected] <[email protected]>; [email protected] <[email protected]> Subject: Re: Large IN predicate performance work and Calcite PRs Hi Vladimir, Thanks for the concrete suggestions. I agree that no PRs should be merged without sufficient human review. At the same time, controlling what is merged is part of the project’s review and merge process. As I decide to push this IN filter performance problem to be fully addressed and touch the code in the Calcite, my responsibility is to make each proposal independently reviewable, provide sufficient tests, share the improvement with data and evidence, and address technical feedback. Your concrete suggestions will be adopted and applied accordingly, The existence of several open proposals should not itself create a risk of unreviewed changes being merged. Thanks and regards. On Wed, Sep 23, 2026 at 1:40 PM Vladimir Sitnikov < [email protected]> wrote: > 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] > >
