Re: [DISCUSS] Code quality/coverage with SonarCloud & JaCoCo

[Stamatis Zampetakis <za...@gmail.com>]

The integration is advancing well and I am hoping to merge the PR in the
coming days.
To avoid unpleasant surprises, I am planning to create a new remote branch
in the main calcite repo to test some things out. I will delete it as soon
as I am done with the tests.

Best.
Stamatis

On Wed, Dec 28, 2022 at 2:47 PM Michael Mior <mm...@apache.org> wrote:

> Thanks Stamatis! I haven't used SonarCloud before, but in general I think
> such tools can be quite useful.
>
> --
> Michael Mior
> mmior@apache.org
>
>
> On Sat, Dec 24, 2022 at 4:01 PM Stamatis Zampetakis <za...@gmail.com>
> wrote:
>
> > Since there were no objections, I just logged INFRA-24038 [1] and plan to
> > move this forward.
> >
> > Let me know if you have questions or concerns regarding the adoption of
> > SonarCloud.
> >
> > Best,
> > Stamatis
> >
> > [1] https://issues.apache.org/jira/browse/INFRA-24038
> >
> > On Sat, Dec 10, 2022 at 11:32 AM Benchao Li <li...@apache.org>
> wrote:
> >
> > > Thanks Stamatis for bringing this up.
> > >
> > > I haven't used Sonar yet, but thanks for the demo[1] you provided, it
> > looks
> > > really interesting. I think it's worth a try for Calcite.
> > >
> > > [1] https://github.com/zabetak/calcite/pull/9
> > >
> > > Alessandro Solimando <al...@gmail.com> 于2022年12月10日周六
> > > 02:54写道：
> > >
> > > > Hi all,
> > > > thanks Stamatis for the proposal and the work, I am a huge fan of
> Sonar
> > > and
> > > > I really miss it on Calcite, so a big +1 from me on this.
> > > >
> > > > In Hive so far we have received good feedback, so I hope it will be
> > > > welcomed here too.
> > > >
> > > > Best regards,
> > > > Alessandro
> > > >
> > > > On Fri, 9 Dec 2022 at 19:02, Stamatis Zampetakis <za...@gmail.com>
> > > > wrote:
> > > >
> > > > > Hi all,
> > > > >
> > > > > I just logged CALCITE-5427 [1] for introducing code quality &
> > coverage
> > > > > metrics in Calcite CI.
> > > > >
> > > > > I added some motivation and examples under the ticket so please
> have
> > a
> > > > look
> > > > > and let me know what you think.
> > > > >
> > > > > If there are no objections, I will follow-up with INFRA to set
> things
> > > up
> > > > > for the official Calcite repo.
> > > > >
> > > > > The integration with SonarCloud has been inspired by HIVE-26196 [2]
> > > that
> > > > > Alessandro put in place for Hive and has been very helpful so far.
> > > > >
> > > > > Best,
> > > > > Stamatis
> > > > >
> > > > > [1] https://issues.apache.org/jira/browse/CALCITE-5427
> > > > > [2] https://issues.apache.org/jira/browse/HIVE-26196
> > > > >
> > > >
> > >
> > >
> > > --
> > >
> > > Best,
> > > Benchao Li
> > >
> >
>

Re: [DISCUSS] Code quality/coverage with SonarCloud & JaCoCo

[Benchao Li <li...@apache.org>]

Thanks Francis for the pointer, it works!

Then I guess the right url should be:
https://sonarcloud.io/project/overview?id=apache_calcite
This one works for me.

Francis Chuang <fr...@apache.org> 于2023年1月16日周一 04:46写道：

> Hey Benchao,
>
> I also see that issue when trying to log in via
> https://sonarcloud.io/project/roles?id=apache_calcite. The solution is
> to log in here: https://sonarcloud.io/login, go to the Apache
> organization and then go to the calcite project.
>
> Francis
>
> On 16/01/2023 12:36 am, Benchao Li wrote:
> > When I open this url[1], it tells me that "You are not authorized to
> access
> > this page". Is this expected?
> > (I'm using my Github account)
> >
> > [1] https://sonarcloud.io/project/roles?id=apache_calcite
> >
> > Alessandro Solimando <al...@gmail.com> 于2023年1月13日周五
> 18:51写道：
> >
> >> Thank you Stamatis for working on this, I see no need to remove the
> Sonar
> >> analysis now that the quality gate is off.
> >>
> >> In the meantime, since all committers have write privilege in Sonar,
> >> whoever is interested can help fine-tuning it to fit our needs (for
> >> instance disabling rules like the code smell for TODOs if unwanted), on
> a
> >> voluntary basis, of course.
> >>
> >> Best regards,
> >> Alessandro
> >>
> >> On Fri, 13 Jan 2023 at 11:23, Stamatis Zampetakis <za...@gmail.com>
> >> wrote:
> >>
> >>> I logged CALCITE-5474 [1] to disable the failures in Sonar checks
> >>> allowing everything to appear as green.
> >>>
> >>> Sonar annotations are still going to appear under the PR after
> >>> CALCITE-5474; if the intention is to remove also these indications let
> >>> me know and I will log another ticket.
> >>>
> >>> Best,
> >>> Stamatis
> >>>
> >>> [1] https://issues.apache.org/jira/browse/CALCITE-5474
> >>>
> >>> On Fri, Jan 13, 2023 at 2:53 AM Julian Hyde <jh...@apache.org> wrote:
> >>>
> >>>> Now a build 'failed' with 7 'code smells'.
> >>>>
> >>>> Duplicating a string literal in a test was deemed as 'critical code
> >>>> smell'. For heaven's sake.
> >>>>
> >>>>
> >>>>
> >>>
> >>
> https://sonarcloud.io/project/issues?resolved=false&severities=CRITICAL&types=CODE_SMELL&pullRequest=2942&id=apache_calcite
> >>>>
> >>>> Adding '@Deprecated' without also adding a javadoc comment that
> >>>> contains '@deprecated' is a 'major code smell'. (I'm guessing that if
> >>>> I add a javadoc comment that only contains '@deprecated' it will tell
> >>>> me that empty javadoc is a code smell.)
> >>>>
> >>>> And "Do not forget to remove this deprecated code someday." is an
> >>>> 'info code smell'. Yeah, right. Wait until the next major version.
> >>>>
> >>>> I'm trying to work here. Get this *****ing robot off my back.
> >>>>
> >>>> Julian
> >>>>
> >>>> On Wed, Jan 11, 2023 at 10:41 PM Alessandro Solimando
> >>>> <al...@gmail.com> wrote:
> >>>>>
> >>>>> Hi everyone,
> >>>>> I agree with Julian that we should be open to see what value Sonar
> >>> brings
> >>>>> with the current setup, but for a true accounting we need many more
> >>> data
> >>>>> points, two examples are just not enough.
> >>>>>
> >>>>> In my experience I see reviewers asking contributors to fix real
> >> issues
> >>>>> that Sonarlint plugin can highlight in IntelliJ even locally.
> >>>>>
> >>>>> So Sonar would save those reviewers time if the contributor would
> >>> review
> >>>>> and fix some of them autonomously.
> >>>>>
> >>>>> If we take Sonar as an opportunity to fix easily 80% (à la Pareto) of
> >>> the
> >>>>> trivial issues we generally see in contributions rather than
> >>> considering
> >>>>> each and every issue as blocking, we can have a positive net IMO.
> >>>>>
> >>>>> There are also fine tunings and exclusions to be added over time for
> >>>>> accepted "issues" (like the test class under src), like Ruben was
> >>>> proposing.
> >>>>>
> >>>>> I was the one who did the Sonar setup in Hive, I had mostly positive
> >>>>> feedback by contributors who just took Sonar as an opportunity to fix
> >>>> some
> >>>>> bugs and improve code, the only difference is that we do not have any
> >>>>> quality gate there, so the report is never marked as "failed", it's
> >> at
> >>>> the
> >>>>> sole discretion of the contributor+reviewer to take it into account
> >> or
> >>>> not.
> >>>>>
> >>>>> I personally don't fix all possible warnings/code smells, but most of
> >>>> them
> >>>>> yes. Some are just fine as-is to me and they can even be considered
> >>> false
> >>>>> positives.
> >>>>>
> >>>>> Best regards,
> >>>>> Alessandro
> >>>>>
> >>>>>
> >>>>> On Wed 11 Jan 2023, 23:17 Julian Hyde, <jh...@gmail.com>
> >> wrote:
> >>>>>
> >>>>>> The instanceof case was complicated. The code that Kevin wrote was
> >>>> good,
> >>>>>> and appropriate, and when Sonar blocked it Stamatis was able to
> >> find
> >>> an
> >>>>>> alternative formulation, which existed because, by luck, the JSR
> >> had
> >>>>>> deprecated an exception class but not its base class. I spent 30
> >>>> minutes
> >>>>>> reviewing the PR and was about to merge it, and because of Sonar’s
> >>>> bump in
> >>>>>> the road that time was wasted. I doubt that there has been a single
> >>>> other
> >>>>>> occasion when someone wrote
> >>>>>> “com.example.MyClass”.equals(x.getClass().getName()) instead of “x
> >>>>>> instanceof MyClass”. So far that particular check has cost us ~1
> >> hour
> >>>> and
> >>>>>> not saved us any time.
> >>>>>>
> >>>>>> I’m not saying that Sonar is net bad. I’m just saying let’s do a
> >> true
> >>>>>> accounting.
> >>>>>>
> >>>>>>
> >>>>>>> On Jan 11, 2023, at 2:42 AM, Ruben Q L <ru...@gmail.com>
> >> wrote:
> >>>>>>>
> >>>>>>> Hello,
> >>>>>>>
> >>>>>>> First of all, thanks Stamatis for implementing this, I think it
> >> is
> >>>>>>> something good for the project.
> >>>>>>> In the beginning things might be a bit complicated (as always)
> >> and
> >>> we
> >>>>>> might
> >>>>>>> need some adjustments / clarifications, but I hope that in the
> >> long
> >>>> run
> >>>>>>> we'll see this as a useful feature.
> >>>>>>>
> >>>>>>> Regarding the two specific issues being discussed:
> >>>>>>> - If I am not mistaken, the fact that SqlOperatorTest was moved
> >> out
> >>>> of
> >>>>>>> 'test' was a consequence of [1], see the corresponding PR [2]
> >> "...
> >>>> it was
> >>>>>>> necessary to move several classes from the 'core' module to
> >>>> 'testkit'". I
> >>>>>>> don't know how simple (or how complex) a potential refactoring to
> >>>> move it
> >>>>>>> out of there might be. Alternatively, it seems that this is
> >> rather
> >>> an
> >>>>>>> exceptional case, so perhaps it should qualify for an exception
> >>> (e.g.
> >>>>>>> everything under /testkit/* shall not be considered for
> >> coverage).
> >>>>>>> - Regarding the instanceof, it seems that it was indeed a valid
> >>>> warning,
> >>>>>>> and it has recently been fixed via [3] (see discussion on its PR
> >>> [4])
> >>>>>>>
> >>>>>>> Best regards,
> >>>>>>> Ruben
> >>>>>>>
> >>>>>>> [1] https://issues.apache.org/jira/browse/CALCITE-4885
> >>>>>>> [2] https://github.com/apache/calcite/pull/2685
> >>>>>>> [3]
> >>>>>>>
> >>>>>>
> >>>>
> >>>
> >>
> https://github.com/apache/calcite/commit/4bebdb07c2f45a95c9a4fdf81e9bcfbdd11a15de
> >>>>>>> [4] https://github.com/apache/calcite/pull/2919
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> On Tue, Jan 10, 2023 at 3:18 PM Stamatis Zampetakis <
> >>>> zabetak@gmail.com>
> >>>>>>> wrote:
> >>>>>>>
> >>>>>>>> Thanks for the feedback!
> >>>>>>>>
> >>>>>>>> I would like to stretch the fact that at this point it is at the
> >>>>>> discretion
> >>>>>>>> of the reviewer/committer to enforce or ignore the information
> >>>> provided
> >>>>>> by
> >>>>>>>> Sonar.
> >>>>>>>>
> >>>>>>>> Sonar, as other similar systems, has limitations thus there are
> >>>> always
> >>>>>>>> going to be false positives. The rules/checks performed are
> >> fully
> >>>>>>>> customisable so we can enable/disable them at will.
> >>>>>>>>
> >>>>>>>> The two issues highlighted by Julian seem like true positives to
> >>> me.
> >>>>>>>> * The "New code" that was introduced recently is not covered
> >>>>>> sufficiently
> >>>>>>>> by tests and that's a fact. Part of the problem seems to come
> >> from
> >>>> the
> >>>>>>>> recent modifications in SqlOperatorTest [1]. The class is
> >> located
> >>>> under
> >>>>>>>> src/main (and not under src/test) so it is considered as a
> >>>> production
> >>>>>> class
> >>>>>>>> and coverage checks are applied. There are ways to exclude
> >> certain
> >>>> paths
> >>>>>>>> from coverage but I would argue that the class shouldn't be
> >> there
> >>>> in the
> >>>>>>>> first place; we should probably log a CALCITE ticket for moving
> >>> out
> >>>> of
> >>>>>>>> there.
> >>>>>>>> * The instance of warning is something that we probably don't
> >>>>>> want/cannot
> >>>>>>>> fix (for the reasons mentioned in the PR) but Sonar did well to
> >>>> bring
> >>>>>> this
> >>>>>>>> up; it is problematic to do comparisons by relying on the class
> >>>> name.
> >>>>>>>>
> >>>>>>>> I encourage others to share their thoughts/remarks as well so
> >> that
> >>>> we
> >>>>>>>> improve as much as possible the developer experience.
> >>>>>>>>
> >>>>>>>> Best,
> >>>>>>>> Stamatis
> >>>>>>>>
> >>>>>>>> [1]
> >>>>>>>>
> >>>>>>>>
> >>>>>>
> >>>>
> >>>
> >>
> https://github.com/apache/calcite/blob/be7135cf1fd3d87e4036b2dd6e58d2f1251f8959/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java
> >>>>>>>>
> >>>>>>>> On Tue, Jan 10, 2023 at 1:38 AM Julian Hyde <jh...@apache.org>
> >>>> wrote:
> >>>>>>>>
> >>>>>>>>> I see two false positives so far:
> >>>>>>>>> * The message on be7135cf "58.1% Coverage on New Code (is less
> >>> than
> >>>>>>>> 80%)"
> >>>>>>>>> * The bug on PR 2942 "Use an "instanceof" comparison instead"
> >>>>>>>>>
> >>>>>>>>> Has Sonarcube had any true positives yet?
> >>>>>>>>>
> >>>>>>>>> Vladimir used to hate when I was skeptical of changes to the
> >>> build
> >>>>>>>>> system. But I have no tolerance for a lint system that makes
> >>> extra
> >>>>>>>>> work.
> >>>>>>>>>
> >>>>>>>>> Julian
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> On Mon, Jan 9, 2023 at 2:46 PM Francis Chuang <
> >>>>>> francischuang@apache.org>
> >>>>>>>>> wrote:
> >>>>>>>>>>
> >>>>>>>>>> Thanks for implementing this, Stamatis! Having code quality
> >>>> metrics on
> >>>>>>>>>> our repos is a huge win.
> >>>>>>>>>>
> >>>>>>>>>> Francis
> >>>>>>>>>>
> >>>>>>>>>> On 10/01/2023 4:22 am, Stamatis Zampetakis wrote:
> >>>>>>>>>>> I just merged the CALCITE-5427 [1] enabling Sonarcloud
> >> analysis
> >>>> for
> >>>>>>>>> Calcite
> >>>>>>>>>>> main branch and new PRs.
> >>>>>>>>>>>
> >>>>>>>>>>> I have left the default Sonar quality gates active so you may
> >>>> start
> >>>>>>>>> seeing
> >>>>>>>>>>> Sonar reporting errors in various cases.
> >>>>>>>>>>>
> >>>>>>>>>>> If you encounter problems or you would like things to work
> >>>>>>>> differently
> >>>>>>>>>>> please create JIRA tickets and ping me if you need help.
> >>>>>>>>>>>
> >>>>>>>>>>> Once we are happy with how things work for Calcite we can
> >> also
> >>>> port
> >>>>>>>> the
> >>>>>>>>>>> changes to Avatica.
> >>>>>>>>>>>
> >>>>>>>>>>> Note that all Calcite committers have administrative
> >> privileges
> >>>> to
> >>>>>>>> the
> >>>>>>>>>>> Calcite project in Sonarcloud [2].
> >>>>>>>>>>>
> >>>>>>>>>>> Best,
> >>>>>>>>>>> Stamatis
> >>>>>>>>>>>
> >>>>>>>>>>> [1]
> >>>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>
> >>>>>>
> >>>>
> >>>
> >>
> https://github.com/apache/calcite/commit/be7135cf1fd3d87e4036b2dd6e58d2f1251f8959
> >>>>>>>>>>> [2] https://sonarcloud.io/project/roles?id=apache_calcite
> >>>>>>>>>>>
> >>>>>>>>>>> On Wed, Jan 4, 2023 at 5:44 PM Stamatis Zampetakis <
> >>>>>>>> zabetak@gmail.com>
> >>>>>>>>>>> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>>> The integration is advancing well and I am hoping to merge
> >> the
> >>>> PR in
> >>>>>>>>> the
> >>>>>>>>>>>> coming days.
> >>>>>>>>>>>> To avoid unpleasant surprises, I am planning to create a new
> >>>> remote
> >>>>>>>>> branch
> >>>>>>>>>>>> in the main calcite repo to test some things out. I will
> >>> delete
> >>>> it
> >>>>>>>> as
> >>>>>>>>> soon
> >>>>>>>>>>>> as I am done with the tests.
> >>>>>>>>>>>>
> >>>>>>>>>>>> Best.
> >>>>>>>>>>>> Stamatis
> >>>>>>>>>>>>
> >>>>>>>>>>>> On Wed, Dec 28, 2022 at 2:47 PM Michael Mior <
> >>> mmior@apache.org>
> >>>>>>>>> wrote:
> >>>>>>>>>>>>
> >>>>>>>>>>>>> Thanks Stamatis! I haven't used SonarCloud before, but in
> >>>> general I
> >>>>>>>>> think
> >>>>>>>>>>>>> such tools can be quite useful.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> --
> >>>>>>>>>>>>> Michael Mior
> >>>>>>>>>>>>> mmior@apache.org
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> On Sat, Dec 24, 2022 at 4:01 PM Stamatis Zampetakis <
> >>>>>>>>> zabetak@gmail.com>
> >>>>>>>>>>>>> wrote:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>> Since there were no objections, I just logged INFRA-24038
> >>> [1]
> >>>> and
> >>>>>>>>> plan
> >>>>>>>>>>>>> to
> >>>>>>>>>>>>>> move this forward.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Let me know if you have questions or concerns regarding
> >> the
> >>>>>>>>> adoption of
> >>>>>>>>>>>>>> SonarCloud.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Best,
> >>>>>>>>>>>>>> Stamatis
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> [1] https://issues.apache.org/jira/browse/INFRA-24038
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> On Sat, Dec 10, 2022 at 11:32 AM Benchao Li <
> >>>> libenchao@apache.org
> >>>>>>>>>
> >>>>>>>>>>>>> wrote:
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Thanks Stamatis for bringing this up.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> I haven't used Sonar yet, but thanks for the demo[1] you
> >>>>>>>> provided,
> >>>>>>>>> it
> >>>>>>>>>>>>>> looks
> >>>>>>>>>>>>>>> really interesting. I think it's worth a try for Calcite.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> [1] https://github.com/zabetak/calcite/pull/9
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Alessandro Solimando <al...@gmail.com>
> >>>>>>>>> 于2022年12月10日周六
> >>>>>>>>>>>>>>> 02:54写道：
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> Hi all,
> >>>>>>>>>>>>>>>> thanks Stamatis for the proposal and the work, I am a
> >> huge
> >>>> fan
> >>>>>>>> of
> >>>>>>>>>>>>> Sonar
> >>>>>>>>>>>>>>> and
> >>>>>>>>>>>>>>>> I really miss it on Calcite, so a big +1 from me on
> >> this.
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> In Hive so far we have received good feedback, so I hope
> >>> it
> >>>> will
> >>>>>>>>> be
> >>>>>>>>>>>>>>>> welcomed here too.
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> Best regards,
> >>>>>>>>>>>>>>>> Alessandro
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> On Fri, 9 Dec 2022 at 19:02, Stamatis Zampetakis <
> >>>>>>>>> zabetak@gmail.com
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> wrote:
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> Hi all,
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> I just logged CALCITE-5427 [1] for introducing code
> >>>> quality &
> >>>>>>>>>>>>>> coverage
> >>>>>>>>>>>>>>>>> metrics in Calcite CI.
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> I added some motivation and examples under the ticket
> >> so
> >>>> please
> >>>>>>>>>>>>> have
> >>>>>>>>>>>>>> a
> >>>>>>>>>>>>>>>> look
> >>>>>>>>>>>>>>>>> and let me know what you think.
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> If there are no objections, I will follow-up with INFRA
> >>> to
> >>>> set
> >>>>>>>>>>>>> things
> >>>>>>>>>>>>>>> up
> >>>>>>>>>>>>>>>>> for the official Calcite repo.
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> The integration with SonarCloud has been inspired by
> >>>> HIVE-26196
> >>>>>>>>>>>>> [2]
> >>>>>>>>>>>>>>> that
> >>>>>>>>>>>>>>>>> Alessandro put in place for Hive and has been very
> >>> helpful
> >>>> so
> >>>>>>>>> far.
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> Best,
> >>>>>>>>>>>>>>>>> Stamatis
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> [1] https://issues.apache.org/jira/browse/CALCITE-5427
> >>>>>>>>>>>>>>>>> [2] https://issues.apache.org/jira/browse/HIVE-26196
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> --
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Best,
> >>>>>>>>>>>>>>> Benchao Li
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>
> >>>>>>
> >>>>>>
> >>>>
> >>>
> >>
> >
> >
>


-- 

Best,
Benchao Li

Re: [DISCUSS] Code quality/coverage with SonarCloud & JaCoCo

[Francis Chuang <fr...@apache.org>]

Hey Benchao,

I also see that issue when trying to log in via 
https://sonarcloud.io/project/roles?id=apache_calcite. The solution is 
to log in here: https://sonarcloud.io/login, go to the Apache 
organization and then go to the calcite project.

Francis

On 16/01/2023 12:36 am, Benchao Li wrote:
> When I open this url[1], it tells me that "You are not authorized to access
> this page". Is this expected?
> (I'm using my Github account)
> 
> [1] https://sonarcloud.io/project/roles?id=apache_calcite
> 
> Alessandro Solimando <al...@gmail.com> 于2023年1月13日周五 18:51写道：
> 
>> Thank you Stamatis for working on this, I see no need to remove the Sonar
>> analysis now that the quality gate is off.
>>
>> In the meantime, since all committers have write privilege in Sonar,
>> whoever is interested can help fine-tuning it to fit our needs (for
>> instance disabling rules like the code smell for TODOs if unwanted), on a
>> voluntary basis, of course.
>>
>> Best regards,
>> Alessandro
>>
>> On Fri, 13 Jan 2023 at 11:23, Stamatis Zampetakis <za...@gmail.com>
>> wrote:
>>
>>> I logged CALCITE-5474 [1] to disable the failures in Sonar checks
>>> allowing everything to appear as green.
>>>
>>> Sonar annotations are still going to appear under the PR after
>>> CALCITE-5474; if the intention is to remove also these indications let
>>> me know and I will log another ticket.
>>>
>>> Best,
>>> Stamatis
>>>
>>> [1] https://issues.apache.org/jira/browse/CALCITE-5474
>>>
>>> On Fri, Jan 13, 2023 at 2:53 AM Julian Hyde <jh...@apache.org> wrote:
>>>
>>>> Now a build 'failed' with 7 'code smells'.
>>>>
>>>> Duplicating a string literal in a test was deemed as 'critical code
>>>> smell'. For heaven's sake.
>>>>
>>>>
>>>>
>>>
>> https://sonarcloud.io/project/issues?resolved=false&severities=CRITICAL&types=CODE_SMELL&pullRequest=2942&id=apache_calcite
>>>>
>>>> Adding '@Deprecated' without also adding a javadoc comment that
>>>> contains '@deprecated' is a 'major code smell'. (I'm guessing that if
>>>> I add a javadoc comment that only contains '@deprecated' it will tell
>>>> me that empty javadoc is a code smell.)
>>>>
>>>> And "Do not forget to remove this deprecated code someday." is an
>>>> 'info code smell'. Yeah, right. Wait until the next major version.
>>>>
>>>> I'm trying to work here. Get this *****ing robot off my back.
>>>>
>>>> Julian
>>>>
>>>> On Wed, Jan 11, 2023 at 10:41 PM Alessandro Solimando
>>>> <al...@gmail.com> wrote:
>>>>>
>>>>> Hi everyone,
>>>>> I agree with Julian that we should be open to see what value Sonar
>>> brings
>>>>> with the current setup, but for a true accounting we need many more
>>> data
>>>>> points, two examples are just not enough.
>>>>>
>>>>> In my experience I see reviewers asking contributors to fix real
>> issues
>>>>> that Sonarlint plugin can highlight in IntelliJ even locally.
>>>>>
>>>>> So Sonar would save those reviewers time if the contributor would
>>> review
>>>>> and fix some of them autonomously.
>>>>>
>>>>> If we take Sonar as an opportunity to fix easily 80% (à la Pareto) of
>>> the
>>>>> trivial issues we generally see in contributions rather than
>>> considering
>>>>> each and every issue as blocking, we can have a positive net IMO.
>>>>>
>>>>> There are also fine tunings and exclusions to be added over time for
>>>>> accepted "issues" (like the test class under src), like Ruben was
>>>> proposing.
>>>>>
>>>>> I was the one who did the Sonar setup in Hive, I had mostly positive
>>>>> feedback by contributors who just took Sonar as an opportunity to fix
>>>> some
>>>>> bugs and improve code, the only difference is that we do not have any
>>>>> quality gate there, so the report is never marked as "failed", it's
>> at
>>>> the
>>>>> sole discretion of the contributor+reviewer to take it into account
>> or
>>>> not.
>>>>>
>>>>> I personally don't fix all possible warnings/code smells, but most of
>>>> them
>>>>> yes. Some are just fine as-is to me and they can even be considered
>>> false
>>>>> positives.
>>>>>
>>>>> Best regards,
>>>>> Alessandro
>>>>>
>>>>>
>>>>> On Wed 11 Jan 2023, 23:17 Julian Hyde, <jh...@gmail.com>
>> wrote:
>>>>>
>>>>>> The instanceof case was complicated. The code that Kevin wrote was
>>>> good,
>>>>>> and appropriate, and when Sonar blocked it Stamatis was able to
>> find
>>> an
>>>>>> alternative formulation, which existed because, by luck, the JSR
>> had
>>>>>> deprecated an exception class but not its base class. I spent 30
>>>> minutes
>>>>>> reviewing the PR and was about to merge it, and because of Sonar’s
>>>> bump in
>>>>>> the road that time was wasted. I doubt that there has been a single
>>>> other
>>>>>> occasion when someone wrote
>>>>>> “com.example.MyClass”.equals(x.getClass().getName()) instead of “x
>>>>>> instanceof MyClass”. So far that particular check has cost us ~1
>> hour
>>>> and
>>>>>> not saved us any time.
>>>>>>
>>>>>> I’m not saying that Sonar is net bad. I’m just saying let’s do a
>> true
>>>>>> accounting.
>>>>>>
>>>>>>
>>>>>>> On Jan 11, 2023, at 2:42 AM, Ruben Q L <ru...@gmail.com>
>> wrote:
>>>>>>>
>>>>>>> Hello,
>>>>>>>
>>>>>>> First of all, thanks Stamatis for implementing this, I think it
>> is
>>>>>>> something good for the project.
>>>>>>> In the beginning things might be a bit complicated (as always)
>> and
>>> we
>>>>>> might
>>>>>>> need some adjustments / clarifications, but I hope that in the
>> long
>>>> run
>>>>>>> we'll see this as a useful feature.
>>>>>>>
>>>>>>> Regarding the two specific issues being discussed:
>>>>>>> - If I am not mistaken, the fact that SqlOperatorTest was moved
>> out
>>>> of
>>>>>>> 'test' was a consequence of [1], see the corresponding PR [2]
>> "...
>>>> it was
>>>>>>> necessary to move several classes from the 'core' module to
>>>> 'testkit'". I
>>>>>>> don't know how simple (or how complex) a potential refactoring to
>>>> move it
>>>>>>> out of there might be. Alternatively, it seems that this is
>> rather
>>> an
>>>>>>> exceptional case, so perhaps it should qualify for an exception
>>> (e.g.
>>>>>>> everything under /testkit/* shall not be considered for
>> coverage).
>>>>>>> - Regarding the instanceof, it seems that it was indeed a valid
>>>> warning,
>>>>>>> and it has recently been fixed via [3] (see discussion on its PR
>>> [4])
>>>>>>>
>>>>>>> Best regards,
>>>>>>> Ruben
>>>>>>>
>>>>>>> [1] https://issues.apache.org/jira/browse/CALCITE-4885
>>>>>>> [2] https://github.com/apache/calcite/pull/2685
>>>>>>> [3]
>>>>>>>
>>>>>>
>>>>
>>>
>> https://github.com/apache/calcite/commit/4bebdb07c2f45a95c9a4fdf81e9bcfbdd11a15de
>>>>>>> [4] https://github.com/apache/calcite/pull/2919
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Tue, Jan 10, 2023 at 3:18 PM Stamatis Zampetakis <
>>>> zabetak@gmail.com>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> Thanks for the feedback!
>>>>>>>>
>>>>>>>> I would like to stretch the fact that at this point it is at the
>>>>>> discretion
>>>>>>>> of the reviewer/committer to enforce or ignore the information
>>>> provided
>>>>>> by
>>>>>>>> Sonar.
>>>>>>>>
>>>>>>>> Sonar, as other similar systems, has limitations thus there are
>>>> always
>>>>>>>> going to be false positives. The rules/checks performed are
>> fully
>>>>>>>> customisable so we can enable/disable them at will.
>>>>>>>>
>>>>>>>> The two issues highlighted by Julian seem like true positives to
>>> me.
>>>>>>>> * The "New code" that was introduced recently is not covered
>>>>>> sufficiently
>>>>>>>> by tests and that's a fact. Part of the problem seems to come
>> from
>>>> the
>>>>>>>> recent modifications in SqlOperatorTest [1]. The class is
>> located
>>>> under
>>>>>>>> src/main (and not under src/test) so it is considered as a
>>>> production
>>>>>> class
>>>>>>>> and coverage checks are applied. There are ways to exclude
>> certain
>>>> paths
>>>>>>>> from coverage but I would argue that the class shouldn't be
>> there
>>>> in the
>>>>>>>> first place; we should probably log a CALCITE ticket for moving
>>> out
>>>> of
>>>>>>>> there.
>>>>>>>> * The instance of warning is something that we probably don't
>>>>>> want/cannot
>>>>>>>> fix (for the reasons mentioned in the PR) but Sonar did well to
>>>> bring
>>>>>> this
>>>>>>>> up; it is problematic to do comparisons by relying on the class
>>>> name.
>>>>>>>>
>>>>>>>> I encourage others to share their thoughts/remarks as well so
>> that
>>>> we
>>>>>>>> improve as much as possible the developer experience.
>>>>>>>>
>>>>>>>> Best,
>>>>>>>> Stamatis
>>>>>>>>
>>>>>>>> [1]
>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>>
>> https://github.com/apache/calcite/blob/be7135cf1fd3d87e4036b2dd6e58d2f1251f8959/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java
>>>>>>>>
>>>>>>>> On Tue, Jan 10, 2023 at 1:38 AM Julian Hyde <jh...@apache.org>
>>>> wrote:
>>>>>>>>
>>>>>>>>> I see two false positives so far:
>>>>>>>>> * The message on be7135cf "58.1% Coverage on New Code (is less
>>> than
>>>>>>>> 80%)"
>>>>>>>>> * The bug on PR 2942 "Use an "instanceof" comparison instead"
>>>>>>>>>
>>>>>>>>> Has Sonarcube had any true positives yet?
>>>>>>>>>
>>>>>>>>> Vladimir used to hate when I was skeptical of changes to the
>>> build
>>>>>>>>> system. But I have no tolerance for a lint system that makes
>>> extra
>>>>>>>>> work.
>>>>>>>>>
>>>>>>>>> Julian
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Mon, Jan 9, 2023 at 2:46 PM Francis Chuang <
>>>>>> francischuang@apache.org>
>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>> Thanks for implementing this, Stamatis! Having code quality
>>>> metrics on
>>>>>>>>>> our repos is a huge win.
>>>>>>>>>>
>>>>>>>>>> Francis
>>>>>>>>>>
>>>>>>>>>> On 10/01/2023 4:22 am, Stamatis Zampetakis wrote:
>>>>>>>>>>> I just merged the CALCITE-5427 [1] enabling Sonarcloud
>> analysis
>>>> for
>>>>>>>>> Calcite
>>>>>>>>>>> main branch and new PRs.
>>>>>>>>>>>
>>>>>>>>>>> I have left the default Sonar quality gates active so you may
>>>> start
>>>>>>>>> seeing
>>>>>>>>>>> Sonar reporting errors in various cases.
>>>>>>>>>>>
>>>>>>>>>>> If you encounter problems or you would like things to work
>>>>>>>> differently
>>>>>>>>>>> please create JIRA tickets and ping me if you need help.
>>>>>>>>>>>
>>>>>>>>>>> Once we are happy with how things work for Calcite we can
>> also
>>>> port
>>>>>>>> the
>>>>>>>>>>> changes to Avatica.
>>>>>>>>>>>
>>>>>>>>>>> Note that all Calcite committers have administrative
>> privileges
>>>> to
>>>>>>>> the
>>>>>>>>>>> Calcite project in Sonarcloud [2].
>>>>>>>>>>>
>>>>>>>>>>> Best,
>>>>>>>>>>> Stamatis
>>>>>>>>>>>
>>>>>>>>>>> [1]
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>>
>> https://github.com/apache/calcite/commit/be7135cf1fd3d87e4036b2dd6e58d2f1251f8959
>>>>>>>>>>> [2] https://sonarcloud.io/project/roles?id=apache_calcite
>>>>>>>>>>>
>>>>>>>>>>> On Wed, Jan 4, 2023 at 5:44 PM Stamatis Zampetakis <
>>>>>>>> zabetak@gmail.com>
>>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> The integration is advancing well and I am hoping to merge
>> the
>>>> PR in
>>>>>>>>> the
>>>>>>>>>>>> coming days.
>>>>>>>>>>>> To avoid unpleasant surprises, I am planning to create a new
>>>> remote
>>>>>>>>> branch
>>>>>>>>>>>> in the main calcite repo to test some things out. I will
>>> delete
>>>> it
>>>>>>>> as
>>>>>>>>> soon
>>>>>>>>>>>> as I am done with the tests.
>>>>>>>>>>>>
>>>>>>>>>>>> Best.
>>>>>>>>>>>> Stamatis
>>>>>>>>>>>>
>>>>>>>>>>>> On Wed, Dec 28, 2022 at 2:47 PM Michael Mior <
>>> mmior@apache.org>
>>>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks Stamatis! I haven't used SonarCloud before, but in
>>>> general I
>>>>>>>>> think
>>>>>>>>>>>>> such tools can be quite useful.
>>>>>>>>>>>>>
>>>>>>>>>>>>> --
>>>>>>>>>>>>> Michael Mior
>>>>>>>>>>>>> mmior@apache.org
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Sat, Dec 24, 2022 at 4:01 PM Stamatis Zampetakis <
>>>>>>>>> zabetak@gmail.com>
>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Since there were no objections, I just logged INFRA-24038
>>> [1]
>>>> and
>>>>>>>>> plan
>>>>>>>>>>>>> to
>>>>>>>>>>>>>> move this forward.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Let me know if you have questions or concerns regarding
>> the
>>>>>>>>> adoption of
>>>>>>>>>>>>>> SonarCloud.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Best,
>>>>>>>>>>>>>> Stamatis
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> [1] https://issues.apache.org/jira/browse/INFRA-24038
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Sat, Dec 10, 2022 at 11:32 AM Benchao Li <
>>>> libenchao@apache.org
>>>>>>>>>
>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks Stamatis for bringing this up.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I haven't used Sonar yet, but thanks for the demo[1] you
>>>>>>>> provided,
>>>>>>>>> it
>>>>>>>>>>>>>> looks
>>>>>>>>>>>>>>> really interesting. I think it's worth a try for Calcite.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> [1] https://github.com/zabetak/calcite/pull/9
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Alessandro Solimando <al...@gmail.com>
>>>>>>>>> 于2022年12月10日周六
>>>>>>>>>>>>>>> 02:54写道：
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>>>>> thanks Stamatis for the proposal and the work, I am a
>> huge
>>>> fan
>>>>>>>> of
>>>>>>>>>>>>> Sonar
>>>>>>>>>>>>>>> and
>>>>>>>>>>>>>>>> I really miss it on Calcite, so a big +1 from me on
>> this.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> In Hive so far we have received good feedback, so I hope
>>> it
>>>> will
>>>>>>>>> be
>>>>>>>>>>>>>>>> welcomed here too.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Best regards,
>>>>>>>>>>>>>>>> Alessandro
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On Fri, 9 Dec 2022 at 19:02, Stamatis Zampetakis <
>>>>>>>>> zabetak@gmail.com
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> I just logged CALCITE-5427 [1] for introducing code
>>>> quality &
>>>>>>>>>>>>>> coverage
>>>>>>>>>>>>>>>>> metrics in Calcite CI.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> I added some motivation and examples under the ticket
>> so
>>>> please
>>>>>>>>>>>>> have
>>>>>>>>>>>>>> a
>>>>>>>>>>>>>>>> look
>>>>>>>>>>>>>>>>> and let me know what you think.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> If there are no objections, I will follow-up with INFRA
>>> to
>>>> set
>>>>>>>>>>>>> things
>>>>>>>>>>>>>>> up
>>>>>>>>>>>>>>>>> for the official Calcite repo.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> The integration with SonarCloud has been inspired by
>>>> HIVE-26196
>>>>>>>>>>>>> [2]
>>>>>>>>>>>>>>> that
>>>>>>>>>>>>>>>>> Alessandro put in place for Hive and has been very
>>> helpful
>>>> so
>>>>>>>>> far.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Best,
>>>>>>>>>>>>>>>>> Stamatis
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> [1] https://issues.apache.org/jira/browse/CALCITE-5427
>>>>>>>>>>>>>>>>> [2] https://issues.apache.org/jira/browse/HIVE-26196
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> --
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Best,
>>>>>>>>>>>>>>> Benchao Li
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>
>>
> 
> 

Re: [DISCUSS] Code quality/coverage with SonarCloud & JaCoCo

[Benchao Li <li...@apache.org>]

When I open this url[1], it tells me that "You are not authorized to access
this page". Is this expected?
(I'm using my Github account)

[1] https://sonarcloud.io/project/roles?id=apache_calcite

Alessandro Solimando <al...@gmail.com> 于2023年1月13日周五 18:51写道：

> Thank you Stamatis for working on this, I see no need to remove the Sonar
> analysis now that the quality gate is off.
>
> In the meantime, since all committers have write privilege in Sonar,
> whoever is interested can help fine-tuning it to fit our needs (for
> instance disabling rules like the code smell for TODOs if unwanted), on a
> voluntary basis, of course.
>
> Best regards,
> Alessandro
>
> On Fri, 13 Jan 2023 at 11:23, Stamatis Zampetakis <za...@gmail.com>
> wrote:
>
> > I logged CALCITE-5474 [1] to disable the failures in Sonar checks
> > allowing everything to appear as green.
> >
> > Sonar annotations are still going to appear under the PR after
> > CALCITE-5474; if the intention is to remove also these indications let
> > me know and I will log another ticket.
> >
> > Best,
> > Stamatis
> >
> > [1] https://issues.apache.org/jira/browse/CALCITE-5474
> >
> > On Fri, Jan 13, 2023 at 2:53 AM Julian Hyde <jh...@apache.org> wrote:
> >
> > > Now a build 'failed' with 7 'code smells'.
> > >
> > > Duplicating a string literal in a test was deemed as 'critical code
> > > smell'. For heaven's sake.
> > >
> > >
> > >
> >
> https://sonarcloud.io/project/issues?resolved=false&severities=CRITICAL&types=CODE_SMELL&pullRequest=2942&id=apache_calcite
> > >
> > > Adding '@Deprecated' without also adding a javadoc comment that
> > > contains '@deprecated' is a 'major code smell'. (I'm guessing that if
> > > I add a javadoc comment that only contains '@deprecated' it will tell
> > > me that empty javadoc is a code smell.)
> > >
> > > And "Do not forget to remove this deprecated code someday." is an
> > > 'info code smell'. Yeah, right. Wait until the next major version.
> > >
> > > I'm trying to work here. Get this *****ing robot off my back.
> > >
> > > Julian
> > >
> > > On Wed, Jan 11, 2023 at 10:41 PM Alessandro Solimando
> > > <al...@gmail.com> wrote:
> > > >
> > > > Hi everyone,
> > > > I agree with Julian that we should be open to see what value Sonar
> > brings
> > > > with the current setup, but for a true accounting we need many more
> > data
> > > > points, two examples are just not enough.
> > > >
> > > > In my experience I see reviewers asking contributors to fix real
> issues
> > > > that Sonarlint plugin can highlight in IntelliJ even locally.
> > > >
> > > > So Sonar would save those reviewers time if the contributor would
> > review
> > > > and fix some of them autonomously.
> > > >
> > > > If we take Sonar as an opportunity to fix easily 80% (à la Pareto) of
> > the
> > > > trivial issues we generally see in contributions rather than
> > considering
> > > > each and every issue as blocking, we can have a positive net IMO.
> > > >
> > > > There are also fine tunings and exclusions to be added over time for
> > > > accepted "issues" (like the test class under src), like Ruben was
> > > proposing.
> > > >
> > > > I was the one who did the Sonar setup in Hive, I had mostly positive
> > > > feedback by contributors who just took Sonar as an opportunity to fix
> > > some
> > > > bugs and improve code, the only difference is that we do not have any
> > > > quality gate there, so the report is never marked as "failed", it's
> at
> > > the
> > > > sole discretion of the contributor+reviewer to take it into account
> or
> > > not.
> > > >
> > > > I personally don't fix all possible warnings/code smells, but most of
> > > them
> > > > yes. Some are just fine as-is to me and they can even be considered
> > false
> > > > positives.
> > > >
> > > > Best regards,
> > > > Alessandro
> > > >
> > > >
> > > > On Wed 11 Jan 2023, 23:17 Julian Hyde, <jh...@gmail.com>
> wrote:
> > > >
> > > > > The instanceof case was complicated. The code that Kevin wrote was
> > > good,
> > > > > and appropriate, and when Sonar blocked it Stamatis was able to
> find
> > an
> > > > > alternative formulation, which existed because, by luck, the JSR
> had
> > > > > deprecated an exception class but not its base class. I spent 30
> > > minutes
> > > > > reviewing the PR and was about to merge it, and because of Sonar’s
> > > bump in
> > > > > the road that time was wasted. I doubt that there has been a single
> > > other
> > > > > occasion when someone wrote
> > > > > “com.example.MyClass”.equals(x.getClass().getName()) instead of “x
> > > > > instanceof MyClass”. So far that particular check has cost us ~1
> hour
> > > and
> > > > > not saved us any time.
> > > > >
> > > > > I’m not saying that Sonar is net bad. I’m just saying let’s do a
> true
> > > > > accounting.
> > > > >
> > > > >
> > > > > > On Jan 11, 2023, at 2:42 AM, Ruben Q L <ru...@gmail.com>
> wrote:
> > > > > >
> > > > > > Hello,
> > > > > >
> > > > > > First of all, thanks Stamatis for implementing this, I think it
> is
> > > > > > something good for the project.
> > > > > > In the beginning things might be a bit complicated (as always)
> and
> > we
> > > > > might
> > > > > > need some adjustments / clarifications, but I hope that in the
> long
> > > run
> > > > > > we'll see this as a useful feature.
> > > > > >
> > > > > > Regarding the two specific issues being discussed:
> > > > > > - If I am not mistaken, the fact that SqlOperatorTest was moved
> out
> > > of
> > > > > > 'test' was a consequence of [1], see the corresponding PR [2]
> "...
> > > it was
> > > > > > necessary to move several classes from the 'core' module to
> > > 'testkit'". I
> > > > > > don't know how simple (or how complex) a potential refactoring to
> > > move it
> > > > > > out of there might be. Alternatively, it seems that this is
> rather
> > an
> > > > > > exceptional case, so perhaps it should qualify for an exception
> > (e.g.
> > > > > > everything under /testkit/* shall not be considered for
> coverage).
> > > > > > - Regarding the instanceof, it seems that it was indeed a valid
> > > warning,
> > > > > > and it has recently been fixed via [3] (see discussion on its PR
> > [4])
> > > > > >
> > > > > > Best regards,
> > > > > > Ruben
> > > > > >
> > > > > > [1] https://issues.apache.org/jira/browse/CALCITE-4885
> > > > > > [2] https://github.com/apache/calcite/pull/2685
> > > > > > [3]
> > > > > >
> > > > >
> > >
> >
> https://github.com/apache/calcite/commit/4bebdb07c2f45a95c9a4fdf81e9bcfbdd11a15de
> > > > > > [4] https://github.com/apache/calcite/pull/2919
> > > > > >
> > > > > >
> > > > > >
> > > > > > On Tue, Jan 10, 2023 at 3:18 PM Stamatis Zampetakis <
> > > zabetak@gmail.com>
> > > > > > wrote:
> > > > > >
> > > > > >> Thanks for the feedback!
> > > > > >>
> > > > > >> I would like to stretch the fact that at this point it is at the
> > > > > discretion
> > > > > >> of the reviewer/committer to enforce or ignore the information
> > > provided
> > > > > by
> > > > > >> Sonar.
> > > > > >>
> > > > > >> Sonar, as other similar systems, has limitations thus there are
> > > always
> > > > > >> going to be false positives. The rules/checks performed are
> fully
> > > > > >> customisable so we can enable/disable them at will.
> > > > > >>
> > > > > >> The two issues highlighted by Julian seem like true positives to
> > me.
> > > > > >> * The "New code" that was introduced recently is not covered
> > > > > sufficiently
> > > > > >> by tests and that's a fact. Part of the problem seems to come
> from
> > > the
> > > > > >> recent modifications in SqlOperatorTest [1]. The class is
> located
> > > under
> > > > > >> src/main (and not under src/test) so it is considered as a
> > > production
> > > > > class
> > > > > >> and coverage checks are applied. There are ways to exclude
> certain
> > > paths
> > > > > >> from coverage but I would argue that the class shouldn't be
> there
> > > in the
> > > > > >> first place; we should probably log a CALCITE ticket for moving
> > out
> > > of
> > > > > >> there.
> > > > > >> * The instance of warning is something that we probably don't
> > > > > want/cannot
> > > > > >> fix (for the reasons mentioned in the PR) but Sonar did well to
> > > bring
> > > > > this
> > > > > >> up; it is problematic to do comparisons by relying on the class
> > > name.
> > > > > >>
> > > > > >> I encourage others to share their thoughts/remarks as well so
> that
> > > we
> > > > > >> improve as much as possible the developer experience.
> > > > > >>
> > > > > >> Best,
> > > > > >> Stamatis
> > > > > >>
> > > > > >> [1]
> > > > > >>
> > > > > >>
> > > > >
> > >
> >
> https://github.com/apache/calcite/blob/be7135cf1fd3d87e4036b2dd6e58d2f1251f8959/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java
> > > > > >>
> > > > > >> On Tue, Jan 10, 2023 at 1:38 AM Julian Hyde <jh...@apache.org>
> > > wrote:
> > > > > >>
> > > > > >>> I see two false positives so far:
> > > > > >>> * The message on be7135cf "58.1% Coverage on New Code (is less
> > than
> > > > > >> 80%)"
> > > > > >>> * The bug on PR 2942 "Use an "instanceof" comparison instead"
> > > > > >>>
> > > > > >>> Has Sonarcube had any true positives yet?
> > > > > >>>
> > > > > >>> Vladimir used to hate when I was skeptical of changes to the
> > build
> > > > > >>> system. But I have no tolerance for a lint system that makes
> > extra
> > > > > >>> work.
> > > > > >>>
> > > > > >>> Julian
> > > > > >>>
> > > > > >>>
> > > > > >>>
> > > > > >>>
> > > > > >>> On Mon, Jan 9, 2023 at 2:46 PM Francis Chuang <
> > > > > francischuang@apache.org>
> > > > > >>> wrote:
> > > > > >>>>
> > > > > >>>> Thanks for implementing this, Stamatis! Having code quality
> > > metrics on
> > > > > >>>> our repos is a huge win.
> > > > > >>>>
> > > > > >>>> Francis
> > > > > >>>>
> > > > > >>>> On 10/01/2023 4:22 am, Stamatis Zampetakis wrote:
> > > > > >>>>> I just merged the CALCITE-5427 [1] enabling Sonarcloud
> analysis
> > > for
> > > > > >>> Calcite
> > > > > >>>>> main branch and new PRs.
> > > > > >>>>>
> > > > > >>>>> I have left the default Sonar quality gates active so you may
> > > start
> > > > > >>> seeing
> > > > > >>>>> Sonar reporting errors in various cases.
> > > > > >>>>>
> > > > > >>>>> If you encounter problems or you would like things to work
> > > > > >> differently
> > > > > >>>>> please create JIRA tickets and ping me if you need help.
> > > > > >>>>>
> > > > > >>>>> Once we are happy with how things work for Calcite we can
> also
> > > port
> > > > > >> the
> > > > > >>>>> changes to Avatica.
> > > > > >>>>>
> > > > > >>>>> Note that all Calcite committers have administrative
> privileges
> > > to
> > > > > >> the
> > > > > >>>>> Calcite project in Sonarcloud [2].
> > > > > >>>>>
> > > > > >>>>> Best,
> > > > > >>>>> Stamatis
> > > > > >>>>>
> > > > > >>>>> [1]
> > > > > >>>>>
> > > > > >>>
> > > > > >>
> > > > >
> > >
> >
> https://github.com/apache/calcite/commit/be7135cf1fd3d87e4036b2dd6e58d2f1251f8959
> > > > > >>>>> [2] https://sonarcloud.io/project/roles?id=apache_calcite
> > > > > >>>>>
> > > > > >>>>> On Wed, Jan 4, 2023 at 5:44 PM Stamatis Zampetakis <
> > > > > >> zabetak@gmail.com>
> > > > > >>>>> wrote:
> > > > > >>>>>
> > > > > >>>>>> The integration is advancing well and I am hoping to merge
> the
> > > PR in
> > > > > >>> the
> > > > > >>>>>> coming days.
> > > > > >>>>>> To avoid unpleasant surprises, I am planning to create a new
> > > remote
> > > > > >>> branch
> > > > > >>>>>> in the main calcite repo to test some things out. I will
> > delete
> > > it
> > > > > >> as
> > > > > >>> soon
> > > > > >>>>>> as I am done with the tests.
> > > > > >>>>>>
> > > > > >>>>>> Best.
> > > > > >>>>>> Stamatis
> > > > > >>>>>>
> > > > > >>>>>> On Wed, Dec 28, 2022 at 2:47 PM Michael Mior <
> > mmior@apache.org>
> > > > > >>> wrote:
> > > > > >>>>>>
> > > > > >>>>>>> Thanks Stamatis! I haven't used SonarCloud before, but in
> > > general I
> > > > > >>> think
> > > > > >>>>>>> such tools can be quite useful.
> > > > > >>>>>>>
> > > > > >>>>>>> --
> > > > > >>>>>>> Michael Mior
> > > > > >>>>>>> mmior@apache.org
> > > > > >>>>>>>
> > > > > >>>>>>>
> > > > > >>>>>>> On Sat, Dec 24, 2022 at 4:01 PM Stamatis Zampetakis <
> > > > > >>> zabetak@gmail.com>
> > > > > >>>>>>> wrote:
> > > > > >>>>>>>
> > > > > >>>>>>>> Since there were no objections, I just logged INFRA-24038
> > [1]
> > > and
> > > > > >>> plan
> > > > > >>>>>>> to
> > > > > >>>>>>>> move this forward.
> > > > > >>>>>>>>
> > > > > >>>>>>>> Let me know if you have questions or concerns regarding
> the
> > > > > >>> adoption of
> > > > > >>>>>>>> SonarCloud.
> > > > > >>>>>>>>
> > > > > >>>>>>>> Best,
> > > > > >>>>>>>> Stamatis
> > > > > >>>>>>>>
> > > > > >>>>>>>> [1] https://issues.apache.org/jira/browse/INFRA-24038
> > > > > >>>>>>>>
> > > > > >>>>>>>> On Sat, Dec 10, 2022 at 11:32 AM Benchao Li <
> > > libenchao@apache.org
> > > > > >>>
> > > > > >>>>>>> wrote:
> > > > > >>>>>>>>
> > > > > >>>>>>>>> Thanks Stamatis for bringing this up.
> > > > > >>>>>>>>>
> > > > > >>>>>>>>> I haven't used Sonar yet, but thanks for the demo[1] you
> > > > > >> provided,
> > > > > >>> it
> > > > > >>>>>>>> looks
> > > > > >>>>>>>>> really interesting. I think it's worth a try for Calcite.
> > > > > >>>>>>>>>
> > > > > >>>>>>>>> [1] https://github.com/zabetak/calcite/pull/9
> > > > > >>>>>>>>>
> > > > > >>>>>>>>> Alessandro Solimando <al...@gmail.com>
> > > > > >>> 于2022年12月10日周六
> > > > > >>>>>>>>> 02:54写道：
> > > > > >>>>>>>>>
> > > > > >>>>>>>>>> Hi all,
> > > > > >>>>>>>>>> thanks Stamatis for the proposal and the work, I am a
> huge
> > > fan
> > > > > >> of
> > > > > >>>>>>> Sonar
> > > > > >>>>>>>>> and
> > > > > >>>>>>>>>> I really miss it on Calcite, so a big +1 from me on
> this.
> > > > > >>>>>>>>>>
> > > > > >>>>>>>>>> In Hive so far we have received good feedback, so I hope
> > it
> > > will
> > > > > >>> be
> > > > > >>>>>>>>>> welcomed here too.
> > > > > >>>>>>>>>>
> > > > > >>>>>>>>>> Best regards,
> > > > > >>>>>>>>>> Alessandro
> > > > > >>>>>>>>>>
> > > > > >>>>>>>>>> On Fri, 9 Dec 2022 at 19:02, Stamatis Zampetakis <
> > > > > >>> zabetak@gmail.com
> > > > > >>>>>>>>
> > > > > >>>>>>>>>> wrote:
> > > > > >>>>>>>>>>
> > > > > >>>>>>>>>>> Hi all,
> > > > > >>>>>>>>>>>
> > > > > >>>>>>>>>>> I just logged CALCITE-5427 [1] for introducing code
> > > quality &
> > > > > >>>>>>>> coverage
> > > > > >>>>>>>>>>> metrics in Calcite CI.
> > > > > >>>>>>>>>>>
> > > > > >>>>>>>>>>> I added some motivation and examples under the ticket
> so
> > > please
> > > > > >>>>>>> have
> > > > > >>>>>>>> a
> > > > > >>>>>>>>>> look
> > > > > >>>>>>>>>>> and let me know what you think.
> > > > > >>>>>>>>>>>
> > > > > >>>>>>>>>>> If there are no objections, I will follow-up with INFRA
> > to
> > > set
> > > > > >>>>>>> things
> > > > > >>>>>>>>> up
> > > > > >>>>>>>>>>> for the official Calcite repo.
> > > > > >>>>>>>>>>>
> > > > > >>>>>>>>>>> The integration with SonarCloud has been inspired by
> > > HIVE-26196
> > > > > >>>>>>> [2]
> > > > > >>>>>>>>> that
> > > > > >>>>>>>>>>> Alessandro put in place for Hive and has been very
> > helpful
> > > so
> > > > > >>> far.
> > > > > >>>>>>>>>>>
> > > > > >>>>>>>>>>> Best,
> > > > > >>>>>>>>>>> Stamatis
> > > > > >>>>>>>>>>>
> > > > > >>>>>>>>>>> [1] https://issues.apache.org/jira/browse/CALCITE-5427
> > > > > >>>>>>>>>>> [2] https://issues.apache.org/jira/browse/HIVE-26196
> > > > > >>>>>>>>>>>
> > > > > >>>>>>>>>>
> > > > > >>>>>>>>>
> > > > > >>>>>>>>>
> > > > > >>>>>>>>> --
> > > > > >>>>>>>>>
> > > > > >>>>>>>>> Best,
> > > > > >>>>>>>>> Benchao Li
> > > > > >>>>>>>>>
> > > > > >>>>>>>>
> > > > > >>>>>>>
> > > > > >>>>>>
> > > > > >>>>>
> > > > > >>>
> > > > > >>
> > > > >
> > > > >
> > >
> >
>


-- 

Best,
Benchao Li

Re: [DISCUSS] Code quality/coverage with SonarCloud & JaCoCo

[Alessandro Solimando <al...@gmail.com>]

Thank you Stamatis for working on this, I see no need to remove the Sonar
analysis now that the quality gate is off.

In the meantime, since all committers have write privilege in Sonar,
whoever is interested can help fine-tuning it to fit our needs (for
instance disabling rules like the code smell for TODOs if unwanted), on a
voluntary basis, of course.

Best regards,
Alessandro

On Fri, 13 Jan 2023 at 11:23, Stamatis Zampetakis <za...@gmail.com> wrote:

> I logged CALCITE-5474 [1] to disable the failures in Sonar checks
> allowing everything to appear as green.
>
> Sonar annotations are still going to appear under the PR after
> CALCITE-5474; if the intention is to remove also these indications let
> me know and I will log another ticket.
>
> Best,
> Stamatis
>
> [1] https://issues.apache.org/jira/browse/CALCITE-5474
>
> On Fri, Jan 13, 2023 at 2:53 AM Julian Hyde <jh...@apache.org> wrote:
>
> > Now a build 'failed' with 7 'code smells'.
> >
> > Duplicating a string literal in a test was deemed as 'critical code
> > smell'. For heaven's sake.
> >
> >
> >
> https://sonarcloud.io/project/issues?resolved=false&severities=CRITICAL&types=CODE_SMELL&pullRequest=2942&id=apache_calcite
> >
> > Adding '@Deprecated' without also adding a javadoc comment that
> > contains '@deprecated' is a 'major code smell'. (I'm guessing that if
> > I add a javadoc comment that only contains '@deprecated' it will tell
> > me that empty javadoc is a code smell.)
> >
> > And "Do not forget to remove this deprecated code someday." is an
> > 'info code smell'. Yeah, right. Wait until the next major version.
> >
> > I'm trying to work here. Get this *****ing robot off my back.
> >
> > Julian
> >
> > On Wed, Jan 11, 2023 at 10:41 PM Alessandro Solimando
> > <al...@gmail.com> wrote:
> > >
> > > Hi everyone,
> > > I agree with Julian that we should be open to see what value Sonar
> brings
> > > with the current setup, but for a true accounting we need many more
> data
> > > points, two examples are just not enough.
> > >
> > > In my experience I see reviewers asking contributors to fix real issues
> > > that Sonarlint plugin can highlight in IntelliJ even locally.
> > >
> > > So Sonar would save those reviewers time if the contributor would
> review
> > > and fix some of them autonomously.
> > >
> > > If we take Sonar as an opportunity to fix easily 80% (à la Pareto) of
> the
> > > trivial issues we generally see in contributions rather than
> considering
> > > each and every issue as blocking, we can have a positive net IMO.
> > >
> > > There are also fine tunings and exclusions to be added over time for
> > > accepted "issues" (like the test class under src), like Ruben was
> > proposing.
> > >
> > > I was the one who did the Sonar setup in Hive, I had mostly positive
> > > feedback by contributors who just took Sonar as an opportunity to fix
> > some
> > > bugs and improve code, the only difference is that we do not have any
> > > quality gate there, so the report is never marked as "failed", it's at
> > the
> > > sole discretion of the contributor+reviewer to take it into account or
> > not.
> > >
> > > I personally don't fix all possible warnings/code smells, but most of
> > them
> > > yes. Some are just fine as-is to me and they can even be considered
> false
> > > positives.
> > >
> > > Best regards,
> > > Alessandro
> > >
> > >
> > > On Wed 11 Jan 2023, 23:17 Julian Hyde, <jh...@gmail.com> wrote:
> > >
> > > > The instanceof case was complicated. The code that Kevin wrote was
> > good,
> > > > and appropriate, and when Sonar blocked it Stamatis was able to find
> an
> > > > alternative formulation, which existed because, by luck, the JSR had
> > > > deprecated an exception class but not its base class. I spent 30
> > minutes
> > > > reviewing the PR and was about to merge it, and because of Sonar’s
> > bump in
> > > > the road that time was wasted. I doubt that there has been a single
> > other
> > > > occasion when someone wrote
> > > > “com.example.MyClass”.equals(x.getClass().getName()) instead of “x
> > > > instanceof MyClass”. So far that particular check has cost us ~1 hour
> > and
> > > > not saved us any time.
> > > >
> > > > I’m not saying that Sonar is net bad. I’m just saying let’s do a true
> > > > accounting.
> > > >
> > > >
> > > > > On Jan 11, 2023, at 2:42 AM, Ruben Q L <ru...@gmail.com> wrote:
> > > > >
> > > > > Hello,
> > > > >
> > > > > First of all, thanks Stamatis for implementing this, I think it is
> > > > > something good for the project.
> > > > > In the beginning things might be a bit complicated (as always) and
> we
> > > > might
> > > > > need some adjustments / clarifications, but I hope that in the long
> > run
> > > > > we'll see this as a useful feature.
> > > > >
> > > > > Regarding the two specific issues being discussed:
> > > > > - If I am not mistaken, the fact that SqlOperatorTest was moved out
> > of
> > > > > 'test' was a consequence of [1], see the corresponding PR [2] "...
> > it was
> > > > > necessary to move several classes from the 'core' module to
> > 'testkit'". I
> > > > > don't know how simple (or how complex) a potential refactoring to
> > move it
> > > > > out of there might be. Alternatively, it seems that this is rather
> an
> > > > > exceptional case, so perhaps it should qualify for an exception
> (e.g.
> > > > > everything under /testkit/* shall not be considered for coverage).
> > > > > - Regarding the instanceof, it seems that it was indeed a valid
> > warning,
> > > > > and it has recently been fixed via [3] (see discussion on its PR
> [4])
> > > > >
> > > > > Best regards,
> > > > > Ruben
> > > > >
> > > > > [1] https://issues.apache.org/jira/browse/CALCITE-4885
> > > > > [2] https://github.com/apache/calcite/pull/2685
> > > > > [3]
> > > > >
> > > >
> >
> https://github.com/apache/calcite/commit/4bebdb07c2f45a95c9a4fdf81e9bcfbdd11a15de
> > > > > [4] https://github.com/apache/calcite/pull/2919
> > > > >
> > > > >
> > > > >
> > > > > On Tue, Jan 10, 2023 at 3:18 PM Stamatis Zampetakis <
> > zabetak@gmail.com>
> > > > > wrote:
> > > > >
> > > > >> Thanks for the feedback!
> > > > >>
> > > > >> I would like to stretch the fact that at this point it is at the
> > > > discretion
> > > > >> of the reviewer/committer to enforce or ignore the information
> > provided
> > > > by
> > > > >> Sonar.
> > > > >>
> > > > >> Sonar, as other similar systems, has limitations thus there are
> > always
> > > > >> going to be false positives. The rules/checks performed are fully
> > > > >> customisable so we can enable/disable them at will.
> > > > >>
> > > > >> The two issues highlighted by Julian seem like true positives to
> me.
> > > > >> * The "New code" that was introduced recently is not covered
> > > > sufficiently
> > > > >> by tests and that's a fact. Part of the problem seems to come from
> > the
> > > > >> recent modifications in SqlOperatorTest [1]. The class is located
> > under
> > > > >> src/main (and not under src/test) so it is considered as a
> > production
> > > > class
> > > > >> and coverage checks are applied. There are ways to exclude certain
> > paths
> > > > >> from coverage but I would argue that the class shouldn't be there
> > in the
> > > > >> first place; we should probably log a CALCITE ticket for moving
> out
> > of
> > > > >> there.
> > > > >> * The instance of warning is something that we probably don't
> > > > want/cannot
> > > > >> fix (for the reasons mentioned in the PR) but Sonar did well to
> > bring
> > > > this
> > > > >> up; it is problematic to do comparisons by relying on the class
> > name.
> > > > >>
> > > > >> I encourage others to share their thoughts/remarks as well so that
> > we
> > > > >> improve as much as possible the developer experience.
> > > > >>
> > > > >> Best,
> > > > >> Stamatis
> > > > >>
> > > > >> [1]
> > > > >>
> > > > >>
> > > >
> >
> https://github.com/apache/calcite/blob/be7135cf1fd3d87e4036b2dd6e58d2f1251f8959/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java
> > > > >>
> > > > >> On Tue, Jan 10, 2023 at 1:38 AM Julian Hyde <jh...@apache.org>
> > wrote:
> > > > >>
> > > > >>> I see two false positives so far:
> > > > >>> * The message on be7135cf "58.1% Coverage on New Code (is less
> than
> > > > >> 80%)"
> > > > >>> * The bug on PR 2942 "Use an "instanceof" comparison instead"
> > > > >>>
> > > > >>> Has Sonarcube had any true positives yet?
> > > > >>>
> > > > >>> Vladimir used to hate when I was skeptical of changes to the
> build
> > > > >>> system. But I have no tolerance for a lint system that makes
> extra
> > > > >>> work.
> > > > >>>
> > > > >>> Julian
> > > > >>>
> > > > >>>
> > > > >>>
> > > > >>>
> > > > >>> On Mon, Jan 9, 2023 at 2:46 PM Francis Chuang <
> > > > francischuang@apache.org>
> > > > >>> wrote:
> > > > >>>>
> > > > >>>> Thanks for implementing this, Stamatis! Having code quality
> > metrics on
> > > > >>>> our repos is a huge win.
> > > > >>>>
> > > > >>>> Francis
> > > > >>>>
> > > > >>>> On 10/01/2023 4:22 am, Stamatis Zampetakis wrote:
> > > > >>>>> I just merged the CALCITE-5427 [1] enabling Sonarcloud analysis
> > for
> > > > >>> Calcite
> > > > >>>>> main branch and new PRs.
> > > > >>>>>
> > > > >>>>> I have left the default Sonar quality gates active so you may
> > start
> > > > >>> seeing
> > > > >>>>> Sonar reporting errors in various cases.
> > > > >>>>>
> > > > >>>>> If you encounter problems or you would like things to work
> > > > >> differently
> > > > >>>>> please create JIRA tickets and ping me if you need help.
> > > > >>>>>
> > > > >>>>> Once we are happy with how things work for Calcite we can also
> > port
> > > > >> the
> > > > >>>>> changes to Avatica.
> > > > >>>>>
> > > > >>>>> Note that all Calcite committers have administrative privileges
> > to
> > > > >> the
> > > > >>>>> Calcite project in Sonarcloud [2].
> > > > >>>>>
> > > > >>>>> Best,
> > > > >>>>> Stamatis
> > > > >>>>>
> > > > >>>>> [1]
> > > > >>>>>
> > > > >>>
> > > > >>
> > > >
> >
> https://github.com/apache/calcite/commit/be7135cf1fd3d87e4036b2dd6e58d2f1251f8959
> > > > >>>>> [2] https://sonarcloud.io/project/roles?id=apache_calcite
> > > > >>>>>
> > > > >>>>> On Wed, Jan 4, 2023 at 5:44 PM Stamatis Zampetakis <
> > > > >> zabetak@gmail.com>
> > > > >>>>> wrote:
> > > > >>>>>
> > > > >>>>>> The integration is advancing well and I am hoping to merge the
> > PR in
> > > > >>> the
> > > > >>>>>> coming days.
> > > > >>>>>> To avoid unpleasant surprises, I am planning to create a new
> > remote
> > > > >>> branch
> > > > >>>>>> in the main calcite repo to test some things out. I will
> delete
> > it
> > > > >> as
> > > > >>> soon
> > > > >>>>>> as I am done with the tests.
> > > > >>>>>>
> > > > >>>>>> Best.
> > > > >>>>>> Stamatis
> > > > >>>>>>
> > > > >>>>>> On Wed, Dec 28, 2022 at 2:47 PM Michael Mior <
> mmior@apache.org>
> > > > >>> wrote:
> > > > >>>>>>
> > > > >>>>>>> Thanks Stamatis! I haven't used SonarCloud before, but in
> > general I
> > > > >>> think
> > > > >>>>>>> such tools can be quite useful.
> > > > >>>>>>>
> > > > >>>>>>> --
> > > > >>>>>>> Michael Mior
> > > > >>>>>>> mmior@apache.org
> > > > >>>>>>>
> > > > >>>>>>>
> > > > >>>>>>> On Sat, Dec 24, 2022 at 4:01 PM Stamatis Zampetakis <
> > > > >>> zabetak@gmail.com>
> > > > >>>>>>> wrote:
> > > > >>>>>>>
> > > > >>>>>>>> Since there were no objections, I just logged INFRA-24038
> [1]
> > and
> > > > >>> plan
> > > > >>>>>>> to
> > > > >>>>>>>> move this forward.
> > > > >>>>>>>>
> > > > >>>>>>>> Let me know if you have questions or concerns regarding the
> > > > >>> adoption of
> > > > >>>>>>>> SonarCloud.
> > > > >>>>>>>>
> > > > >>>>>>>> Best,
> > > > >>>>>>>> Stamatis
> > > > >>>>>>>>
> > > > >>>>>>>> [1] https://issues.apache.org/jira/browse/INFRA-24038
> > > > >>>>>>>>
> > > > >>>>>>>> On Sat, Dec 10, 2022 at 11:32 AM Benchao Li <
> > libenchao@apache.org
> > > > >>>
> > > > >>>>>>> wrote:
> > > > >>>>>>>>
> > > > >>>>>>>>> Thanks Stamatis for bringing this up.
> > > > >>>>>>>>>
> > > > >>>>>>>>> I haven't used Sonar yet, but thanks for the demo[1] you
> > > > >> provided,
> > > > >>> it
> > > > >>>>>>>> looks
> > > > >>>>>>>>> really interesting. I think it's worth a try for Calcite.
> > > > >>>>>>>>>
> > > > >>>>>>>>> [1] https://github.com/zabetak/calcite/pull/9
> > > > >>>>>>>>>
> > > > >>>>>>>>> Alessandro Solimando <al...@gmail.com>
> > > > >>> 于2022年12月10日周六
> > > > >>>>>>>>> 02:54写道：
> > > > >>>>>>>>>
> > > > >>>>>>>>>> Hi all,
> > > > >>>>>>>>>> thanks Stamatis for the proposal and the work, I am a huge
> > fan
> > > > >> of
> > > > >>>>>>> Sonar
> > > > >>>>>>>>> and
> > > > >>>>>>>>>> I really miss it on Calcite, so a big +1 from me on this.
> > > > >>>>>>>>>>
> > > > >>>>>>>>>> In Hive so far we have received good feedback, so I hope
> it
> > will
> > > > >>> be
> > > > >>>>>>>>>> welcomed here too.
> > > > >>>>>>>>>>
> > > > >>>>>>>>>> Best regards,
> > > > >>>>>>>>>> Alessandro
> > > > >>>>>>>>>>
> > > > >>>>>>>>>> On Fri, 9 Dec 2022 at 19:02, Stamatis Zampetakis <
> > > > >>> zabetak@gmail.com
> > > > >>>>>>>>
> > > > >>>>>>>>>> wrote:
> > > > >>>>>>>>>>
> > > > >>>>>>>>>>> Hi all,
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>> I just logged CALCITE-5427 [1] for introducing code
> > quality &
> > > > >>>>>>>> coverage
> > > > >>>>>>>>>>> metrics in Calcite CI.
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>> I added some motivation and examples under the ticket so
> > please
> > > > >>>>>>> have
> > > > >>>>>>>> a
> > > > >>>>>>>>>> look
> > > > >>>>>>>>>>> and let me know what you think.
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>> If there are no objections, I will follow-up with INFRA
> to
> > set
> > > > >>>>>>> things
> > > > >>>>>>>>> up
> > > > >>>>>>>>>>> for the official Calcite repo.
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>> The integration with SonarCloud has been inspired by
> > HIVE-26196
> > > > >>>>>>> [2]
> > > > >>>>>>>>> that
> > > > >>>>>>>>>>> Alessandro put in place for Hive and has been very
> helpful
> > so
> > > > >>> far.
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>> Best,
> > > > >>>>>>>>>>> Stamatis
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>> [1] https://issues.apache.org/jira/browse/CALCITE-5427
> > > > >>>>>>>>>>> [2] https://issues.apache.org/jira/browse/HIVE-26196
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>
> > > > >>>>>>>>>
> > > > >>>>>>>>>
> > > > >>>>>>>>> --
> > > > >>>>>>>>>
> > > > >>>>>>>>> Best,
> > > > >>>>>>>>> Benchao Li
> > > > >>>>>>>>>
> > > > >>>>>>>>
> > > > >>>>>>>
> > > > >>>>>>
> > > > >>>>>
> > > > >>>
> > > > >>
> > > >
> > > >
> >
>

Re: [DISCUSS] Code quality/coverage with SonarCloud & JaCoCo

[Stamatis Zampetakis <za...@gmail.com>]

I logged CALCITE-5474 [1] to disable the failures in Sonar checks
allowing everything to appear as green.

Sonar annotations are still going to appear under the PR after
CALCITE-5474; if the intention is to remove also these indications let
me know and I will log another ticket.

Best,
Stamatis

[1] https://issues.apache.org/jira/browse/CALCITE-5474

On Fri, Jan 13, 2023 at 2:53 AM Julian Hyde <jh...@apache.org> wrote:

> Now a build 'failed' with 7 'code smells'.
>
> Duplicating a string literal in a test was deemed as 'critical code
> smell'. For heaven's sake.
>
>
> https://sonarcloud.io/project/issues?resolved=false&severities=CRITICAL&types=CODE_SMELL&pullRequest=2942&id=apache_calcite
>
> Adding '@Deprecated' without also adding a javadoc comment that
> contains '@deprecated' is a 'major code smell'. (I'm guessing that if
> I add a javadoc comment that only contains '@deprecated' it will tell
> me that empty javadoc is a code smell.)
>
> And "Do not forget to remove this deprecated code someday." is an
> 'info code smell'. Yeah, right. Wait until the next major version.
>
> I'm trying to work here. Get this *****ing robot off my back.
>
> Julian
>
> On Wed, Jan 11, 2023 at 10:41 PM Alessandro Solimando
> <al...@gmail.com> wrote:
> >
> > Hi everyone,
> > I agree with Julian that we should be open to see what value Sonar brings
> > with the current setup, but for a true accounting we need many more data
> > points, two examples are just not enough.
> >
> > In my experience I see reviewers asking contributors to fix real issues
> > that Sonarlint plugin can highlight in IntelliJ even locally.
> >
> > So Sonar would save those reviewers time if the contributor would review
> > and fix some of them autonomously.
> >
> > If we take Sonar as an opportunity to fix easily 80% (à la Pareto) of the
> > trivial issues we generally see in contributions rather than considering
> > each and every issue as blocking, we can have a positive net IMO.
> >
> > There are also fine tunings and exclusions to be added over time for
> > accepted "issues" (like the test class under src), like Ruben was
> proposing.
> >
> > I was the one who did the Sonar setup in Hive, I had mostly positive
> > feedback by contributors who just took Sonar as an opportunity to fix
> some
> > bugs and improve code, the only difference is that we do not have any
> > quality gate there, so the report is never marked as "failed", it's at
> the
> > sole discretion of the contributor+reviewer to take it into account or
> not.
> >
> > I personally don't fix all possible warnings/code smells, but most of
> them
> > yes. Some are just fine as-is to me and they can even be considered false
> > positives.
> >
> > Best regards,
> > Alessandro
> >
> >
> > On Wed 11 Jan 2023, 23:17 Julian Hyde, <jh...@gmail.com> wrote:
> >
> > > The instanceof case was complicated. The code that Kevin wrote was
> good,
> > > and appropriate, and when Sonar blocked it Stamatis was able to find an
> > > alternative formulation, which existed because, by luck, the JSR had
> > > deprecated an exception class but not its base class. I spent 30
> minutes
> > > reviewing the PR and was about to merge it, and because of Sonar’s
> bump in
> > > the road that time was wasted. I doubt that there has been a single
> other
> > > occasion when someone wrote
> > > “com.example.MyClass”.equals(x.getClass().getName()) instead of “x
> > > instanceof MyClass”. So far that particular check has cost us ~1 hour
> and
> > > not saved us any time.
> > >
> > > I’m not saying that Sonar is net bad. I’m just saying let’s do a true
> > > accounting.
> > >
> > >
> > > > On Jan 11, 2023, at 2:42 AM, Ruben Q L <ru...@gmail.com> wrote:
> > > >
> > > > Hello,
> > > >
> > > > First of all, thanks Stamatis for implementing this, I think it is
> > > > something good for the project.
> > > > In the beginning things might be a bit complicated (as always) and we
> > > might
> > > > need some adjustments / clarifications, but I hope that in the long
> run
> > > > we'll see this as a useful feature.
> > > >
> > > > Regarding the two specific issues being discussed:
> > > > - If I am not mistaken, the fact that SqlOperatorTest was moved out
> of
> > > > 'test' was a consequence of [1], see the corresponding PR [2] "...
> it was
> > > > necessary to move several classes from the 'core' module to
> 'testkit'". I
> > > > don't know how simple (or how complex) a potential refactoring to
> move it
> > > > out of there might be. Alternatively, it seems that this is rather an
> > > > exceptional case, so perhaps it should qualify for an exception (e.g.
> > > > everything under /testkit/* shall not be considered for coverage).
> > > > - Regarding the instanceof, it seems that it was indeed a valid
> warning,
> > > > and it has recently been fixed via [3] (see discussion on its PR [4])
> > > >
> > > > Best regards,
> > > > Ruben
> > > >
> > > > [1] https://issues.apache.org/jira/browse/CALCITE-4885
> > > > [2] https://github.com/apache/calcite/pull/2685
> > > > [3]
> > > >
> > >
> https://github.com/apache/calcite/commit/4bebdb07c2f45a95c9a4fdf81e9bcfbdd11a15de
> > > > [4] https://github.com/apache/calcite/pull/2919
> > > >
> > > >
> > > >
> > > > On Tue, Jan 10, 2023 at 3:18 PM Stamatis Zampetakis <
> zabetak@gmail.com>
> > > > wrote:
> > > >
> > > >> Thanks for the feedback!
> > > >>
> > > >> I would like to stretch the fact that at this point it is at the
> > > discretion
> > > >> of the reviewer/committer to enforce or ignore the information
> provided
> > > by
> > > >> Sonar.
> > > >>
> > > >> Sonar, as other similar systems, has limitations thus there are
> always
> > > >> going to be false positives. The rules/checks performed are fully
> > > >> customisable so we can enable/disable them at will.
> > > >>
> > > >> The two issues highlighted by Julian seem like true positives to me.
> > > >> * The "New code" that was introduced recently is not covered
> > > sufficiently
> > > >> by tests and that's a fact. Part of the problem seems to come from
> the
> > > >> recent modifications in SqlOperatorTest [1]. The class is located
> under
> > > >> src/main (and not under src/test) so it is considered as a
> production
> > > class
> > > >> and coverage checks are applied. There are ways to exclude certain
> paths
> > > >> from coverage but I would argue that the class shouldn't be there
> in the
> > > >> first place; we should probably log a CALCITE ticket for moving out
> of
> > > >> there.
> > > >> * The instance of warning is something that we probably don't
> > > want/cannot
> > > >> fix (for the reasons mentioned in the PR) but Sonar did well to
> bring
> > > this
> > > >> up; it is problematic to do comparisons by relying on the class
> name.
> > > >>
> > > >> I encourage others to share their thoughts/remarks as well so that
> we
> > > >> improve as much as possible the developer experience.
> > > >>
> > > >> Best,
> > > >> Stamatis
> > > >>
> > > >> [1]
> > > >>
> > > >>
> > >
> https://github.com/apache/calcite/blob/be7135cf1fd3d87e4036b2dd6e58d2f1251f8959/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java
> > > >>
> > > >> On Tue, Jan 10, 2023 at 1:38 AM Julian Hyde <jh...@apache.org>
> wrote:
> > > >>
> > > >>> I see two false positives so far:
> > > >>> * The message on be7135cf "58.1% Coverage on New Code (is less than
> > > >> 80%)"
> > > >>> * The bug on PR 2942 "Use an "instanceof" comparison instead"
> > > >>>
> > > >>> Has Sonarcube had any true positives yet?
> > > >>>
> > > >>> Vladimir used to hate when I was skeptical of changes to the build
> > > >>> system. But I have no tolerance for a lint system that makes extra
> > > >>> work.
> > > >>>
> > > >>> Julian
> > > >>>
> > > >>>
> > > >>>
> > > >>>
> > > >>> On Mon, Jan 9, 2023 at 2:46 PM Francis Chuang <
> > > francischuang@apache.org>
> > > >>> wrote:
> > > >>>>
> > > >>>> Thanks for implementing this, Stamatis! Having code quality
> metrics on
> > > >>>> our repos is a huge win.
> > > >>>>
> > > >>>> Francis
> > > >>>>
> > > >>>> On 10/01/2023 4:22 am, Stamatis Zampetakis wrote:
> > > >>>>> I just merged the CALCITE-5427 [1] enabling Sonarcloud analysis
> for
> > > >>> Calcite
> > > >>>>> main branch and new PRs.
> > > >>>>>
> > > >>>>> I have left the default Sonar quality gates active so you may
> start
> > > >>> seeing
> > > >>>>> Sonar reporting errors in various cases.
> > > >>>>>
> > > >>>>> If you encounter problems or you would like things to work
> > > >> differently
> > > >>>>> please create JIRA tickets and ping me if you need help.
> > > >>>>>
> > > >>>>> Once we are happy with how things work for Calcite we can also
> port
> > > >> the
> > > >>>>> changes to Avatica.
> > > >>>>>
> > > >>>>> Note that all Calcite committers have administrative privileges
> to
> > > >> the
> > > >>>>> Calcite project in Sonarcloud [2].
> > > >>>>>
> > > >>>>> Best,
> > > >>>>> Stamatis
> > > >>>>>
> > > >>>>> [1]
> > > >>>>>
> > > >>>
> > > >>
> > >
> https://github.com/apache/calcite/commit/be7135cf1fd3d87e4036b2dd6e58d2f1251f8959
> > > >>>>> [2] https://sonarcloud.io/project/roles?id=apache_calcite
> > > >>>>>
> > > >>>>> On Wed, Jan 4, 2023 at 5:44 PM Stamatis Zampetakis <
> > > >> zabetak@gmail.com>
> > > >>>>> wrote:
> > > >>>>>
> > > >>>>>> The integration is advancing well and I am hoping to merge the
> PR in
> > > >>> the
> > > >>>>>> coming days.
> > > >>>>>> To avoid unpleasant surprises, I am planning to create a new
> remote
> > > >>> branch
> > > >>>>>> in the main calcite repo to test some things out. I will delete
> it
> > > >> as
> > > >>> soon
> > > >>>>>> as I am done with the tests.
> > > >>>>>>
> > > >>>>>> Best.
> > > >>>>>> Stamatis
> > > >>>>>>
> > > >>>>>> On Wed, Dec 28, 2022 at 2:47 PM Michael Mior <mm...@apache.org>
> > > >>> wrote:
> > > >>>>>>
> > > >>>>>>> Thanks Stamatis! I haven't used SonarCloud before, but in
> general I
> > > >>> think
> > > >>>>>>> such tools can be quite useful.
> > > >>>>>>>
> > > >>>>>>> --
> > > >>>>>>> Michael Mior
> > > >>>>>>> mmior@apache.org
> > > >>>>>>>
> > > >>>>>>>
> > > >>>>>>> On Sat, Dec 24, 2022 at 4:01 PM Stamatis Zampetakis <
> > > >>> zabetak@gmail.com>
> > > >>>>>>> wrote:
> > > >>>>>>>
> > > >>>>>>>> Since there were no objections, I just logged INFRA-24038 [1]
> and
> > > >>> plan
> > > >>>>>>> to
> > > >>>>>>>> move this forward.
> > > >>>>>>>>
> > > >>>>>>>> Let me know if you have questions or concerns regarding the
> > > >>> adoption of
> > > >>>>>>>> SonarCloud.
> > > >>>>>>>>
> > > >>>>>>>> Best,
> > > >>>>>>>> Stamatis
> > > >>>>>>>>
> > > >>>>>>>> [1] https://issues.apache.org/jira/browse/INFRA-24038
> > > >>>>>>>>
> > > >>>>>>>> On Sat, Dec 10, 2022 at 11:32 AM Benchao Li <
> libenchao@apache.org
> > > >>>
> > > >>>>>>> wrote:
> > > >>>>>>>>
> > > >>>>>>>>> Thanks Stamatis for bringing this up.
> > > >>>>>>>>>
> > > >>>>>>>>> I haven't used Sonar yet, but thanks for the demo[1] you
> > > >> provided,
> > > >>> it
> > > >>>>>>>> looks
> > > >>>>>>>>> really interesting. I think it's worth a try for Calcite.
> > > >>>>>>>>>
> > > >>>>>>>>> [1] https://github.com/zabetak/calcite/pull/9
> > > >>>>>>>>>
> > > >>>>>>>>> Alessandro Solimando <al...@gmail.com>
> > > >>> 于2022年12月10日周六
> > > >>>>>>>>> 02:54写道：
> > > >>>>>>>>>
> > > >>>>>>>>>> Hi all,
> > > >>>>>>>>>> thanks Stamatis for the proposal and the work, I am a huge
> fan
> > > >> of
> > > >>>>>>> Sonar
> > > >>>>>>>>> and
> > > >>>>>>>>>> I really miss it on Calcite, so a big +1 from me on this.
> > > >>>>>>>>>>
> > > >>>>>>>>>> In Hive so far we have received good feedback, so I hope it
> will
> > > >>> be
> > > >>>>>>>>>> welcomed here too.
> > > >>>>>>>>>>
> > > >>>>>>>>>> Best regards,
> > > >>>>>>>>>> Alessandro
> > > >>>>>>>>>>
> > > >>>>>>>>>> On Fri, 9 Dec 2022 at 19:02, Stamatis Zampetakis <
> > > >>> zabetak@gmail.com
> > > >>>>>>>>
> > > >>>>>>>>>> wrote:
> > > >>>>>>>>>>
> > > >>>>>>>>>>> Hi all,
> > > >>>>>>>>>>>
> > > >>>>>>>>>>> I just logged CALCITE-5427 [1] for introducing code
> quality &
> > > >>>>>>>> coverage
> > > >>>>>>>>>>> metrics in Calcite CI.
> > > >>>>>>>>>>>
> > > >>>>>>>>>>> I added some motivation and examples under the ticket so
> please
> > > >>>>>>> have
> > > >>>>>>>> a
> > > >>>>>>>>>> look
> > > >>>>>>>>>>> and let me know what you think.
> > > >>>>>>>>>>>
> > > >>>>>>>>>>> If there are no objections, I will follow-up with INFRA to
> set
> > > >>>>>>> things
> > > >>>>>>>>> up
> > > >>>>>>>>>>> for the official Calcite repo.
> > > >>>>>>>>>>>
> > > >>>>>>>>>>> The integration with SonarCloud has been inspired by
> HIVE-26196
> > > >>>>>>> [2]
> > > >>>>>>>>> that
> > > >>>>>>>>>>> Alessandro put in place for Hive and has been very helpful
> so
> > > >>> far.
> > > >>>>>>>>>>>
> > > >>>>>>>>>>> Best,
> > > >>>>>>>>>>> Stamatis
> > > >>>>>>>>>>>
> > > >>>>>>>>>>> [1] https://issues.apache.org/jira/browse/CALCITE-5427
> > > >>>>>>>>>>> [2] https://issues.apache.org/jira/browse/HIVE-26196
> > > >>>>>>>>>>>
> > > >>>>>>>>>>
> > > >>>>>>>>>
> > > >>>>>>>>>
> > > >>>>>>>>> --
> > > >>>>>>>>>
> > > >>>>>>>>> Best,
> > > >>>>>>>>> Benchao Li
> > > >>>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>
> > > >>>>>>
> > > >>>>>
> > > >>>
> > > >>
> > >
> > >
>

Re: [DISCUSS] Code quality/coverage with SonarCloud & JaCoCo

[Julian Hyde <jh...@apache.org>]

Now a build 'failed' with 7 'code smells'.

Duplicating a string literal in a test was deemed as 'critical code
smell'. For heaven's sake.

  https://sonarcloud.io/project/issues?resolved=false&severities=CRITICAL&types=CODE_SMELL&pullRequest=2942&id=apache_calcite

Adding '@Deprecated' without also adding a javadoc comment that
contains '@deprecated' is a 'major code smell'. (I'm guessing that if
I add a javadoc comment that only contains '@deprecated' it will tell
me that empty javadoc is a code smell.)

And "Do not forget to remove this deprecated code someday." is an
'info code smell'. Yeah, right. Wait until the next major version.

I'm trying to work here. Get this *****ing robot off my back.

Julian

On Wed, Jan 11, 2023 at 10:41 PM Alessandro Solimando
<al...@gmail.com> wrote:
>
> Hi everyone,
> I agree with Julian that we should be open to see what value Sonar brings
> with the current setup, but for a true accounting we need many more data
> points, two examples are just not enough.
>
> In my experience I see reviewers asking contributors to fix real issues
> that Sonarlint plugin can highlight in IntelliJ even locally.
>
> So Sonar would save those reviewers time if the contributor would review
> and fix some of them autonomously.
>
> If we take Sonar as an opportunity to fix easily 80% (à la Pareto) of the
> trivial issues we generally see in contributions rather than considering
> each and every issue as blocking, we can have a positive net IMO.
>
> There are also fine tunings and exclusions to be added over time for
> accepted "issues" (like the test class under src), like Ruben was proposing.
>
> I was the one who did the Sonar setup in Hive, I had mostly positive
> feedback by contributors who just took Sonar as an opportunity to fix some
> bugs and improve code, the only difference is that we do not have any
> quality gate there, so the report is never marked as "failed", it's at the
> sole discretion of the contributor+reviewer to take it into account or not.
>
> I personally don't fix all possible warnings/code smells, but most of them
> yes. Some are just fine as-is to me and they can even be considered false
> positives.
>
> Best regards,
> Alessandro
>
>
> On Wed 11 Jan 2023, 23:17 Julian Hyde, <jh...@gmail.com> wrote:
>
> > The instanceof case was complicated. The code that Kevin wrote was good,
> > and appropriate, and when Sonar blocked it Stamatis was able to find an
> > alternative formulation, which existed because, by luck, the JSR had
> > deprecated an exception class but not its base class. I spent 30 minutes
> > reviewing the PR and was about to merge it, and because of Sonar’s bump in
> > the road that time was wasted. I doubt that there has been a single other
> > occasion when someone wrote
> > “com.example.MyClass”.equals(x.getClass().getName()) instead of “x
> > instanceof MyClass”. So far that particular check has cost us ~1 hour and
> > not saved us any time.
> >
> > I’m not saying that Sonar is net bad. I’m just saying let’s do a true
> > accounting.
> >
> >
> > > On Jan 11, 2023, at 2:42 AM, Ruben Q L <ru...@gmail.com> wrote:
> > >
> > > Hello,
> > >
> > > First of all, thanks Stamatis for implementing this, I think it is
> > > something good for the project.
> > > In the beginning things might be a bit complicated (as always) and we
> > might
> > > need some adjustments / clarifications, but I hope that in the long run
> > > we'll see this as a useful feature.
> > >
> > > Regarding the two specific issues being discussed:
> > > - If I am not mistaken, the fact that SqlOperatorTest was moved out of
> > > 'test' was a consequence of [1], see the corresponding PR [2] "... it was
> > > necessary to move several classes from the 'core' module to 'testkit'". I
> > > don't know how simple (or how complex) a potential refactoring to move it
> > > out of there might be. Alternatively, it seems that this is rather an
> > > exceptional case, so perhaps it should qualify for an exception (e.g.
> > > everything under /testkit/* shall not be considered for coverage).
> > > - Regarding the instanceof, it seems that it was indeed a valid warning,
> > > and it has recently been fixed via [3] (see discussion on its PR [4])
> > >
> > > Best regards,
> > > Ruben
> > >
> > > [1] https://issues.apache.org/jira/browse/CALCITE-4885
> > > [2] https://github.com/apache/calcite/pull/2685
> > > [3]
> > >
> > https://github.com/apache/calcite/commit/4bebdb07c2f45a95c9a4fdf81e9bcfbdd11a15de
> > > [4] https://github.com/apache/calcite/pull/2919
> > >
> > >
> > >
> > > On Tue, Jan 10, 2023 at 3:18 PM Stamatis Zampetakis <za...@gmail.com>
> > > wrote:
> > >
> > >> Thanks for the feedback!
> > >>
> > >> I would like to stretch the fact that at this point it is at the
> > discretion
> > >> of the reviewer/committer to enforce or ignore the information provided
> > by
> > >> Sonar.
> > >>
> > >> Sonar, as other similar systems, has limitations thus there are always
> > >> going to be false positives. The rules/checks performed are fully
> > >> customisable so we can enable/disable them at will.
> > >>
> > >> The two issues highlighted by Julian seem like true positives to me.
> > >> * The "New code" that was introduced recently is not covered
> > sufficiently
> > >> by tests and that's a fact. Part of the problem seems to come from the
> > >> recent modifications in SqlOperatorTest [1]. The class is located under
> > >> src/main (and not under src/test) so it is considered as a production
> > class
> > >> and coverage checks are applied. There are ways to exclude certain paths
> > >> from coverage but I would argue that the class shouldn't be there in the
> > >> first place; we should probably log a CALCITE ticket for moving out of
> > >> there.
> > >> * The instance of warning is something that we probably don't
> > want/cannot
> > >> fix (for the reasons mentioned in the PR) but Sonar did well to bring
> > this
> > >> up; it is problematic to do comparisons by relying on the class name.
> > >>
> > >> I encourage others to share their thoughts/remarks as well so that we
> > >> improve as much as possible the developer experience.
> > >>
> > >> Best,
> > >> Stamatis
> > >>
> > >> [1]
> > >>
> > >>
> > https://github.com/apache/calcite/blob/be7135cf1fd3d87e4036b2dd6e58d2f1251f8959/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java
> > >>
> > >> On Tue, Jan 10, 2023 at 1:38 AM Julian Hyde <jh...@apache.org> wrote:
> > >>
> > >>> I see two false positives so far:
> > >>> * The message on be7135cf "58.1% Coverage on New Code (is less than
> > >> 80%)"
> > >>> * The bug on PR 2942 "Use an "instanceof" comparison instead"
> > >>>
> > >>> Has Sonarcube had any true positives yet?
> > >>>
> > >>> Vladimir used to hate when I was skeptical of changes to the build
> > >>> system. But I have no tolerance for a lint system that makes extra
> > >>> work.
> > >>>
> > >>> Julian
> > >>>
> > >>>
> > >>>
> > >>>
> > >>> On Mon, Jan 9, 2023 at 2:46 PM Francis Chuang <
> > francischuang@apache.org>
> > >>> wrote:
> > >>>>
> > >>>> Thanks for implementing this, Stamatis! Having code quality metrics on
> > >>>> our repos is a huge win.
> > >>>>
> > >>>> Francis
> > >>>>
> > >>>> On 10/01/2023 4:22 am, Stamatis Zampetakis wrote:
> > >>>>> I just merged the CALCITE-5427 [1] enabling Sonarcloud analysis for
> > >>> Calcite
> > >>>>> main branch and new PRs.
> > >>>>>
> > >>>>> I have left the default Sonar quality gates active so you may start
> > >>> seeing
> > >>>>> Sonar reporting errors in various cases.
> > >>>>>
> > >>>>> If you encounter problems or you would like things to work
> > >> differently
> > >>>>> please create JIRA tickets and ping me if you need help.
> > >>>>>
> > >>>>> Once we are happy with how things work for Calcite we can also port
> > >> the
> > >>>>> changes to Avatica.
> > >>>>>
> > >>>>> Note that all Calcite committers have administrative privileges to
> > >> the
> > >>>>> Calcite project in Sonarcloud [2].
> > >>>>>
> > >>>>> Best,
> > >>>>> Stamatis
> > >>>>>
> > >>>>> [1]
> > >>>>>
> > >>>
> > >>
> > https://github.com/apache/calcite/commit/be7135cf1fd3d87e4036b2dd6e58d2f1251f8959
> > >>>>> [2] https://sonarcloud.io/project/roles?id=apache_calcite
> > >>>>>
> > >>>>> On Wed, Jan 4, 2023 at 5:44 PM Stamatis Zampetakis <
> > >> zabetak@gmail.com>
> > >>>>> wrote:
> > >>>>>
> > >>>>>> The integration is advancing well and I am hoping to merge the PR in
> > >>> the
> > >>>>>> coming days.
> > >>>>>> To avoid unpleasant surprises, I am planning to create a new remote
> > >>> branch
> > >>>>>> in the main calcite repo to test some things out. I will delete it
> > >> as
> > >>> soon
> > >>>>>> as I am done with the tests.
> > >>>>>>
> > >>>>>> Best.
> > >>>>>> Stamatis
> > >>>>>>
> > >>>>>> On Wed, Dec 28, 2022 at 2:47 PM Michael Mior <mm...@apache.org>
> > >>> wrote:
> > >>>>>>
> > >>>>>>> Thanks Stamatis! I haven't used SonarCloud before, but in general I
> > >>> think
> > >>>>>>> such tools can be quite useful.
> > >>>>>>>
> > >>>>>>> --
> > >>>>>>> Michael Mior
> > >>>>>>> mmior@apache.org
> > >>>>>>>
> > >>>>>>>
> > >>>>>>> On Sat, Dec 24, 2022 at 4:01 PM Stamatis Zampetakis <
> > >>> zabetak@gmail.com>
> > >>>>>>> wrote:
> > >>>>>>>
> > >>>>>>>> Since there were no objections, I just logged INFRA-24038 [1] and
> > >>> plan
> > >>>>>>> to
> > >>>>>>>> move this forward.
> > >>>>>>>>
> > >>>>>>>> Let me know if you have questions or concerns regarding the
> > >>> adoption of
> > >>>>>>>> SonarCloud.
> > >>>>>>>>
> > >>>>>>>> Best,
> > >>>>>>>> Stamatis
> > >>>>>>>>
> > >>>>>>>> [1] https://issues.apache.org/jira/browse/INFRA-24038
> > >>>>>>>>
> > >>>>>>>> On Sat, Dec 10, 2022 at 11:32 AM Benchao Li <libenchao@apache.org
> > >>>
> > >>>>>>> wrote:
> > >>>>>>>>
> > >>>>>>>>> Thanks Stamatis for bringing this up.
> > >>>>>>>>>
> > >>>>>>>>> I haven't used Sonar yet, but thanks for the demo[1] you
> > >> provided,
> > >>> it
> > >>>>>>>> looks
> > >>>>>>>>> really interesting. I think it's worth a try for Calcite.
> > >>>>>>>>>
> > >>>>>>>>> [1] https://github.com/zabetak/calcite/pull/9
> > >>>>>>>>>
> > >>>>>>>>> Alessandro Solimando <al...@gmail.com>
> > >>> 于2022年12月10日周六
> > >>>>>>>>> 02:54写道：
> > >>>>>>>>>
> > >>>>>>>>>> Hi all,
> > >>>>>>>>>> thanks Stamatis for the proposal and the work, I am a huge fan
> > >> of
> > >>>>>>> Sonar
> > >>>>>>>>> and
> > >>>>>>>>>> I really miss it on Calcite, so a big +1 from me on this.
> > >>>>>>>>>>
> > >>>>>>>>>> In Hive so far we have received good feedback, so I hope it will
> > >>> be
> > >>>>>>>>>> welcomed here too.
> > >>>>>>>>>>
> > >>>>>>>>>> Best regards,
> > >>>>>>>>>> Alessandro
> > >>>>>>>>>>
> > >>>>>>>>>> On Fri, 9 Dec 2022 at 19:02, Stamatis Zampetakis <
> > >>> zabetak@gmail.com
> > >>>>>>>>
> > >>>>>>>>>> wrote:
> > >>>>>>>>>>
> > >>>>>>>>>>> Hi all,
> > >>>>>>>>>>>
> > >>>>>>>>>>> I just logged CALCITE-5427 [1] for introducing code quality &
> > >>>>>>>> coverage
> > >>>>>>>>>>> metrics in Calcite CI.
> > >>>>>>>>>>>
> > >>>>>>>>>>> I added some motivation and examples under the ticket so please
> > >>>>>>> have
> > >>>>>>>> a
> > >>>>>>>>>> look
> > >>>>>>>>>>> and let me know what you think.
> > >>>>>>>>>>>
> > >>>>>>>>>>> If there are no objections, I will follow-up with INFRA to set
> > >>>>>>> things
> > >>>>>>>>> up
> > >>>>>>>>>>> for the official Calcite repo.
> > >>>>>>>>>>>
> > >>>>>>>>>>> The integration with SonarCloud has been inspired by HIVE-26196
> > >>>>>>> [2]
> > >>>>>>>>> that
> > >>>>>>>>>>> Alessandro put in place for Hive and has been very helpful so
> > >>> far.
> > >>>>>>>>>>>
> > >>>>>>>>>>> Best,
> > >>>>>>>>>>> Stamatis
> > >>>>>>>>>>>
> > >>>>>>>>>>> [1] https://issues.apache.org/jira/browse/CALCITE-5427
> > >>>>>>>>>>> [2] https://issues.apache.org/jira/browse/HIVE-26196
> > >>>>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>>
> > >>>>>>>>>
> > >>>>>>>>> --
> > >>>>>>>>>
> > >>>>>>>>> Best,
> > >>>>>>>>> Benchao Li
> > >>>>>>>>>
> > >>>>>>>>
> > >>>>>>>
> > >>>>>>
> > >>>>>
> > >>>
> > >>
> >
> >

Re: [DISCUSS] Code quality/coverage with SonarCloud & JaCoCo

[Alessandro Solimando <al...@gmail.com>]

Hi everyone,
I agree with Julian that we should be open to see what value Sonar brings
with the current setup, but for a true accounting we need many more data
points, two examples are just not enough.

In my experience I see reviewers asking contributors to fix real issues
that Sonarlint plugin can highlight in IntelliJ even locally.

So Sonar would save those reviewers time if the contributor would review
and fix some of them autonomously.

If we take Sonar as an opportunity to fix easily 80% (à la Pareto) of the
trivial issues we generally see in contributions rather than considering
each and every issue as blocking, we can have a positive net IMO.

There are also fine tunings and exclusions to be added over time for
accepted "issues" (like the test class under src), like Ruben was proposing.

I was the one who did the Sonar setup in Hive, I had mostly positive
feedback by contributors who just took Sonar as an opportunity to fix some
bugs and improve code, the only difference is that we do not have any
quality gate there, so the report is never marked as "failed", it's at the
sole discretion of the contributor+reviewer to take it into account or not.

I personally don't fix all possible warnings/code smells, but most of them
yes. Some are just fine as-is to me and they can even be considered false
positives.

Best regards,
Alessandro


On Wed 11 Jan 2023, 23:17 Julian Hyde, <jh...@gmail.com> wrote:

> The instanceof case was complicated. The code that Kevin wrote was good,
> and appropriate, and when Sonar blocked it Stamatis was able to find an
> alternative formulation, which existed because, by luck, the JSR had
> deprecated an exception class but not its base class. I spent 30 minutes
> reviewing the PR and was about to merge it, and because of Sonar’s bump in
> the road that time was wasted. I doubt that there has been a single other
> occasion when someone wrote
> “com.example.MyClass”.equals(x.getClass().getName()) instead of “x
> instanceof MyClass”. So far that particular check has cost us ~1 hour and
> not saved us any time.
>
> I’m not saying that Sonar is net bad. I’m just saying let’s do a true
> accounting.
>
>
> > On Jan 11, 2023, at 2:42 AM, Ruben Q L <ru...@gmail.com> wrote:
> >
> > Hello,
> >
> > First of all, thanks Stamatis for implementing this, I think it is
> > something good for the project.
> > In the beginning things might be a bit complicated (as always) and we
> might
> > need some adjustments / clarifications, but I hope that in the long run
> > we'll see this as a useful feature.
> >
> > Regarding the two specific issues being discussed:
> > - If I am not mistaken, the fact that SqlOperatorTest was moved out of
> > 'test' was a consequence of [1], see the corresponding PR [2] "... it was
> > necessary to move several classes from the 'core' module to 'testkit'". I
> > don't know how simple (or how complex) a potential refactoring to move it
> > out of there might be. Alternatively, it seems that this is rather an
> > exceptional case, so perhaps it should qualify for an exception (e.g.
> > everything under /testkit/* shall not be considered for coverage).
> > - Regarding the instanceof, it seems that it was indeed a valid warning,
> > and it has recently been fixed via [3] (see discussion on its PR [4])
> >
> > Best regards,
> > Ruben
> >
> > [1] https://issues.apache.org/jira/browse/CALCITE-4885
> > [2] https://github.com/apache/calcite/pull/2685
> > [3]
> >
> https://github.com/apache/calcite/commit/4bebdb07c2f45a95c9a4fdf81e9bcfbdd11a15de
> > [4] https://github.com/apache/calcite/pull/2919
> >
> >
> >
> > On Tue, Jan 10, 2023 at 3:18 PM Stamatis Zampetakis <za...@gmail.com>
> > wrote:
> >
> >> Thanks for the feedback!
> >>
> >> I would like to stretch the fact that at this point it is at the
> discretion
> >> of the reviewer/committer to enforce or ignore the information provided
> by
> >> Sonar.
> >>
> >> Sonar, as other similar systems, has limitations thus there are always
> >> going to be false positives. The rules/checks performed are fully
> >> customisable so we can enable/disable them at will.
> >>
> >> The two issues highlighted by Julian seem like true positives to me.
> >> * The "New code" that was introduced recently is not covered
> sufficiently
> >> by tests and that's a fact. Part of the problem seems to come from the
> >> recent modifications in SqlOperatorTest [1]. The class is located under
> >> src/main (and not under src/test) so it is considered as a production
> class
> >> and coverage checks are applied. There are ways to exclude certain paths
> >> from coverage but I would argue that the class shouldn't be there in the
> >> first place; we should probably log a CALCITE ticket for moving out of
> >> there.
> >> * The instance of warning is something that we probably don't
> want/cannot
> >> fix (for the reasons mentioned in the PR) but Sonar did well to bring
> this
> >> up; it is problematic to do comparisons by relying on the class name.
> >>
> >> I encourage others to share their thoughts/remarks as well so that we
> >> improve as much as possible the developer experience.
> >>
> >> Best,
> >> Stamatis
> >>
> >> [1]
> >>
> >>
> https://github.com/apache/calcite/blob/be7135cf1fd3d87e4036b2dd6e58d2f1251f8959/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java
> >>
> >> On Tue, Jan 10, 2023 at 1:38 AM Julian Hyde <jh...@apache.org> wrote:
> >>
> >>> I see two false positives so far:
> >>> * The message on be7135cf "58.1% Coverage on New Code (is less than
> >> 80%)"
> >>> * The bug on PR 2942 "Use an "instanceof" comparison instead"
> >>>
> >>> Has Sonarcube had any true positives yet?
> >>>
> >>> Vladimir used to hate when I was skeptical of changes to the build
> >>> system. But I have no tolerance for a lint system that makes extra
> >>> work.
> >>>
> >>> Julian
> >>>
> >>>
> >>>
> >>>
> >>> On Mon, Jan 9, 2023 at 2:46 PM Francis Chuang <
> francischuang@apache.org>
> >>> wrote:
> >>>>
> >>>> Thanks for implementing this, Stamatis! Having code quality metrics on
> >>>> our repos is a huge win.
> >>>>
> >>>> Francis
> >>>>
> >>>> On 10/01/2023 4:22 am, Stamatis Zampetakis wrote:
> >>>>> I just merged the CALCITE-5427 [1] enabling Sonarcloud analysis for
> >>> Calcite
> >>>>> main branch and new PRs.
> >>>>>
> >>>>> I have left the default Sonar quality gates active so you may start
> >>> seeing
> >>>>> Sonar reporting errors in various cases.
> >>>>>
> >>>>> If you encounter problems or you would like things to work
> >> differently
> >>>>> please create JIRA tickets and ping me if you need help.
> >>>>>
> >>>>> Once we are happy with how things work for Calcite we can also port
> >> the
> >>>>> changes to Avatica.
> >>>>>
> >>>>> Note that all Calcite committers have administrative privileges to
> >> the
> >>>>> Calcite project in Sonarcloud [2].
> >>>>>
> >>>>> Best,
> >>>>> Stamatis
> >>>>>
> >>>>> [1]
> >>>>>
> >>>
> >>
> https://github.com/apache/calcite/commit/be7135cf1fd3d87e4036b2dd6e58d2f1251f8959
> >>>>> [2] https://sonarcloud.io/project/roles?id=apache_calcite
> >>>>>
> >>>>> On Wed, Jan 4, 2023 at 5:44 PM Stamatis Zampetakis <
> >> zabetak@gmail.com>
> >>>>> wrote:
> >>>>>
> >>>>>> The integration is advancing well and I am hoping to merge the PR in
> >>> the
> >>>>>> coming days.
> >>>>>> To avoid unpleasant surprises, I am planning to create a new remote
> >>> branch
> >>>>>> in the main calcite repo to test some things out. I will delete it
> >> as
> >>> soon
> >>>>>> as I am done with the tests.
> >>>>>>
> >>>>>> Best.
> >>>>>> Stamatis
> >>>>>>
> >>>>>> On Wed, Dec 28, 2022 at 2:47 PM Michael Mior <mm...@apache.org>
> >>> wrote:
> >>>>>>
> >>>>>>> Thanks Stamatis! I haven't used SonarCloud before, but in general I
> >>> think
> >>>>>>> such tools can be quite useful.
> >>>>>>>
> >>>>>>> --
> >>>>>>> Michael Mior
> >>>>>>> mmior@apache.org
> >>>>>>>
> >>>>>>>
> >>>>>>> On Sat, Dec 24, 2022 at 4:01 PM Stamatis Zampetakis <
> >>> zabetak@gmail.com>
> >>>>>>> wrote:
> >>>>>>>
> >>>>>>>> Since there were no objections, I just logged INFRA-24038 [1] and
> >>> plan
> >>>>>>> to
> >>>>>>>> move this forward.
> >>>>>>>>
> >>>>>>>> Let me know if you have questions or concerns regarding the
> >>> adoption of
> >>>>>>>> SonarCloud.
> >>>>>>>>
> >>>>>>>> Best,
> >>>>>>>> Stamatis
> >>>>>>>>
> >>>>>>>> [1] https://issues.apache.org/jira/browse/INFRA-24038
> >>>>>>>>
> >>>>>>>> On Sat, Dec 10, 2022 at 11:32 AM Benchao Li <libenchao@apache.org
> >>>
> >>>>>>> wrote:
> >>>>>>>>
> >>>>>>>>> Thanks Stamatis for bringing this up.
> >>>>>>>>>
> >>>>>>>>> I haven't used Sonar yet, but thanks for the demo[1] you
> >> provided,
> >>> it
> >>>>>>>> looks
> >>>>>>>>> really interesting. I think it's worth a try for Calcite.
> >>>>>>>>>
> >>>>>>>>> [1] https://github.com/zabetak/calcite/pull/9
> >>>>>>>>>
> >>>>>>>>> Alessandro Solimando <al...@gmail.com>
> >>> 于2022年12月10日周六
> >>>>>>>>> 02:54写道：
> >>>>>>>>>
> >>>>>>>>>> Hi all,
> >>>>>>>>>> thanks Stamatis for the proposal and the work, I am a huge fan
> >> of
> >>>>>>> Sonar
> >>>>>>>>> and
> >>>>>>>>>> I really miss it on Calcite, so a big +1 from me on this.
> >>>>>>>>>>
> >>>>>>>>>> In Hive so far we have received good feedback, so I hope it will
> >>> be
> >>>>>>>>>> welcomed here too.
> >>>>>>>>>>
> >>>>>>>>>> Best regards,
> >>>>>>>>>> Alessandro
> >>>>>>>>>>
> >>>>>>>>>> On Fri, 9 Dec 2022 at 19:02, Stamatis Zampetakis <
> >>> zabetak@gmail.com
> >>>>>>>>
> >>>>>>>>>> wrote:
> >>>>>>>>>>
> >>>>>>>>>>> Hi all,
> >>>>>>>>>>>
> >>>>>>>>>>> I just logged CALCITE-5427 [1] for introducing code quality &
> >>>>>>>> coverage
> >>>>>>>>>>> metrics in Calcite CI.
> >>>>>>>>>>>
> >>>>>>>>>>> I added some motivation and examples under the ticket so please
> >>>>>>> have
> >>>>>>>> a
> >>>>>>>>>> look
> >>>>>>>>>>> and let me know what you think.
> >>>>>>>>>>>
> >>>>>>>>>>> If there are no objections, I will follow-up with INFRA to set
> >>>>>>> things
> >>>>>>>>> up
> >>>>>>>>>>> for the official Calcite repo.
> >>>>>>>>>>>
> >>>>>>>>>>> The integration with SonarCloud has been inspired by HIVE-26196
> >>>>>>> [2]
> >>>>>>>>> that
> >>>>>>>>>>> Alessandro put in place for Hive and has been very helpful so
> >>> far.
> >>>>>>>>>>>
> >>>>>>>>>>> Best,
> >>>>>>>>>>> Stamatis
> >>>>>>>>>>>
> >>>>>>>>>>> [1] https://issues.apache.org/jira/browse/CALCITE-5427
> >>>>>>>>>>> [2] https://issues.apache.org/jira/browse/HIVE-26196
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> --
> >>>>>>>>>
> >>>>>>>>> Best,
> >>>>>>>>> Benchao Li
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>
> >>>
> >>
>
>

Re: [DISCUSS] Code quality/coverage with SonarCloud & JaCoCo

[Julian Hyde <jh...@gmail.com>]

The instanceof case was complicated. The code that Kevin wrote was good, and appropriate, and when Sonar blocked it Stamatis was able to find an alternative formulation, which existed because, by luck, the JSR had deprecated an exception class but not its base class. I spent 30 minutes reviewing the PR and was about to merge it, and because of Sonar’s bump in the road that time was wasted. I doubt that there has been a single other occasion when someone wrote “com.example.MyClass”.equals(x.getClass().getName()) instead of “x instanceof MyClass”. So far that particular check has cost us ~1 hour and not saved us any time.

I’m not saying that Sonar is net bad. I’m just saying let’s do a true accounting.


> On Jan 11, 2023, at 2:42 AM, Ruben Q L <ru...@gmail.com> wrote:
> 
> Hello,
> 
> First of all, thanks Stamatis for implementing this, I think it is
> something good for the project.
> In the beginning things might be a bit complicated (as always) and we might
> need some adjustments / clarifications, but I hope that in the long run
> we'll see this as a useful feature.
> 
> Regarding the two specific issues being discussed:
> - If I am not mistaken, the fact that SqlOperatorTest was moved out of
> 'test' was a consequence of [1], see the corresponding PR [2] "... it was
> necessary to move several classes from the 'core' module to 'testkit'". I
> don't know how simple (or how complex) a potential refactoring to move it
> out of there might be. Alternatively, it seems that this is rather an
> exceptional case, so perhaps it should qualify for an exception (e.g.
> everything under /testkit/* shall not be considered for coverage).
> - Regarding the instanceof, it seems that it was indeed a valid warning,
> and it has recently been fixed via [3] (see discussion on its PR [4])
> 
> Best regards,
> Ruben
> 
> [1] https://issues.apache.org/jira/browse/CALCITE-4885
> [2] https://github.com/apache/calcite/pull/2685
> [3]
> https://github.com/apache/calcite/commit/4bebdb07c2f45a95c9a4fdf81e9bcfbdd11a15de
> [4] https://github.com/apache/calcite/pull/2919
> 
> 
> 
> On Tue, Jan 10, 2023 at 3:18 PM Stamatis Zampetakis <za...@gmail.com>
> wrote:
> 
>> Thanks for the feedback!
>> 
>> I would like to stretch the fact that at this point it is at the discretion
>> of the reviewer/committer to enforce or ignore the information provided by
>> Sonar.
>> 
>> Sonar, as other similar systems, has limitations thus there are always
>> going to be false positives. The rules/checks performed are fully
>> customisable so we can enable/disable them at will.
>> 
>> The two issues highlighted by Julian seem like true positives to me.
>> * The "New code" that was introduced recently is not covered sufficiently
>> by tests and that's a fact. Part of the problem seems to come from the
>> recent modifications in SqlOperatorTest [1]. The class is located under
>> src/main (and not under src/test) so it is considered as a production class
>> and coverage checks are applied. There are ways to exclude certain paths
>> from coverage but I would argue that the class shouldn't be there in the
>> first place; we should probably log a CALCITE ticket for moving out of
>> there.
>> * The instance of warning is something that we probably don't want/cannot
>> fix (for the reasons mentioned in the PR) but Sonar did well to bring this
>> up; it is problematic to do comparisons by relying on the class name.
>> 
>> I encourage others to share their thoughts/remarks as well so that we
>> improve as much as possible the developer experience.
>> 
>> Best,
>> Stamatis
>> 
>> [1]
>> 
>> https://github.com/apache/calcite/blob/be7135cf1fd3d87e4036b2dd6e58d2f1251f8959/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java
>> 
>> On Tue, Jan 10, 2023 at 1:38 AM Julian Hyde <jh...@apache.org> wrote:
>> 
>>> I see two false positives so far:
>>> * The message on be7135cf "58.1% Coverage on New Code (is less than
>> 80%)"
>>> * The bug on PR 2942 "Use an "instanceof" comparison instead"
>>> 
>>> Has Sonarcube had any true positives yet?
>>> 
>>> Vladimir used to hate when I was skeptical of changes to the build
>>> system. But I have no tolerance for a lint system that makes extra
>>> work.
>>> 
>>> Julian
>>> 
>>> 
>>> 
>>> 
>>> On Mon, Jan 9, 2023 at 2:46 PM Francis Chuang <fr...@apache.org>
>>> wrote:
>>>> 
>>>> Thanks for implementing this, Stamatis! Having code quality metrics on
>>>> our repos is a huge win.
>>>> 
>>>> Francis
>>>> 
>>>> On 10/01/2023 4:22 am, Stamatis Zampetakis wrote:
>>>>> I just merged the CALCITE-5427 [1] enabling Sonarcloud analysis for
>>> Calcite
>>>>> main branch and new PRs.
>>>>> 
>>>>> I have left the default Sonar quality gates active so you may start
>>> seeing
>>>>> Sonar reporting errors in various cases.
>>>>> 
>>>>> If you encounter problems or you would like things to work
>> differently
>>>>> please create JIRA tickets and ping me if you need help.
>>>>> 
>>>>> Once we are happy with how things work for Calcite we can also port
>> the
>>>>> changes to Avatica.
>>>>> 
>>>>> Note that all Calcite committers have administrative privileges to
>> the
>>>>> Calcite project in Sonarcloud [2].
>>>>> 
>>>>> Best,
>>>>> Stamatis
>>>>> 
>>>>> [1]
>>>>> 
>>> 
>> https://github.com/apache/calcite/commit/be7135cf1fd3d87e4036b2dd6e58d2f1251f8959
>>>>> [2] https://sonarcloud.io/project/roles?id=apache_calcite
>>>>> 
>>>>> On Wed, Jan 4, 2023 at 5:44 PM Stamatis Zampetakis <
>> zabetak@gmail.com>
>>>>> wrote:
>>>>> 
>>>>>> The integration is advancing well and I am hoping to merge the PR in
>>> the
>>>>>> coming days.
>>>>>> To avoid unpleasant surprises, I am planning to create a new remote
>>> branch
>>>>>> in the main calcite repo to test some things out. I will delete it
>> as
>>> soon
>>>>>> as I am done with the tests.
>>>>>> 
>>>>>> Best.
>>>>>> Stamatis
>>>>>> 
>>>>>> On Wed, Dec 28, 2022 at 2:47 PM Michael Mior <mm...@apache.org>
>>> wrote:
>>>>>> 
>>>>>>> Thanks Stamatis! I haven't used SonarCloud before, but in general I
>>> think
>>>>>>> such tools can be quite useful.
>>>>>>> 
>>>>>>> --
>>>>>>> Michael Mior
>>>>>>> mmior@apache.org
>>>>>>> 
>>>>>>> 
>>>>>>> On Sat, Dec 24, 2022 at 4:01 PM Stamatis Zampetakis <
>>> zabetak@gmail.com>
>>>>>>> wrote:
>>>>>>> 
>>>>>>>> Since there were no objections, I just logged INFRA-24038 [1] and
>>> plan
>>>>>>> to
>>>>>>>> move this forward.
>>>>>>>> 
>>>>>>>> Let me know if you have questions or concerns regarding the
>>> adoption of
>>>>>>>> SonarCloud.
>>>>>>>> 
>>>>>>>> Best,
>>>>>>>> Stamatis
>>>>>>>> 
>>>>>>>> [1] https://issues.apache.org/jira/browse/INFRA-24038
>>>>>>>> 
>>>>>>>> On Sat, Dec 10, 2022 at 11:32 AM Benchao Li <libenchao@apache.org
>>> 
>>>>>>> wrote:
>>>>>>>> 
>>>>>>>>> Thanks Stamatis for bringing this up.
>>>>>>>>> 
>>>>>>>>> I haven't used Sonar yet, but thanks for the demo[1] you
>> provided,
>>> it
>>>>>>>> looks
>>>>>>>>> really interesting. I think it's worth a try for Calcite.
>>>>>>>>> 
>>>>>>>>> [1] https://github.com/zabetak/calcite/pull/9
>>>>>>>>> 
>>>>>>>>> Alessandro Solimando <al...@gmail.com>
>>> 于2022年12月10日周六
>>>>>>>>> 02:54写道：
>>>>>>>>> 
>>>>>>>>>> Hi all,
>>>>>>>>>> thanks Stamatis for the proposal and the work, I am a huge fan
>> of
>>>>>>> Sonar
>>>>>>>>> and
>>>>>>>>>> I really miss it on Calcite, so a big +1 from me on this.
>>>>>>>>>> 
>>>>>>>>>> In Hive so far we have received good feedback, so I hope it will
>>> be
>>>>>>>>>> welcomed here too.
>>>>>>>>>> 
>>>>>>>>>> Best regards,
>>>>>>>>>> Alessandro
>>>>>>>>>> 
>>>>>>>>>> On Fri, 9 Dec 2022 at 19:02, Stamatis Zampetakis <
>>> zabetak@gmail.com
>>>>>>>> 
>>>>>>>>>> wrote:
>>>>>>>>>> 
>>>>>>>>>>> Hi all,
>>>>>>>>>>> 
>>>>>>>>>>> I just logged CALCITE-5427 [1] for introducing code quality &
>>>>>>>> coverage
>>>>>>>>>>> metrics in Calcite CI.
>>>>>>>>>>> 
>>>>>>>>>>> I added some motivation and examples under the ticket so please
>>>>>>> have
>>>>>>>> a
>>>>>>>>>> look
>>>>>>>>>>> and let me know what you think.
>>>>>>>>>>> 
>>>>>>>>>>> If there are no objections, I will follow-up with INFRA to set
>>>>>>> things
>>>>>>>>> up
>>>>>>>>>>> for the official Calcite repo.
>>>>>>>>>>> 
>>>>>>>>>>> The integration with SonarCloud has been inspired by HIVE-26196
>>>>>>> [2]
>>>>>>>>> that
>>>>>>>>>>> Alessandro put in place for Hive and has been very helpful so
>>> far.
>>>>>>>>>>> 
>>>>>>>>>>> Best,
>>>>>>>>>>> Stamatis
>>>>>>>>>>> 
>>>>>>>>>>> [1] https://issues.apache.org/jira/browse/CALCITE-5427
>>>>>>>>>>> [2] https://issues.apache.org/jira/browse/HIVE-26196
>>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> --
>>>>>>>>> 
>>>>>>>>> Best,
>>>>>>>>> Benchao Li
>>>>>>>>> 
>>>>>>>> 
>>>>>>> 
>>>>>> 
>>>>> 
>>> 
>> 

Re: [DISCUSS] Code quality/coverage with SonarCloud & JaCoCo

[Ruben Q L <ru...@gmail.com>]

Hello,

First of all, thanks Stamatis for implementing this, I think it is
something good for the project.
In the beginning things might be a bit complicated (as always) and we might
need some adjustments / clarifications, but I hope that in the long run
we'll see this as a useful feature.

Regarding the two specific issues being discussed:
- If I am not mistaken, the fact that SqlOperatorTest was moved out of
'test' was a consequence of [1], see the corresponding PR [2] "... it was
necessary to move several classes from the 'core' module to 'testkit'". I
don't know how simple (or how complex) a potential refactoring to move it
out of there might be. Alternatively, it seems that this is rather an
exceptional case, so perhaps it should qualify for an exception (e.g.
everything under /testkit/* shall not be considered for coverage).
- Regarding the instanceof, it seems that it was indeed a valid warning,
and it has recently been fixed via [3] (see discussion on its PR [4])

Best regards,
Ruben

[1] https://issues.apache.org/jira/browse/CALCITE-4885
[2] https://github.com/apache/calcite/pull/2685
[3]
https://github.com/apache/calcite/commit/4bebdb07c2f45a95c9a4fdf81e9bcfbdd11a15de
[4] https://github.com/apache/calcite/pull/2919



On Tue, Jan 10, 2023 at 3:18 PM Stamatis Zampetakis <za...@gmail.com>
wrote:

> Thanks for the feedback!
>
> I would like to stretch the fact that at this point it is at the discretion
> of the reviewer/committer to enforce or ignore the information provided by
> Sonar.
>
> Sonar, as other similar systems, has limitations thus there are always
> going to be false positives. The rules/checks performed are fully
> customisable so we can enable/disable them at will.
>
> The two issues highlighted by Julian seem like true positives to me.
> * The "New code" that was introduced recently is not covered sufficiently
> by tests and that's a fact. Part of the problem seems to come from the
> recent modifications in SqlOperatorTest [1]. The class is located under
> src/main (and not under src/test) so it is considered as a production class
> and coverage checks are applied. There are ways to exclude certain paths
> from coverage but I would argue that the class shouldn't be there in the
> first place; we should probably log a CALCITE ticket for moving out of
> there.
> * The instance of warning is something that we probably don't want/cannot
> fix (for the reasons mentioned in the PR) but Sonar did well to bring this
> up; it is problematic to do comparisons by relying on the class name.
>
> I encourage others to share their thoughts/remarks as well so that we
> improve as much as possible the developer experience.
>
> Best,
> Stamatis
>
> [1]
>
> https://github.com/apache/calcite/blob/be7135cf1fd3d87e4036b2dd6e58d2f1251f8959/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java
>
> On Tue, Jan 10, 2023 at 1:38 AM Julian Hyde <jh...@apache.org> wrote:
>
> > I see two false positives so far:
> >  * The message on be7135cf "58.1% Coverage on New Code (is less than
> 80%)"
> >  * The bug on PR 2942 "Use an "instanceof" comparison instead"
> >
> > Has Sonarcube had any true positives yet?
> >
> > Vladimir used to hate when I was skeptical of changes to the build
> > system. But I have no tolerance for a lint system that makes extra
> > work.
> >
> > Julian
> >
> >
> >
> >
> > On Mon, Jan 9, 2023 at 2:46 PM Francis Chuang <fr...@apache.org>
> > wrote:
> > >
> > > Thanks for implementing this, Stamatis! Having code quality metrics on
> > > our repos is a huge win.
> > >
> > > Francis
> > >
> > > On 10/01/2023 4:22 am, Stamatis Zampetakis wrote:
> > > > I just merged the CALCITE-5427 [1] enabling Sonarcloud analysis for
> > Calcite
> > > > main branch and new PRs.
> > > >
> > > > I have left the default Sonar quality gates active so you may start
> > seeing
> > > > Sonar reporting errors in various cases.
> > > >
> > > > If you encounter problems or you would like things to work
> differently
> > > > please create JIRA tickets and ping me if you need help.
> > > >
> > > > Once we are happy with how things work for Calcite we can also port
> the
> > > > changes to Avatica.
> > > >
> > > > Note that all Calcite committers have administrative privileges to
> the
> > > > Calcite project in Sonarcloud [2].
> > > >
> > > > Best,
> > > > Stamatis
> > > >
> > > > [1]
> > > >
> >
> https://github.com/apache/calcite/commit/be7135cf1fd3d87e4036b2dd6e58d2f1251f8959
> > > > [2] https://sonarcloud.io/project/roles?id=apache_calcite
> > > >
> > > > On Wed, Jan 4, 2023 at 5:44 PM Stamatis Zampetakis <
> zabetak@gmail.com>
> > > > wrote:
> > > >
> > > >> The integration is advancing well and I am hoping to merge the PR in
> > the
> > > >> coming days.
> > > >> To avoid unpleasant surprises, I am planning to create a new remote
> > branch
> > > >> in the main calcite repo to test some things out. I will delete it
> as
> > soon
> > > >> as I am done with the tests.
> > > >>
> > > >> Best.
> > > >> Stamatis
> > > >>
> > > >> On Wed, Dec 28, 2022 at 2:47 PM Michael Mior <mm...@apache.org>
> > wrote:
> > > >>
> > > >>> Thanks Stamatis! I haven't used SonarCloud before, but in general I
> > think
> > > >>> such tools can be quite useful.
> > > >>>
> > > >>> --
> > > >>> Michael Mior
> > > >>> mmior@apache.org
> > > >>>
> > > >>>
> > > >>> On Sat, Dec 24, 2022 at 4:01 PM Stamatis Zampetakis <
> > zabetak@gmail.com>
> > > >>> wrote:
> > > >>>
> > > >>>> Since there were no objections, I just logged INFRA-24038 [1] and
> > plan
> > > >>> to
> > > >>>> move this forward.
> > > >>>>
> > > >>>> Let me know if you have questions or concerns regarding the
> > adoption of
> > > >>>> SonarCloud.
> > > >>>>
> > > >>>> Best,
> > > >>>> Stamatis
> > > >>>>
> > > >>>> [1] https://issues.apache.org/jira/browse/INFRA-24038
> > > >>>>
> > > >>>> On Sat, Dec 10, 2022 at 11:32 AM Benchao Li <libenchao@apache.org
> >
> > > >>> wrote:
> > > >>>>
> > > >>>>> Thanks Stamatis for bringing this up.
> > > >>>>>
> > > >>>>> I haven't used Sonar yet, but thanks for the demo[1] you
> provided,
> > it
> > > >>>> looks
> > > >>>>> really interesting. I think it's worth a try for Calcite.
> > > >>>>>
> > > >>>>> [1] https://github.com/zabetak/calcite/pull/9
> > > >>>>>
> > > >>>>> Alessandro Solimando <al...@gmail.com>
> > 于2022年12月10日周六
> > > >>>>> 02:54写道：
> > > >>>>>
> > > >>>>>> Hi all,
> > > >>>>>> thanks Stamatis for the proposal and the work, I am a huge fan
> of
> > > >>> Sonar
> > > >>>>> and
> > > >>>>>> I really miss it on Calcite, so a big +1 from me on this.
> > > >>>>>>
> > > >>>>>> In Hive so far we have received good feedback, so I hope it will
> > be
> > > >>>>>> welcomed here too.
> > > >>>>>>
> > > >>>>>> Best regards,
> > > >>>>>> Alessandro
> > > >>>>>>
> > > >>>>>> On Fri, 9 Dec 2022 at 19:02, Stamatis Zampetakis <
> > zabetak@gmail.com
> > > >>>>
> > > >>>>>> wrote:
> > > >>>>>>
> > > >>>>>>> Hi all,
> > > >>>>>>>
> > > >>>>>>> I just logged CALCITE-5427 [1] for introducing code quality &
> > > >>>> coverage
> > > >>>>>>> metrics in Calcite CI.
> > > >>>>>>>
> > > >>>>>>> I added some motivation and examples under the ticket so please
> > > >>> have
> > > >>>> a
> > > >>>>>> look
> > > >>>>>>> and let me know what you think.
> > > >>>>>>>
> > > >>>>>>> If there are no objections, I will follow-up with INFRA to set
> > > >>> things
> > > >>>>> up
> > > >>>>>>> for the official Calcite repo.
> > > >>>>>>>
> > > >>>>>>> The integration with SonarCloud has been inspired by HIVE-26196
> > > >>> [2]
> > > >>>>> that
> > > >>>>>>> Alessandro put in place for Hive and has been very helpful so
> > far.
> > > >>>>>>>
> > > >>>>>>> Best,
> > > >>>>>>> Stamatis
> > > >>>>>>>
> > > >>>>>>> [1] https://issues.apache.org/jira/browse/CALCITE-5427
> > > >>>>>>> [2] https://issues.apache.org/jira/browse/HIVE-26196
> > > >>>>>>>
> > > >>>>>>
> > > >>>>>
> > > >>>>>
> > > >>>>> --
> > > >>>>>
> > > >>>>> Best,
> > > >>>>> Benchao Li
> > > >>>>>
> > > >>>>
> > > >>>
> > > >>
> > > >
> >
>

Re: [DISCUSS] Code quality/coverage with SonarCloud & JaCoCo

[Stamatis Zampetakis <za...@gmail.com>]

Thanks for the feedback!

I would like to stretch the fact that at this point it is at the discretion
of the reviewer/committer to enforce or ignore the information provided by
Sonar.

Sonar, as other similar systems, has limitations thus there are always
going to be false positives. The rules/checks performed are fully
customisable so we can enable/disable them at will.

The two issues highlighted by Julian seem like true positives to me.
* The "New code" that was introduced recently is not covered sufficiently
by tests and that's a fact. Part of the problem seems to come from the
recent modifications in SqlOperatorTest [1]. The class is located under
src/main (and not under src/test) so it is considered as a production class
and coverage checks are applied. There are ways to exclude certain paths
from coverage but I would argue that the class shouldn't be there in the
first place; we should probably log a CALCITE ticket for moving out of
there.
* The instance of warning is something that we probably don't want/cannot
fix (for the reasons mentioned in the PR) but Sonar did well to bring this
up; it is problematic to do comparisons by relying on the class name.

I encourage others to share their thoughts/remarks as well so that we
improve as much as possible the developer experience.

Best,
Stamatis

[1]
https://github.com/apache/calcite/blob/be7135cf1fd3d87e4036b2dd6e58d2f1251f8959/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

On Tue, Jan 10, 2023 at 1:38 AM Julian Hyde <jh...@apache.org> wrote:

> I see two false positives so far:
>  * The message on be7135cf "58.1% Coverage on New Code (is less than 80%)"
>  * The bug on PR 2942 "Use an "instanceof" comparison instead"
>
> Has Sonarcube had any true positives yet?
>
> Vladimir used to hate when I was skeptical of changes to the build
> system. But I have no tolerance for a lint system that makes extra
> work.
>
> Julian
>
>
>
>
> On Mon, Jan 9, 2023 at 2:46 PM Francis Chuang <fr...@apache.org>
> wrote:
> >
> > Thanks for implementing this, Stamatis! Having code quality metrics on
> > our repos is a huge win.
> >
> > Francis
> >
> > On 10/01/2023 4:22 am, Stamatis Zampetakis wrote:
> > > I just merged the CALCITE-5427 [1] enabling Sonarcloud analysis for
> Calcite
> > > main branch and new PRs.
> > >
> > > I have left the default Sonar quality gates active so you may start
> seeing
> > > Sonar reporting errors in various cases.
> > >
> > > If you encounter problems or you would like things to work differently
> > > please create JIRA tickets and ping me if you need help.
> > >
> > > Once we are happy with how things work for Calcite we can also port the
> > > changes to Avatica.
> > >
> > > Note that all Calcite committers have administrative privileges to the
> > > Calcite project in Sonarcloud [2].
> > >
> > > Best,
> > > Stamatis
> > >
> > > [1]
> > >
> https://github.com/apache/calcite/commit/be7135cf1fd3d87e4036b2dd6e58d2f1251f8959
> > > [2] https://sonarcloud.io/project/roles?id=apache_calcite
> > >
> > > On Wed, Jan 4, 2023 at 5:44 PM Stamatis Zampetakis <za...@gmail.com>
> > > wrote:
> > >
> > >> The integration is advancing well and I am hoping to merge the PR in
> the
> > >> coming days.
> > >> To avoid unpleasant surprises, I am planning to create a new remote
> branch
> > >> in the main calcite repo to test some things out. I will delete it as
> soon
> > >> as I am done with the tests.
> > >>
> > >> Best.
> > >> Stamatis
> > >>
> > >> On Wed, Dec 28, 2022 at 2:47 PM Michael Mior <mm...@apache.org>
> wrote:
> > >>
> > >>> Thanks Stamatis! I haven't used SonarCloud before, but in general I
> think
> > >>> such tools can be quite useful.
> > >>>
> > >>> --
> > >>> Michael Mior
> > >>> mmior@apache.org
> > >>>
> > >>>
> > >>> On Sat, Dec 24, 2022 at 4:01 PM Stamatis Zampetakis <
> zabetak@gmail.com>
> > >>> wrote:
> > >>>
> > >>>> Since there were no objections, I just logged INFRA-24038 [1] and
> plan
> > >>> to
> > >>>> move this forward.
> > >>>>
> > >>>> Let me know if you have questions or concerns regarding the
> adoption of
> > >>>> SonarCloud.
> > >>>>
> > >>>> Best,
> > >>>> Stamatis
> > >>>>
> > >>>> [1] https://issues.apache.org/jira/browse/INFRA-24038
> > >>>>
> > >>>> On Sat, Dec 10, 2022 at 11:32 AM Benchao Li <li...@apache.org>
> > >>> wrote:
> > >>>>
> > >>>>> Thanks Stamatis for bringing this up.
> > >>>>>
> > >>>>> I haven't used Sonar yet, but thanks for the demo[1] you provided,
> it
> > >>>> looks
> > >>>>> really interesting. I think it's worth a try for Calcite.
> > >>>>>
> > >>>>> [1] https://github.com/zabetak/calcite/pull/9
> > >>>>>
> > >>>>> Alessandro Solimando <al...@gmail.com>
> 于2022年12月10日周六
> > >>>>> 02:54写道：
> > >>>>>
> > >>>>>> Hi all,
> > >>>>>> thanks Stamatis for the proposal and the work, I am a huge fan of
> > >>> Sonar
> > >>>>> and
> > >>>>>> I really miss it on Calcite, so a big +1 from me on this.
> > >>>>>>
> > >>>>>> In Hive so far we have received good feedback, so I hope it will
> be
> > >>>>>> welcomed here too.
> > >>>>>>
> > >>>>>> Best regards,
> > >>>>>> Alessandro
> > >>>>>>
> > >>>>>> On Fri, 9 Dec 2022 at 19:02, Stamatis Zampetakis <
> zabetak@gmail.com
> > >>>>
> > >>>>>> wrote:
> > >>>>>>
> > >>>>>>> Hi all,
> > >>>>>>>
> > >>>>>>> I just logged CALCITE-5427 [1] for introducing code quality &
> > >>>> coverage
> > >>>>>>> metrics in Calcite CI.
> > >>>>>>>
> > >>>>>>> I added some motivation and examples under the ticket so please
> > >>> have
> > >>>> a
> > >>>>>> look
> > >>>>>>> and let me know what you think.
> > >>>>>>>
> > >>>>>>> If there are no objections, I will follow-up with INFRA to set
> > >>> things
> > >>>>> up
> > >>>>>>> for the official Calcite repo.
> > >>>>>>>
> > >>>>>>> The integration with SonarCloud has been inspired by HIVE-26196
> > >>> [2]
> > >>>>> that
> > >>>>>>> Alessandro put in place for Hive and has been very helpful so
> far.
> > >>>>>>>
> > >>>>>>> Best,
> > >>>>>>> Stamatis
> > >>>>>>>
> > >>>>>>> [1] https://issues.apache.org/jira/browse/CALCITE-5427
> > >>>>>>> [2] https://issues.apache.org/jira/browse/HIVE-26196
> > >>>>>>>
> > >>>>>>
> > >>>>>
> > >>>>>
> > >>>>> --
> > >>>>>
> > >>>>> Best,
> > >>>>> Benchao Li
> > >>>>>
> > >>>>
> > >>>
> > >>
> > >
>

Re: [DISCUSS] Code quality/coverage with SonarCloud & JaCoCo

[Julian Hyde <jh...@apache.org>]

I see two false positives so far:
 * The message on be7135cf "58.1% Coverage on New Code (is less than 80%)"
 * The bug on PR 2942 "Use an "instanceof" comparison instead"

Has Sonarcube had any true positives yet?

Vladimir used to hate when I was skeptical of changes to the build
system. But I have no tolerance for a lint system that makes extra
work.

Julian




On Mon, Jan 9, 2023 at 2:46 PM Francis Chuang <fr...@apache.org> wrote:
>
> Thanks for implementing this, Stamatis! Having code quality metrics on
> our repos is a huge win.
>
> Francis
>
> On 10/01/2023 4:22 am, Stamatis Zampetakis wrote:
> > I just merged the CALCITE-5427 [1] enabling Sonarcloud analysis for Calcite
> > main branch and new PRs.
> >
> > I have left the default Sonar quality gates active so you may start seeing
> > Sonar reporting errors in various cases.
> >
> > If you encounter problems or you would like things to work differently
> > please create JIRA tickets and ping me if you need help.
> >
> > Once we are happy with how things work for Calcite we can also port the
> > changes to Avatica.
> >
> > Note that all Calcite committers have administrative privileges to the
> > Calcite project in Sonarcloud [2].
> >
> > Best,
> > Stamatis
> >
> > [1]
> > https://github.com/apache/calcite/commit/be7135cf1fd3d87e4036b2dd6e58d2f1251f8959
> > [2] https://sonarcloud.io/project/roles?id=apache_calcite
> >
> > On Wed, Jan 4, 2023 at 5:44 PM Stamatis Zampetakis <za...@gmail.com>
> > wrote:
> >
> >> The integration is advancing well and I am hoping to merge the PR in the
> >> coming days.
> >> To avoid unpleasant surprises, I am planning to create a new remote branch
> >> in the main calcite repo to test some things out. I will delete it as soon
> >> as I am done with the tests.
> >>
> >> Best.
> >> Stamatis
> >>
> >> On Wed, Dec 28, 2022 at 2:47 PM Michael Mior <mm...@apache.org> wrote:
> >>
> >>> Thanks Stamatis! I haven't used SonarCloud before, but in general I think
> >>> such tools can be quite useful.
> >>>
> >>> --
> >>> Michael Mior
> >>> mmior@apache.org
> >>>
> >>>
> >>> On Sat, Dec 24, 2022 at 4:01 PM Stamatis Zampetakis <za...@gmail.com>
> >>> wrote:
> >>>
> >>>> Since there were no objections, I just logged INFRA-24038 [1] and plan
> >>> to
> >>>> move this forward.
> >>>>
> >>>> Let me know if you have questions or concerns regarding the adoption of
> >>>> SonarCloud.
> >>>>
> >>>> Best,
> >>>> Stamatis
> >>>>
> >>>> [1] https://issues.apache.org/jira/browse/INFRA-24038
> >>>>
> >>>> On Sat, Dec 10, 2022 at 11:32 AM Benchao Li <li...@apache.org>
> >>> wrote:
> >>>>
> >>>>> Thanks Stamatis for bringing this up.
> >>>>>
> >>>>> I haven't used Sonar yet, but thanks for the demo[1] you provided, it
> >>>> looks
> >>>>> really interesting. I think it's worth a try for Calcite.
> >>>>>
> >>>>> [1] https://github.com/zabetak/calcite/pull/9
> >>>>>
> >>>>> Alessandro Solimando <al...@gmail.com> 于2022年12月10日周六
> >>>>> 02:54写道：
> >>>>>
> >>>>>> Hi all,
> >>>>>> thanks Stamatis for the proposal and the work, I am a huge fan of
> >>> Sonar
> >>>>> and
> >>>>>> I really miss it on Calcite, so a big +1 from me on this.
> >>>>>>
> >>>>>> In Hive so far we have received good feedback, so I hope it will be
> >>>>>> welcomed here too.
> >>>>>>
> >>>>>> Best regards,
> >>>>>> Alessandro
> >>>>>>
> >>>>>> On Fri, 9 Dec 2022 at 19:02, Stamatis Zampetakis <zabetak@gmail.com
> >>>>
> >>>>>> wrote:
> >>>>>>
> >>>>>>> Hi all,
> >>>>>>>
> >>>>>>> I just logged CALCITE-5427 [1] for introducing code quality &
> >>>> coverage
> >>>>>>> metrics in Calcite CI.
> >>>>>>>
> >>>>>>> I added some motivation and examples under the ticket so please
> >>> have
> >>>> a
> >>>>>> look
> >>>>>>> and let me know what you think.
> >>>>>>>
> >>>>>>> If there are no objections, I will follow-up with INFRA to set
> >>> things
> >>>>> up
> >>>>>>> for the official Calcite repo.
> >>>>>>>
> >>>>>>> The integration with SonarCloud has been inspired by HIVE-26196
> >>> [2]
> >>>>> that
> >>>>>>> Alessandro put in place for Hive and has been very helpful so far.
> >>>>>>>
> >>>>>>> Best,
> >>>>>>> Stamatis
> >>>>>>>
> >>>>>>> [1] https://issues.apache.org/jira/browse/CALCITE-5427
> >>>>>>> [2] https://issues.apache.org/jira/browse/HIVE-26196
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>>
> >>>>> --
> >>>>>
> >>>>> Best,
> >>>>> Benchao Li
> >>>>>
> >>>>
> >>>
> >>
> >

Re: [DISCUSS] Code quality/coverage with SonarCloud & JaCoCo

[Francis Chuang <fr...@apache.org>]

Thanks for implementing this, Stamatis! Having code quality metrics on 
our repos is a huge win.

Francis

On 10/01/2023 4:22 am, Stamatis Zampetakis wrote:
> I just merged the CALCITE-5427 [1] enabling Sonarcloud analysis for Calcite
> main branch and new PRs.
> 
> I have left the default Sonar quality gates active so you may start seeing
> Sonar reporting errors in various cases.
> 
> If you encounter problems or you would like things to work differently
> please create JIRA tickets and ping me if you need help.
> 
> Once we are happy with how things work for Calcite we can also port the
> changes to Avatica.
> 
> Note that all Calcite committers have administrative privileges to the
> Calcite project in Sonarcloud [2].
> 
> Best,
> Stamatis
> 
> [1]
> https://github.com/apache/calcite/commit/be7135cf1fd3d87e4036b2dd6e58d2f1251f8959
> [2] https://sonarcloud.io/project/roles?id=apache_calcite
> 
> On Wed, Jan 4, 2023 at 5:44 PM Stamatis Zampetakis <za...@gmail.com>
> wrote:
> 
>> The integration is advancing well and I am hoping to merge the PR in the
>> coming days.
>> To avoid unpleasant surprises, I am planning to create a new remote branch
>> in the main calcite repo to test some things out. I will delete it as soon
>> as I am done with the tests.
>>
>> Best.
>> Stamatis
>>
>> On Wed, Dec 28, 2022 at 2:47 PM Michael Mior <mm...@apache.org> wrote:
>>
>>> Thanks Stamatis! I haven't used SonarCloud before, but in general I think
>>> such tools can be quite useful.
>>>
>>> --
>>> Michael Mior
>>> mmior@apache.org
>>>
>>>
>>> On Sat, Dec 24, 2022 at 4:01 PM Stamatis Zampetakis <za...@gmail.com>
>>> wrote:
>>>
>>>> Since there were no objections, I just logged INFRA-24038 [1] and plan
>>> to
>>>> move this forward.
>>>>
>>>> Let me know if you have questions or concerns regarding the adoption of
>>>> SonarCloud.
>>>>
>>>> Best,
>>>> Stamatis
>>>>
>>>> [1] https://issues.apache.org/jira/browse/INFRA-24038
>>>>
>>>> On Sat, Dec 10, 2022 at 11:32 AM Benchao Li <li...@apache.org>
>>> wrote:
>>>>
>>>>> Thanks Stamatis for bringing this up.
>>>>>
>>>>> I haven't used Sonar yet, but thanks for the demo[1] you provided, it
>>>> looks
>>>>> really interesting. I think it's worth a try for Calcite.
>>>>>
>>>>> [1] https://github.com/zabetak/calcite/pull/9
>>>>>
>>>>> Alessandro Solimando <al...@gmail.com> 于2022年12月10日周六
>>>>> 02:54写道：
>>>>>
>>>>>> Hi all,
>>>>>> thanks Stamatis for the proposal and the work, I am a huge fan of
>>> Sonar
>>>>> and
>>>>>> I really miss it on Calcite, so a big +1 from me on this.
>>>>>>
>>>>>> In Hive so far we have received good feedback, so I hope it will be
>>>>>> welcomed here too.
>>>>>>
>>>>>> Best regards,
>>>>>> Alessandro
>>>>>>
>>>>>> On Fri, 9 Dec 2022 at 19:02, Stamatis Zampetakis <zabetak@gmail.com
>>>>
>>>>>> wrote:
>>>>>>
>>>>>>> Hi all,
>>>>>>>
>>>>>>> I just logged CALCITE-5427 [1] for introducing code quality &
>>>> coverage
>>>>>>> metrics in Calcite CI.
>>>>>>>
>>>>>>> I added some motivation and examples under the ticket so please
>>> have
>>>> a
>>>>>> look
>>>>>>> and let me know what you think.
>>>>>>>
>>>>>>> If there are no objections, I will follow-up with INFRA to set
>>> things
>>>>> up
>>>>>>> for the official Calcite repo.
>>>>>>>
>>>>>>> The integration with SonarCloud has been inspired by HIVE-26196
>>> [2]
>>>>> that
>>>>>>> Alessandro put in place for Hive and has been very helpful so far.
>>>>>>>
>>>>>>> Best,
>>>>>>> Stamatis
>>>>>>>
>>>>>>> [1] https://issues.apache.org/jira/browse/CALCITE-5427
>>>>>>> [2] https://issues.apache.org/jira/browse/HIVE-26196
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>>
>>>>> Best,
>>>>> Benchao Li
>>>>>
>>>>
>>>
>>
> 

Re: [DISCUSS] Code quality/coverage with SonarCloud & JaCoCo

[Stamatis Zampetakis <za...@gmail.com>]

I just merged the CALCITE-5427 [1] enabling Sonarcloud analysis for Calcite
main branch and new PRs.

I have left the default Sonar quality gates active so you may start seeing
Sonar reporting errors in various cases.

If you encounter problems or you would like things to work differently
please create JIRA tickets and ping me if you need help.

Once we are happy with how things work for Calcite we can also port the
changes to Avatica.

Note that all Calcite committers have administrative privileges to the
Calcite project in Sonarcloud [2].

Best,
Stamatis

[1]
https://github.com/apache/calcite/commit/be7135cf1fd3d87e4036b2dd6e58d2f1251f8959
[2] https://sonarcloud.io/project/roles?id=apache_calcite

On Wed, Jan 4, 2023 at 5:44 PM Stamatis Zampetakis <za...@gmail.com>
wrote:

> The integration is advancing well and I am hoping to merge the PR in the
> coming days.
> To avoid unpleasant surprises, I am planning to create a new remote branch
> in the main calcite repo to test some things out. I will delete it as soon
> as I am done with the tests.
>
> Best.
> Stamatis
>
> On Wed, Dec 28, 2022 at 2:47 PM Michael Mior <mm...@apache.org> wrote:
>
>> Thanks Stamatis! I haven't used SonarCloud before, but in general I think
>> such tools can be quite useful.
>>
>> --
>> Michael Mior
>> mmior@apache.org
>>
>>
>> On Sat, Dec 24, 2022 at 4:01 PM Stamatis Zampetakis <za...@gmail.com>
>> wrote:
>>
>> > Since there were no objections, I just logged INFRA-24038 [1] and plan
>> to
>> > move this forward.
>> >
>> > Let me know if you have questions or concerns regarding the adoption of
>> > SonarCloud.
>> >
>> > Best,
>> > Stamatis
>> >
>> > [1] https://issues.apache.org/jira/browse/INFRA-24038
>> >
>> > On Sat, Dec 10, 2022 at 11:32 AM Benchao Li <li...@apache.org>
>> wrote:
>> >
>> > > Thanks Stamatis for bringing this up.
>> > >
>> > > I haven't used Sonar yet, but thanks for the demo[1] you provided, it
>> > looks
>> > > really interesting. I think it's worth a try for Calcite.
>> > >
>> > > [1] https://github.com/zabetak/calcite/pull/9
>> > >
>> > > Alessandro Solimando <al...@gmail.com> 于2022年12月10日周六
>> > > 02:54写道：
>> > >
>> > > > Hi all,
>> > > > thanks Stamatis for the proposal and the work, I am a huge fan of
>> Sonar
>> > > and
>> > > > I really miss it on Calcite, so a big +1 from me on this.
>> > > >
>> > > > In Hive so far we have received good feedback, so I hope it will be
>> > > > welcomed here too.
>> > > >
>> > > > Best regards,
>> > > > Alessandro
>> > > >
>> > > > On Fri, 9 Dec 2022 at 19:02, Stamatis Zampetakis <zabetak@gmail.com
>> >
>> > > > wrote:
>> > > >
>> > > > > Hi all,
>> > > > >
>> > > > > I just logged CALCITE-5427 [1] for introducing code quality &
>> > coverage
>> > > > > metrics in Calcite CI.
>> > > > >
>> > > > > I added some motivation and examples under the ticket so please
>> have
>> > a
>> > > > look
>> > > > > and let me know what you think.
>> > > > >
>> > > > > If there are no objections, I will follow-up with INFRA to set
>> things
>> > > up
>> > > > > for the official Calcite repo.
>> > > > >
>> > > > > The integration with SonarCloud has been inspired by HIVE-26196
>> [2]
>> > > that
>> > > > > Alessandro put in place for Hive and has been very helpful so far.
>> > > > >
>> > > > > Best,
>> > > > > Stamatis
>> > > > >
>> > > > > [1] https://issues.apache.org/jira/browse/CALCITE-5427
>> > > > > [2] https://issues.apache.org/jira/browse/HIVE-26196
>> > > > >
>> > > >
>> > >
>> > >
>> > > --
>> > >
>> > > Best,
>> > > Benchao Li
>> > >
>> >
>>
>