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

2023-01-12 Thread Julian Hyde 
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=CRITICAL=CODE_SMELL=2942=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
 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,  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  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 
> > > 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.
>

Jenkins build is back to normal : Calcite » Calcite-snapshots #319

2023-01-12 Thread Apache Jenkins Server 
See

Build failed in Jenkins: Calcite » Calcite-snapshots #318

2023-01-12 Thread Apache Jenkins Server 
See 


Changes:

[Julian Hyde] [CALCITE-5405] MongoDB adapter does not parse dates correctly


--
[...truncated 326.85 KB...]
  9.6sec, 
org.apache.calcite.adapter.geode.rel.GeodeBookstoreTest > 
testSortWithProjection()
  9.6sec, 
org.apache.calcite.adapter.geode.rel.GeodeBookstoreTest > 
testCount()
  9.7sec, 
org.apache.calcite.adapter.geode.rel.GeodeBookstoreTest > 
testAddMissingGroupByColumnToProjectedFields()
  9.9sec, 
org.apache.calcite.adapter.geode.rel.GeodeBookstoreTest > 
testFilterWithNestedField()
  9.9sec, 
org.apache.calcite.adapter.geode.rel.GeodeBookstoreTest > 
testWhereWithOr()
 10.0sec, 
org.apache.calcite.adapter.geode.rel.GeodeAllDataTypesTest > 
testSqlSingleTimestampWhereFilter()
 10.1sec, 
org.apache.calcite.adapter.geode.rel.GeodeAllDataTypesTest > 
testSqlSingleTimeWhereFilter()
 10.3sec, 
org.apache.calcite.adapter.geode.rel.GeodeAllDataTypesTest > 
testSqlSingleDateWhereFilter()
 28.8sec,   12 completed,   0 failed,   0 skipped, 
org.apache.calcite.adapter.geode.rel.GeodeAllDataTypesTest
  2.2sec, 
org.apache.calcite.adapter.geode.rel.GeodeBookstoreTest > 
testMaxMinSumAvgInGroupBy()
  2.1sec, 
org.apache.calcite.adapter.geode.rel.GeodeBookstoreTest > 
testMaxMinSumAvg()
  3.2sec, 
org.apache.calcite.adapter.geode.rel.GeodeZipsTest > 
testWhereWithOrForLargeValueList()
  2.6sec, 
org.apache.calcite.adapter.geode.rel.GeodeBookstoreTest > 
testWhereWithOrAnd()
 31.0sec,   36 completed,   0 failed,   0 skipped, 
org.apache.calcite.adapter.geode.rel.GeodeBookstoreTest
  3.2sec, 
org.apache.calcite.adapter.geode.rel.GeodeZipsTest > 
testItemPredicate()
WARNING  31.3sec,   14 completed,   0 failed,   3 
skipped, org.apache.calcite.adapter.geode.rel.GeodeZipsTest
WARNING  38.1sec,   62 completed,   0 failed,   3 
skipped, Gradle Test Run :geode:test

> Task :testkit:test
  6.9sec, org.apache.calcite.test.FixtureTest > 
testRuleFixtureNeedsDiffRepos()
  7.2sec, org.apache.calcite.test.FixtureTest > 
testMetadata()
  8.3sec, org.apache.calcite.test.FixtureTest > 
testRuleFixture()
  9.5sec, org.apache.calcite.test.FixtureTest > 
testOperatorFixture()
  9.7sec,9 completed,   0 failed,   0 skipped, 
org.apache.calcite.test.FixtureTest
 13.9sec,   16 completed,   0 failed,   0 skipped, Gradle Test Run 
:testkit:test

> Task :redis:test
docker[redis:2.8.19] 2023-01-13 00:59:44,507 
[docker-java-stream--1133810895] INFO  - Starting to pull image
docker[redis:2.8.19] 2023-01-13 00:59:44,525 
[docker-java-stream--1133810895] INFO  - Pulling image layers:  0 pending,  0 
downloaded,  0 extracted, (0 bytes/0 bytes)
docker[redis:2.8.19] 2023-01-13 00:59:44,835 
[docker-java-stream--1133810895] INFO  - Pulling image layers:  8 pending,  1 
downloaded,  0 extracted, (32 bytes/? MB)
docker[redis:2.8.19] 2023-01-13 00:59:44,846 
[docker-java-stream--1133810895] INFO  - Pulling image layers:  7 pending,  2 
downloaded,  0 extracted, (1 KB/? MB)
docker[redis:2.8.19] 2023-01-13 00:59:45,134 
[docker-java-stream--1133810895] INFO  - Pulling image layers:  6 pending,  3 
downloaded,  0 extracted, (9 MB/? MB)
docker[redis:2.8.19] 2023-01-13 00:59:45,475 
[docker-java-stream--1133810895] INFO  - Pulling image layers:  5 pending,  4 
downloaded,  0 extracted, (24 MB/? MB)
docker[redis:2.8.19] 2023-01-13 00:59:45,640 
[docker-java-stream--1133810895] INFO  - Pulling image layers:  4 pending,  5 
downloaded,  0 extracted, (34 MB/? MB)
docker[redis:2.8.19] 2023-01-13 00:59:45,873 
[docker-java-stream--1133810895] INFO  - Pulling image layers:  3 pending,  6 
downloaded,  0 extracted, (34 MB/? MB)
docker[redis:2.8.19] 2023-01-13 00:59:45,930 
[docker-java-stream--1133810895] INFO  - Pulling image layers:  2 pending,  7 
downloaded,  0 extracted, (36 MB/? MB)
docker[redis:2.8.19] 2023-01-13 00:59:46,073 
[docker-java-stream--1133810895] INFO  - Pulling image layers:  1 pending,  8 
downloaded,  0 extracted, (39 MB/? MB)
docker[redis:2.8.19] 2023-01-13 00:59:46,165 
[docker-java-stream--1133810895] INFO  - Pulling image layers:  0 pending,  9 
downloaded,  0 extracted, (39 MB/46 MB)

>

[jira] [Created] (CALCITE-5473) Keep more collations in RelMdCollation#project for monotonic RexCalls

2023-01-12 Thread Thomas Rebele (Jira) 
Thomas Rebele created CALCITE-5473:
--

 Summary: Keep more collations in RelMdCollation#project for 
monotonic RexCalls
 Key: CALCITE-5473
 URL: https://issues.apache.org/jira/browse/CALCITE-5473
 Project: Calcite
  Issue Type: Improvement
Reporter: Thomas Rebele


Some RexCalls do not affect the collation, i.e., $0+10 has the same collation 
as $0 (if there are now integer overflows). This is already implemented in 
{{{}SqlMonotonicBinaryOperator#getMonotonicity{}}}.

If we have an input with collation [0,1], and a {{{}Calc(a=$0+10, b=$1){}}}, 
then the output collation would be [0, 1] as well. This is the case for 
STRICTLY_INCREASING or STRICTLY_DECREASING calls (though the direction of the 
field collation might need to be adapted).

However, if the Calc was {{{}Calc(a=FLOOR($0), b=$1){}}}, then the output 
collation would be just [0], as the sortedness of b is not guaranteed. This 
happens for example with rows (0.2, 50) and (0.3, 20) and (0.4, 30), in that 
order. So for INCREASING, DECREASING, or CONSTANT the collation would be 
shortened.

The right place to implement this would probably be 
{{{}RelMdCollation#project{}}}. The {{fieldCollationsForRexCalls}} loop would 
need to stay, because some calls may introduce a collation, regardless of the 
input. E.g., {{SELECT CURRENT_DATE FROM some_random_table}} is always sorted.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

[jira] [Created] (CALCITE-5472) RexCallBinding#getOperandMonotonicity should return CONSTANT for RexLiterals

2023-01-12 Thread Thomas Rebele (Jira) 
Thomas Rebele created CALCITE-5472:
--

 Summary: RexCallBinding#getOperandMonotonicity should return 
CONSTANT for RexLiterals
 Key: CALCITE-5472
 URL: https://issues.apache.org/jira/browse/CALCITE-5472
 Project: Calcite
  Issue Type: Task
Reporter: Thomas Rebele






--
This message was sent by Atlassian Jira
(v8.20.10#820010)

[jira] [Created] (CALCITE-5471) RelSupplier.SqlRelSupplier#apply should use .project(), not .rel

2023-01-12 Thread Thomas Rebele (Jira) 
Thomas Rebele created CALCITE-5471:
--

 Summary: RelSupplier.SqlRelSupplier#apply should use .project(), 
not .rel
 Key: CALCITE-5471
 URL: https://issues.apache.org/jira/browse/CALCITE-5471
 Project: Calcite
  Issue Type: Task
Reporter: Thomas Rebele


RelSupplier.SqlRelSupplier#apply should use .project(), not .rel, otherwise the 
result has columns that are not part of the SELECT clause.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

[jira] [Created] (CALCITE-5470) VolcanoPlanner removes a necessary sort

2023-01-12 Thread Thomas Rebele (Jira) 
Thomas Rebele created CALCITE-5470:
--

 Summary: VolcanoPlanner removes a necessary sort
 Key: CALCITE-5470
 URL: https://issues.apache.org/jira/browse/CALCITE-5470
 Project: Calcite
  Issue Type: Bug
  Components: core
Affects Versions: 1.32.0
Reporter: Thomas Rebele


A logical plan that roughly corresponds to SELECT a*a FROM 
(VALUES(-10),(2),(3)) T(a) ORDER BY a*a produces a physical plan that is not 
sorted:
{noformat}
logical plan:
LogicalSort(sort0=[$0], dir0=[ASC])
  LogicalProject($f0=[*($0, $0)])
    LogicalValues(tuples=[[{ -10 }, { 2 }, { 3 }]])
{noformat}
The result should be 4, 9, 100.
{noformat}
physical plan:
EnumerableProject($f0=[*($0, $0)])
  EnumerableValues(tuples=[[{ -10 }, { 2 }, { 3 }]]){noformat}
If I understand the physical plan correctly, its result would be 100, 4, 9.

Using sqlline gives the correct result (see attached log), so the problem could 
be in the test itself.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

[jira] [Created] (CALCITE-5469) Implement BigQuery DATETIME_ADD/DATETIME_DIFF

2023-01-12 Thread Tanner Clary (Jira) 
Tanner Clary created CALCITE-5469:
-

 Summary: Implement BigQuery DATETIME_ADD/DATETIME_DIFF
 Key: CALCITE-5469
 URL: https://issues.apache.org/jira/browse/CALCITE-5469
 Project: Calcite
  Issue Type: Sub-task
Reporter: Tanner Clary
Assignee: Tanner Clary


Add support for BigQuery's {{DATETIME_ADD/DATETIME_DIFF}} functions.

{{DATETIME_ADD(datetime, interval)}} can accept a timestamp (or a datetime, 
which is an alias for timestamp) for its first argument and an interval for its 
second. The output is the datetime that occurs {{interval}} after the provided 
{{datetime}}.

{{DATETIME_DIFF(datetime, datetime2, timeUnit)}} returns the whole number of 
{{timeUnit}} between {{datetime}} and {{datetime2}}, with the result being 
negative if {{datetime}} occurs before {{datetime2}}

Examples:
{{DATETIME_ADD(TIMESTAMP '2008-12-25 15:30:00', INTERVAL 5 MINUTE)}} would 
return: '2008-12-25 15:35:00'.

{{DATETIME_DIFF(DATETIME '2008-12-25 15:30:00', DATETIME '2008-12-26 15:30:00', 
DAY)}} would return: -1.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

Why we cannot get all predicates for outer join query?

2023-01-12 Thread Jiajun Xie 
Hello, all.

I try to use RelMetadataQuery#getAllPredicates get predicate,
but I get null for outer join query that left column name is same as right
column name.
```
final RelNode rel = sql("select name as dname from emp left outer join dept"
+ " on emp.deptno = dept.deptno").toRel();
final RelMetadataQuery mq = rel.getCluster().getMetadataQuery();
final RelOptPredicateList r = mq.getAllPredicates(rel);
assertNull(r);
```


After commenting on two pieces of code:
1. RelMdAllPredicates#getAllPredicates
```
if (join.getJoinType().isOuterJoin()) {
  // We cannot map origin of this expression.
  return null;
}
2. RelMdExpressionLineage#getExpressionLineage
```
if (rel.getJoinType().isOuterJoin()) {
  // If we reference the inner side, we will bail out
  if (rel.getJoinType() == JoinRelType.LEFT) {
ImmutableBitSet rightFields = ImmutableBitSet.range(
nLeftColumns, rel.getRowType().getFieldCount());
if (inputFieldsUsed.intersects(rightFields)) {
  // We cannot map origin of this expression.
  return null;
}
  } else if (rel.getJoinType() == JoinRelType.RIGHT) {
ImmutableBitSet leftFields = ImmutableBitSet.range(
0, nLeftColumns);
if (inputFieldsUsed.intersects(leftFields)) {
  // We cannot map origin of this expression.
  return null;
}
  } else {
// We cannot map origin of this expression.
return null;
  }
}
I can get the results I need
```
final RelNode rel = sql("select name as dname from emp left outer join dept"
+ " on emp.deptno = dept.deptno").toRel();
final RelMetadataQuery mq = rel.getCluster().getMetadataQuery();
final RelOptPredicateList r = mq.getAllPredicates(rel);
assertThat(r.pulledUpPredicates.get(0).toString(),
equalTo("=([CATALOG, SALES, EMP].#0.$7, [CATALOG, SALES,
DEPT].#0.$0)"));
```


It seems that we deliberately return null
in RelMetadataQuery#getAllPredicates . Can someone tell me why? Thanks!