Hey Julian, I can imagine it might be unexpected (and/or fearful) to see that many issues / changes out of a blue. I do not expect that you rush into review right away (or even spent your time on that!).
However, I do value your feedback, and it is indeed helpful. No jokes, it was helpful, and I was surprised for having feedback that early. Let me add some clarifications: 1) I create tickets under a single parent, so it helps to keep them together and/or close in a bulk. 2) Sometimes I find "interesting" code (e.g. dead code?), and I create a ticket just in case to manage the workqueue 3) Many tickets might even be closed later for any reason (e.g. https://issues.apache.org/jira/browse/CALCITE-4231 ). I won't complain like "I want to make a code change for the ticket". For instance, if 4231 is dead code, and we agree to keep it, ok. If we agree to deprecate&remove, that is fine as well. 4) I do my best to incorporate feedback, and I believe I made the changes in 4226 and 4217 in line with your feedback. 5) There are cases where I need more time to settle on the solution and figure out the way to proceed (e.g. statistics, or even 4214 RelDataType#getFamily). Even though draft PRs are there, I'm not sure they are all good, so I do not commit them yet. >And the moment I look away, he will commit them It is really sad to hear that. In any case, even if I type the wrong command and push something unexpected, the option to revert / force-push is always there. There's no way I push a change like "add nullability annotations" without a discussion. Note: the change is not ready yet, and I believe there's nothing to discuss yet. We don't know the amount of the changes, we don't know how it looks, and so on. Of course, early feedback is always welcome. ---- Julian>in my opinion doesn’t improve the project one iota. It is sad to hear that as well. "[CALCITE-4228] FlatLists.Flat6List#append" is a true bug. There's discussion pending on "statistics nullability". I hope we could figure out the way to make it into a better API for the users and make the core stronger. Here's a PR that shows Calcite core fails with NPEs if only statistic callbacks respond with nulls (which is explicitly allowed by the javadocs in core, and it was you who insist that "unknowns" should be returned as null): https://github.com/apache/calcite/pull/2136/files Traits / TraitSet are not null-ready, so we might need to discuss if nulls should be allowed there at all. The above alone does sound like a non-trivial improvement to me. Julian>CALCITE-4226>. It works fine. There are no bugs. Yet he still commits a change. Here's a test: @Test void npeTest() { RelBuilder.create(config().build()) .scan("EMP") .fields(Mappings.target(Arrays.asList(2, 4), 10)); } The current Calcite fails as follows: java.lang.NullPointerException at org.apache.calcite.tools.RelBuilder.fields(RelBuilder.java:544) at org.apache.calcite.tools.RelBuilder.fields(RelBuilder.java:566) at org.apache.calcite.test.RelBuilderTest.npeTest(RelBuilderTest.java:228) It does look like a bug to me. I believe RelBuilder#fields should be user-friendly public API, so it should not just throw NPE. At least it should clarify what went wrong. That is why I added a an extra function for Mappers that would make the message easier to understand. --- Julian> CALCITE-4217> to the list of changes that Vladimir has committed despite my clear objections. You asked to keep "crosstype is not struct" behavior. I believe I did exactly that. However, it is clear you still have objections :( Could you please clarify what are they? I truly do not understand what is wrong. The bug was "getFieldCount was not working", I fixed that, and I kept everything else as is. The change in 4217 was to allow clients to call getFieldCount() Note: the clients could use getFieldList().size() as a workaround, however, it does look awkward that a trivial getFieldCount() (which is in RelDataType interface) fails to work. Of course, users can figure out the workaround, but why should it be broken like that? Vladimir
