Julian, I hear the concern about reviewer time. Five PRs at once and long descriptions made prioritization harder; I’ve shortened them. Let me address the open comments on #5281 left by Vladimir first, then bring the remaining PRs forward in priority order.
The Druid results establish a benefit for that workload; representative Calcite evidence and specific regression concerns will help test or narrow the broader changes. Contributors also invest unpaid time, so concise proposals and actionable feedback respect both sides. Thanks and regards. On Fri, Sep 25, 2026 at 12:02 AM Julian Hyde <[email protected]> wrote: > Frank, > > Please stop being offended by a particular word, and listen to what I > am trying to say. > > Human time and attention is limited. Calcite committers, including me, > are unpaid volunteers. > > It took me an hour yesterday to write that email to you. You rebutted > it, point by point, with an equally long email. I am not going to > respond further. > > I am trying to say that a good (human) engineer can make their case in > one paragraph, and you submitted that one paragraph plus a few dozen > more. > > Less is more. > > Julian > > On Thu, Sep 24, 2026 at 12:28 AM Frank Chen <[email protected]> wrote: > > > > > > I also want to clearly point out why the word “slop” concerns me. > > In my cultural and linguistic context, applying that word to a person’s > contributions is received not merely as criticism of the material, > > but as a demeaning judgment about the contributor and the quality of > their work. > > It made me feel closer to an ad hominem dismissal than technical > feedback. > > I understand that this may not have been the intended meaning, but that > was its effect on me. > > > > I accept the concrete criticism concerning review burden. However, I > would appreciate future criticism being expressed through specific > technical or process concerns rather than labels such as “slop.” > > That would allow us to disagree constructively while keeping the > discussion focused on the merits of each change. > > > > Since this work has been characterized as “a moderate performance > improvement”—“not trivial, worth having,” with the patches “worth further > investigation", > > let me push these changes forward and have each one evaluated on its > technical merits. > > > > 1. I updated the PR and JIRA descriptions to present the why, what, and > how more clearly and concisely. Please let me know whether the revised > descriptions address the concerns about verbosity and reviewability. > > 2. I'm going to address Vladimir’s concrete feedback, including: > > a. strengthening Calcite-level functional tests; > > b. adding Calcite-level performance measurements where appropriate; > > c. applying other suggestions > > If there are additional concrete technical concerns, please leave them > on the relevant PR. I will evaluate and address them accordingly. > > > > Thanks and regards. > > > > On Thu, Sep 24, 2026 at 11:32 AM Frank Chen <[email protected]> > wrote: > >> > >> > >> 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] > >>> > > >>> > >
