Re: Calcite contributions

[Julian Hyde]

It’s never going to be black and white. We’re never going to have a code of 
conduct that covers all cases. (But please, everyone go read the CoC if you 
have not read it recently: 
https://www.apache.org/foundation/policies/conduct.html 
.)

But we all know civility when we see it.

In this case, Vladimir was a bit hasty in committing a fix, and in so doing 
trod on Zoltan’s toes. Zoltan was a bit upset, and started this thread to voice 
his feelings.

If I literally stepped on someone’s toes while walking along the street, I’d 
say “Sorry, dude! I didn’t mean to tread on your toes!” and that would probably 
be the end of the matter. We don’t need to start a debate about whether Zoltan 
was walking on the right part of the sidewalk. I think an apology is often all 
that is needed.

Also, everyone should assume good faith in everyone else. Vladimir isn’t trying 
to diminish Zoltan, he just wants to make the product better. Zoltan isn’t 
trying to make a fuss, and would be happy to see his PR overridden if he saw a 
better solution.

Julian





> On Aug 29, 2018, at 10:13 AM, Michael Mior  wrote:
> 
>> I bet it is impossible to set "code of conduct" that makes everybody is
> happy
> Agreed, although we may be able to agree on a minimum standard.
> 
>> Would you call me violent if I just commit the proper fix and ignore
> PR802?
> I don't think anyone was suggesting violence.
> 
>> What if I have committed the fix yesterday?
>> What if I have committed the fix a couple of days ago?
> I don't think the issue here is timing so much as that in the case of
> CALCITE-2327, there was no effort made to run the fix past Zoltan before
> committing (please correct me if I'm wrong). In general, I think waiting a
> day or two is reasonable. Even if someone isn't able to respond in that
> window, I think people will appreciate that a heads up was given.
> 
> --
> Michael Mior
> mm...@apache.org
> 
> 
> 
> Le mer. 29 août 2018 à 04:00, Vladimir Sitnikov 
> a écrit :
> 
>> Julian>If you had just said “Hey Zoltan, I think I’ve come up with a better
>> fix than your PR; do you mind if I commit it?” then Zoltan would have said
>> “Sure”.
>> 
>> While I agree with general points (tough I bet it is impossible to set
>> "code of conduct" that makes everybody is happy), however reality is not
>> black and white.
>> 
>> What is the timeout for the answer?
>> Does "absence of answer within 2 hours" count as "sure"?
>> Does "absence of answer within 24 hours" count as "sure"?
>> Does "absence of answer within 48 hours" count as "sure"?
>> ...
>> 
>> Here's an (on-going!) example:
>> https://issues.apache.org/jira/browse/CALCITE-2484 (Dynamic table tests
>> give wrong results when running tests concurrently)
>> There's a bug, there's PR.
>> 
>> I have reviewed the PR and suggested changes. JIRA reads that my review was
>> "4 days ago".
>> Would you call me violent if I just commit the proper fix and ignore PR802?
>> What if I have committed the fix yesterday?
>> What if I have committed the fix a couple of days ago?
>> 
>> In both cases, PR/Issue author puts no warnings to the issue/pr that
>> suggest if (s)he is actively working on the problem.
>> 
>> I do agree it feels bad when your work (issue comments, code changes) is
>> discarded. However, people make mistakes, so it might happen they raise
>> tickets/do code changes that should never be made in the first place
>> (==>those changes are doomed to be discarded).
>> 
>> Vladimir
>> 

Re: Calcite contributions

[Michael Mior]

> I bet it is impossible to set "code of conduct" that makes everybody is
happy
Agreed, although we may be able to agree on a minimum standard.

> Would you call me violent if I just commit the proper fix and ignore
PR802?
I don't think anyone was suggesting violence.

> What if I have committed the fix yesterday?
> What if I have committed the fix a couple of days ago?
I don't think the issue here is timing so much as that in the case of
CALCITE-2327, there was no effort made to run the fix past Zoltan before
committing (please correct me if I'm wrong). In general, I think waiting a
day or two is reasonable. Even if someone isn't able to respond in that
window, I think people will appreciate that a heads up was given.

--
Michael Mior
mm...@apache.org



Le mer. 29 août 2018 à 04:00, Vladimir Sitnikov 
a écrit :

> Julian>If you had just said “Hey Zoltan, I think I’ve come up with a better
> fix than your PR; do you mind if I commit it?” then Zoltan would have said
> “Sure”.
>
> While I agree with general points (tough I bet it is impossible to set
> "code of conduct" that makes everybody is happy), however reality is not
> black and white.
>
> What is the timeout for the answer?
> Does "absence of answer within 2 hours" count as "sure"?
> Does "absence of answer within 24 hours" count as "sure"?
> Does "absence of answer within 48 hours" count as "sure"?
> ...
>
> Here's an (on-going!) example:
> https://issues.apache.org/jira/browse/CALCITE-2484 (Dynamic table tests
> give wrong results when running tests concurrently)
> There's a bug, there's PR.
>
> I have reviewed the PR and suggested changes. JIRA reads that my review was
> "4 days ago".
> Would you call me violent if I just commit the proper fix and ignore PR802?
> What if I have committed the fix yesterday?
> What if I have committed the fix a couple of days ago?
>
> In both cases, PR/Issue author puts no warnings to the issue/pr that
> suggest if (s)he is actively working on the problem.
>
> I do agree it feels bad when your work (issue comments, code changes) is
> discarded. However, people make mistakes, so it might happen they raise
> tickets/do code changes that should never be made in the first place
> (==>those changes are doomed to be discarded).
>
> Vladimir
>

Re: Calcite contributions

[Vladimir Sitnikov]

Julian>If you had just said “Hey Zoltan, I think I’ve come up with a better
fix than your PR; do you mind if I commit it?” then Zoltan would have said
“Sure”.

While I agree with general points (tough I bet it is impossible to set
"code of conduct" that makes everybody is happy), however reality is not
black and white.

What is the timeout for the answer?
Does "absence of answer within 2 hours" count as "sure"?
Does "absence of answer within 24 hours" count as "sure"?
Does "absence of answer within 48 hours" count as "sure"?
...

Here's an (on-going!) example:
https://issues.apache.org/jira/browse/CALCITE-2484 (Dynamic table tests
give wrong results when running tests concurrently)
There's a bug, there's PR.

I have reviewed the PR and suggested changes. JIRA reads that my review was
"4 days ago".
Would you call me violent if I just commit the proper fix and ignore PR802?
What if I have committed the fix yesterday?
What if I have committed the fix a couple of days ago?

In both cases, PR/Issue author puts no warnings to the issue/pr that
suggest if (s)he is actively working on the problem.

I do agree it feels bad when your work (issue comments, code changes) is
discarded. However, people make mistakes, so it might happen they raise
tickets/do code changes that should never be made in the first place
(==>those changes are doomed to be discarded).

Vladimir

Re: Calcite contributions

[Julian Hyde]

Again, I’m going to speak more strongly than Michael.

Vladimir,

Even if you are right, that doesn’t give you the right to be uncivil. 
Over-riding Zoltan’s PR, for which he had asked for a review, but not received, 
was not OK.

If you had just said “Hey Zoltan, I think I’ve come up with a better fix than 
your PR; do you mind if I commit it?” then Zoltan would have said “Sure”. 
Please do that next time.

If we don’t have civility and trust, this community will fall apart. 
Maintaining civility is more important than fixing a bug.

Julian


> On Aug 28, 2018, at 4:32 PM, Michael Mior  wrote:
> 
> Thanks for clarifying. It seems like this is a case where there's a broader
> discussion to be had about the merits of the optimization (completely
> separate from the issue at hand).
> 
> 1) I'm not opposed to developing a fix that's better than one which was
> already proposed although I think it would be good to run it by whoever
> originally filed the issue.
> 2) I think "backward" here is subjective. I'm not picking a side here, but
> certainly there are cases where disabling a buggy optimization is the right
> thing to do.
> 3) Developing a different fix may sometimes be the right thing to do, but I
> think other contributors would appreciate a discussion before their code is
> effectively ignored.
> 
> --
> Michael Mior
> mm...@apache.org
> 
> 
> 
> Le mar. 28 août 2018 à 17:01, Vladimir Sitnikov 
> a écrit :
> 
>> Michael>One of the other side effects in this case
>> Michael>seems to be (without having examined the technical merit of either
>> Michael>solution) that the fix which was ultimately committed still didn't
>> solve
>> Michael>the original issue.
>> 
>> I'm afraid you did are wrong here.
>> My first commit implemented exactly one test. I removed @Ignored from the
>> test and implemented a fix.
>> 
>> It turned out Zoltan crafted more complex test that identified a bug in the
>> v1 of the implementation.
>> Note: that was a new test, and it was not included in PR707.
>> Note: there are millions of test cases missing, and I know the proper way
>> to cover it.
>> However, it looks like everybody here likes "one test per issue" approach
>> more, so I follow it somehow: I unlock a single test, so everybody is
>> happy.
>> 
>> Michael>In this case, it seems like Zoltan was still willing to help
>> provide a
>> solution
>> 
>> AFAIK, no-one (including Zoltan) cares to suggest a test case to defeat
>> current code in master.
>> I treat that as "the feature is good enough".
>> 
>> Michael>see the last activity on both of those before the period of
>> inactivity was
>> by Zoltan
>> 
>> My point here is
>> 1) It takes ~30 min to "develop+test" the fix
>> 2) PR707 goes in opposite direction: it disables the optimization instead
>> of just unlocking a single @Ignore test
>> 3) The bug does bother me
>> ==> I just fix it and merge it.
>> On top of that, I see nothing I could reuse from PR707, so I had to just
>> discard it.
>> 
>> Vladimir
>> 

Re: Calcite contributions

[Michael Mior]

Thanks for clarifying. It seems like this is a case where there's a broader
discussion to be had about the merits of the optimization (completely
separate from the issue at hand).

1) I'm not opposed to developing a fix that's better than one which was
already proposed although I think it would be good to run it by whoever
originally filed the issue.
2) I think "backward" here is subjective. I'm not picking a side here, but
certainly there are cases where disabling a buggy optimization is the right
thing to do.
3) Developing a different fix may sometimes be the right thing to do, but I
think other contributors would appreciate a discussion before their code is
effectively ignored.

--
Michael Mior
mm...@apache.org



Le mar. 28 août 2018 à 17:01, Vladimir Sitnikov 
a écrit :

> Michael>One of the other side effects in this case
> Michael>seems to be (without having examined the technical merit of either
> Michael>solution) that the fix which was ultimately committed still didn't
> solve
> Michael>the original issue.
>
> I'm afraid you did are wrong here.
> My first commit implemented exactly one test. I removed @Ignored from the
> test and implemented a fix.
>
> It turned out Zoltan crafted more complex test that identified a bug in the
> v1 of the implementation.
> Note: that was a new test, and it was not included in PR707.
> Note: there are millions of test cases missing, and I know the proper way
> to cover it.
> However, it looks like everybody here likes "one test per issue" approach
> more, so I follow it somehow: I unlock a single test, so everybody is
> happy.
>
> Michael>In this case, it seems like Zoltan was still willing to help
> provide a
> solution
>
> AFAIK, no-one (including Zoltan) cares to suggest a test case to defeat
> current code in master.
> I treat that as "the feature is good enough".
>
> Michael>see the last activity on both of those before the period of
> inactivity was
> by Zoltan
>
> My point here is
> 1) It takes ~30 min to "develop+test" the fix
> 2) PR707 goes in opposite direction: it disables the optimization instead
> of just unlocking a single @Ignore test
> 3) The bug does bother me
> ==> I just fix it and merge it.
> On top of that, I see nothing I could reuse from PR707, so I had to just
> discard it.
>
> Vladimir
>

Re: Calcite contributions

[Vladimir Sitnikov]

Michael>One of the other side effects in this case
Michael>seems to be (without having examined the technical merit of either
Michael>solution) that the fix which was ultimately committed still didn't
solve
Michael>the original issue.

I'm afraid you did are wrong here.
My first commit implemented exactly one test. I removed @Ignored from the
test and implemented a fix.

It turned out Zoltan crafted more complex test that identified a bug in the
v1 of the implementation.
Note: that was a new test, and it was not included in PR707.
Note: there are millions of test cases missing, and I know the proper way
to cover it.
However, it looks like everybody here likes "one test per issue" approach
more, so I follow it somehow: I unlock a single test, so everybody is happy.

Michael>In this case, it seems like Zoltan was still willing to help
provide a
solution

AFAIK, no-one (including Zoltan) cares to suggest a test case to defeat
current code in master.
I treat that as "the feature is good enough".

Michael>see the last activity on both of those before the period of
inactivity was
by Zoltan

My point here is
1) It takes ~30 min to "develop+test" the fix
2) PR707 goes in opposite direction: it disables the optimization instead
of just unlocking a single @Ignore test
3) The bug does bother me
==> I just fix it and merge it.
On top of that, I see nothing I could reuse from PR707, so I had to just
discard it.

Vladimir

Re: Calcite contributions

[Michael Mior]

My thought was that the least that should be done is post a note that the
issue has been fixed. However (others can correct me if I'm wrong), it
seems that you did do this before Zoltan sent out his earlier email. In any
case, while it's true that both of the issue and the PR were "inactive", I
see the last activity on both of those before the period of inactivity was
by Zoltan. I don't see this as a case of the issue being abandoned. When
there's no response from a contributor, developing and committing an
alternate fix seems reasonable.

In this case, it seems like Zoltan was still willing to help provide a
solution so I don't think it's appropriate to ignore the work he put in
which seems to be what happened. One of the other side effects in this case
seems to be (without having examined the technical merit of either
solution) that the fix which was ultimately committed still didn't solve
the original issue.

--
Michael Mior
mm...@apache.org



Le mar. 28 août 2018 à 15:09, Vladimir Sitnikov 
a écrit :

> Michael>I still think
> Michael>this should be communicated to the original contributor along with
> a link
> Michael>to the fix. This at least gives the person an opportunity to learn.
>
> I have linked the original PR and the fix, and it did help Zoltan to note
> an alternative implementation.
>
> Julian>Your behavior was extremely rude.
>
> Do you mean just CALCITE-2327?
> Do you mean other cases?
>
> Julian>It does seem that you were technically correct in this case. But we
> require civility in this community.
>
> Does that imply the behavior was not civil?
>
> The bug is trivial, and I really see no reasons to spend time on discussing
> the bits.
>
> Remember: CALCITE-2327/PR707 was inactive since July. The fix takes just
> 10-20minutes.
> What sense does it make to ping the issue?
> I would rather commit the fix than spend days on discussion. No-one would
> learn from that discussion.
>
> As you remember, bike-shedding causes
> https://issues.apache.org/jira/browse/CALCITE-2438
>
> Vladimir
>

Re: Calcite contributions

[Vladimir Sitnikov]

Michael>I still think
Michael>this should be communicated to the original contributor along with
a link
Michael>to the fix. This at least gives the person an opportunity to learn.

I have linked the original PR and the fix, and it did help Zoltan to note
an alternative implementation.

Julian>Your behavior was extremely rude.

Do you mean just CALCITE-2327?
Do you mean other cases?

Julian>It does seem that you were technically correct in this case. But we
require civility in this community.

Does that imply the behavior was not civil?

The bug is trivial, and I really see no reasons to spend time on discussing
the bits.

Remember: CALCITE-2327/PR707 was inactive since July. The fix takes just
10-20minutes.
What sense does it make to ping the issue?
I would rather commit the fix than spend days on discussion. No-one would
learn from that discussion.

As you remember, bike-shedding causes
https://issues.apache.org/jira/browse/CALCITE-2438

Vladimir

Re: Calcite contributions

[Julian Hyde]

Vladimir,

I think Michael is being a bit too nice. Your behavior was extremely rude.

It does seem that you were technically correct in this case. But we require 
civility in this community.

Julian


> On Aug 28, 2018, at 11:11 AM, Michael Mior  wrote:
> 
> I understand that when there are issues with trivial fixes, it's sometimes
> much easier to just fix them yourself. I think it's still beneficial if
> people are willing to work with the original contributor to fix things, but
> I understand not everyone will be able to invest that time. If an alternate
> fix is made that supersedes a patch which was contributed, I still think
> this should be communicated to the original contributor along with a link
> to the fix. This at least gives the person an opportunity to learn.
> 
> --
> Michael Mior
> mm...@apache.org
> 
> 
> 
> Le mar. 28 août 2018 à 09:56, Vladimir Sitnikov 
> a écrit :
> 
>> Michael> But the default in those cases should hopefully be to work with
>> the
>> Michael>original contributor to make any necessary changes or in some cases
>> explain
>> 
>> Please clarify if you mean that BOTH trivial and non-trivial cases to be
>> handled in exactly the same way.
>> 
>> Vladimir
>> 

Re: Calcite contributions

[Michael Mior]

I understand that when there are issues with trivial fixes, it's sometimes
much easier to just fix them yourself. I think it's still beneficial if
people are willing to work with the original contributor to fix things, but
I understand not everyone will be able to invest that time. If an alternate
fix is made that supersedes a patch which was contributed, I still think
this should be communicated to the original contributor along with a link
to the fix. This at least gives the person an opportunity to learn.

--
Michael Mior
mm...@apache.org



Le mar. 28 août 2018 à 09:56, Vladimir Sitnikov 
a écrit :

> Michael> But the default in those cases should hopefully be to work with
> the
> Michael>original contributor to make any necessary changes or in some cases
> explain
>
> Please clarify if you mean that BOTH trivial and non-trivial cases to be
> handled in exactly the same way.
>
> Vladimir
>

Re: Calcite contributions

[Vladimir Sitnikov]

Michael> But the default in those cases should hopefully be to work with the
Michael>original contributor to make any necessary changes or in some cases
explain

Please clarify if you mean that BOTH trivial and non-trivial cases to be
handled in exactly the same way.

Vladimir

Re: Calcite contributions

[Michael Mior]

I do think it would have been more productive if this discussion had
happened earlier. I generally expect for non-trivial PRs that the first
version have issues (certainly most/all of mine do.) As committers/PMC
members, we do have a role as gatekeepers and we shouldn't merge PRs with
issues. But the default in those cases should hopefully be to work with the
original contributor to make any necessary changes or in some cases explain
why the PR is likely to never be merged (because of some inherent issue to
the approach).

Unfortunately, this often takes more time and effort than just fixing
things ourselves, but that's part of what's necessary for building a
healthy and sustainable open source project.

--
Michael Mior
mm...@apache.org



Le mar. 28 août 2018 à 08:31, Zoltan Haindrich  a écrit :

> Hello,
>
> I've found a simplification issue a while ago; written an ignored test for
> it - and opened a new jira to have the fix its own ticket:
> https://issues.apache.org/jira/browse/CALCITE-2327 - because it was not
> connected to the actual ticket.
>
> * I've submitted a patch to disable this oversimplification because that
> seemed to be the right move to me.
> * The review process became stuck after a while...
> * Today I've got a mail that my PR is got closed by Vladimir Sitnikov...I
> went to see what's up...
> * Seems like Vladimir have just committed a different patch - and closed
> the ticket (which is still assigned to me) and that's it...
> * I feel that it would have been better to at least contact me prior to
> just throwing a completely different patch at the problem - ...because
> unfortunately this new patch
> still contains an bug.
>
> I've looked at the last few commits to see if this a "pattern": for
> CALCITE-2271 something similar happened...Alexey Makhmutov have submitted a
> PR in April; 2 days ago
> Vladimir have continued and submitted his own version of it.
>
> Sorry, but I don't think the above process is "right"...
>
> cheers,
> Zoltan
>

Re: Calcite contributions

[Atri Sharma]

On Tue, Aug 28, 2018 at 6:09 PM Vladimir Sitnikov
 wrote:

> Initial analysis by Alexey was not right, so he implemented a wrong
> solution. That happens when people try to fix unfamiliar codebases.

I am completely out of line here, but this statement sounds pretty
detrimental to any potential future contributors who would want to
start small. If a contributor is going in the wrong path, it would be
helpful to point it out to him and *suggested* help him in the right
direction, rather than steam rolling his efforts. Even if a committer
wishes to re do a patch, he/she should point out the mistakes in the
contributors patch before posting a different patch.

Re: Calcite contributions

[Vladimir Sitnikov]

Zoltan>...because unfortunately this new patch  still contains an bug.

Could I please correct you a bit?
The patch did solve the issue that is represented in the issue description.
The patch did pass all the tests you had by that moment.

So "still contains a bug" is more like "there are uncovered cases".

Zoltan>* I've submitted a patch to disable this oversimplification because
that seemed to be the right move to me.

That is the culprit. Your analysis was too pessimistic, so you decided to
remove a feature rather than implement a bug fix.
There are cases when it is safer to just remove half-backed feature,
however 2327 is not of that kind.

>CALCITE-2271 something similar happened...Alexey Makhmutov

Initial analysis by Alexey was not right, so he implemented a wrong
solution. That happens when people try to fix unfamiliar codebases.

Vladimir

Calcite contributions

[Zoltan Haindrich]

Hello,

I've found a simplification issue a while ago; written an ignored test for it - 
and opened a new jira to have the fix its own ticket:
https://issues.apache.org/jira/browse/CALCITE-2327 - because it was not 
connected to the actual ticket.

* I've submitted a patch to disable this oversimplification because that seemed 
to be the right move to me.
* The review process became stuck after a while...
* Today I've got a mail that my PR is got closed by Vladimir Sitnikov...I went 
to see what's up...
* Seems like Vladimir have just committed a different patch - and closed the 
ticket (which is still assigned to me) and that's it...
* I feel that it would have been better to at least contact me prior to just throwing a completely different patch at the problem - ...because unfortunately this new patch 
still contains an bug.


I've looked at the last few commits to see if this a "pattern": for CALCITE-2271 something similar happened...Alexey Makhmutov have submitted a PR in April; 2 days ago 
Vladimir have continued and submitted his own version of it.


Sorry, but I don't think the above process is "right"...

cheers,
Zoltan