To Mihai, As I said before, CALCITE-7782 has already addressed the main problem(which makes the query unable to produce a result), what we are doing now is try to push it further. I want to figure out that when looking at the percentage number, sometimes we also need to look at the base number. When looking at the saved memory allocation size each change brings, it's not small, and the saving is stable in contrast to latency, and considering some changes are small, I think it's also worth trying to fix. And If we stack these changes together, from the JFR, the 100_000 and 1_000_000 case both show another 70% allocation reduction. This is the core motivation that I submit the changes.
Had there been only one change that brought about very small improvement, I would never create a JIRA issue and then create a PR which took my time and your time of reviewing. To Julian, In the original issue, the description is already highly summarized like the way you propose: > CALCITE-7782 and PR #5263 eliminated quadratic operand-list copying while validating large ARRAY and MAP constructors. That correction allows a 1,000,000-element string ARRAY to complete instead of running for more than 20 minutes. > After that correction, validation is linear, but planning still creates many temporary objects for every string literal. In the benchmark below, planning a 1,000,000-element string IN expression allocates approximately 15.4 GB during one operation and takes approximately 7.8 seconds. and for each sub issue, the issue description is also a short description to answer a 'why' question. I can try to make the description more concise and better. You are complaining about the long PR description, which currently explains what and how. The 'why' question is indeed not directly in the PR description as the first part, but it has a link to the JIRA ticket which contains a 'why' question. This is a following of current Calcite PR template: https://github.com/apache/calcite/blob/main/.github/pull_request_template.md For the 'Change Proposed' part of a PR, generally I always follow a practice to explain 'what' and 'how'. What I can do is that if the explanation is a bit long, I can fold some text by default. And about this one: > These PRs are very focused on fixing one particular Druid issue. They claim to improve performance, but they cite a benchmark based on just one Druid query I think I have done my responsibility to provide fact/evidence based feedback and ways to improve, and changes I think are good enough within my understanding of Calcite. You can't expect everyone who contributes a PR has a full picture of all details in the Calcite so that the change is broad and perfect enough and all things are under consideration. As changes are submitted, it's up to the maintainers to decide if they have another impact. Maintainers can give concrete feedback on what contributors can do next, asking for more data/change/test, and then contributors can continue the work. Thanks and regards. On Thu, Sep 24, 2026 at 3:46 AM Mihai Budiu <[email protected]> wrote: > 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] > > > > >
