You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@calcite.apache.org by Julian Hyde <jh...@gmail.com> on 2020/09/05 22:43:36 UTC
[DISCUSS] Fixing things that aren't broken
In the Apache Way, how do I say, ‘Please stop trying to fix this. It isn’t broken.’
Vladimir has been trying to fix things that aren’t broken over the last week. It has been driving me crazy. It has wasted a bunch of my time, answering his questions, pointing out that the behavior is intended. And still he goes ahead and changes stuff.
For example, https://issues.apache.org/jira/browse/CALCITE-4226 <https://issues.apache.org/jira/browse/CALCITE-4226>. It works fine. There are no bugs. Yet he still commits a change.
This change does not have consensus. The only person who weighed in (me) was against it. He did not leave time for anyone to review.
What should I do now - back out his change?
I fear that it is going to continue. https://issues.apache.org/jira/browse/CALCITE-4199 <https://issues.apache.org/jira/browse/CALCITE-4199>, the parent task of this whole initiative to add more nullability annotations, has a bunch of other sub-tasks. Early on, I tried to middle ground, but expressed skepticism about the whole endeavor: "I would support evolution and incremental improvements to our annotations. … But I don't want our code to look radically different in six months.” And yet he has forged ahead, logging countless other tasks that I think are useless. And the moment I look away, he will commit them.
I find Vladimir’s behavior utterly exhausting. It saps time that I could be using for better purposes in this project and in my opinion doesn’t improve the project one iota.
Julian
Re: [DISCUSS] Fixing things that aren't broken
Posted by Kenneth Knowles <ke...@apache.org>.
From the peanut gallery... I have been spending my "free" hours adding
checkerframework nullness typing to Beam and reliably finding bugs. It is
super valuable. It is not at all surprising that Vladimir has uncovered
bugs. The likelihood of a Calcite-sized project successfully defending
against nullability errors via any method other than via a sound type
system is zero. The only way to eliminate NPEs from Calcite/Beam/etc is by
adding such a type system.
When it does not uncover bugs, it often makes code more readable. Some
static analyses cannot grok good code well enough to accept it. That is not
the case for checkerframework in most cases, in my experience. They have
covered the common OK cases like "this Set contains the keys for that Map"
or "this can only be null if that other thing is null". Since it is a type
system, it is trivial to understand, doubles as good documentation, and has
no weird behaviors like spotbugs and other heuristic approaches.
It is a valid point that any place a user can pass things in they might
violate the null-safe type system. This is not a new problem, but a subset
of your current problem. FWIW it would make my life in Beam better if
Calcite's public APIs had accurate nullness types.
I can see that there is more to this discussion than just "should we get
Calcite to use a nullness type checker" but if you could limit scope to
that question, I would emphatically say "yes, please".
Kenn
p.s. <rant>The unchecked assumption that anything is nullable by default is
an obsolete concept of yesteryear, and a solved problem. The cost of the
"billion dollar mistake" was a massive underestimate. For a time, nullness
safety was confined to "advanced" languages and was not available to those
working in more widespread industrial languages. With Java's pluggable type
systems and the checkerframework it is finally available to "everyone".
There is no excuse (other than lack of time to do the work) remaining to
ever have an NPE under any circumstances. It is more justifiable to have
SEGV-vulnerable code (for perf reasons) than NPE-vulnerable code,
IMO.</rant>
On Sat, Sep 5, 2020 at 4:45 PM Vladimir Sitnikov <
sitnikov.vladimir@gmail.com> wrote:
> 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
>
Re: [DISCUSS] Fixing things that aren't broken
Posted by Vladimir Sitnikov <si...@gmail.com>.
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
Re: [DISCUSS] Fixing things that aren't broken
Posted by Julian Hyde <jh...@gmail.com>.
And let’s add https://issues.apache.org/jira/browse/CALCITE-4217 <https://issues.apache.org/jira/browse/CALCITE-4217> to the list of changes that Vladimir has committed despite my clear objections.
> On Sep 5, 2020, at 3:43 PM, Julian Hyde <jh...@gmail.com> wrote:
>
> In the Apache Way, how do I say, ‘Please stop trying to fix this. It isn’t broken.’
>
> Vladimir has been trying to fix things that aren’t broken over the last week. It has been driving me crazy. It has wasted a bunch of my time, answering his questions, pointing out that the behavior is intended. And still he goes ahead and changes stuff.
>
> For example, https://issues.apache.org/jira/browse/CALCITE-4226 <https://issues.apache.org/jira/browse/CALCITE-4226>. It works fine. There are no bugs. Yet he still commits a change.
>
> This change does not have consensus. The only person who weighed in (me) was against it. He did not leave time for anyone to review.
>
> What should I do now - back out his change?
>
> I fear that it is going to continue. https://issues.apache.org/jira/browse/CALCITE-4199 <https://issues.apache.org/jira/browse/CALCITE-4199>, the parent task of this whole initiative to add more nullability annotations, has a bunch of other sub-tasks. Early on, I tried to middle ground, but expressed skepticism about the whole endeavor: "I would support evolution and incremental improvements to our annotations. … But I don't want our code to look radically different in six months.” And yet he has forged ahead, logging countless other tasks that I think are useless. And the moment I look away, he will commit them.
>
> I find Vladimir’s behavior utterly exhausting. It saps time that I could be using for better purposes in this project and in my opinion doesn’t improve the project one iota.
>
> Julian
>