zabetak commented on code in PR #3065:
URL: https://github.com/apache/calcite/pull/3065#discussion_r1107125176
##########
core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java:
##########
@@ -6562,10 +6562,10 @@ public interface Config {
Config withExplain(boolean explain);
/** Returns the {@code expand} option. Controls whether to expand
- * sub-queries. If false, each sub-query becomes a
+ * sub-queries. If false (the default), each sub-query becomes a
* {@link org.apache.calcite.rex.RexSubQuery}. */
@Value.Default default boolean isExpand() {
Review Comment:
Since we don't want people to expand using the `SqlToRelConverter` maybe we
should consider deprecating the method in the near future. As it is right now
it is not straight forward to understand that this code path is discouraged and
not easy as well to find the suggested alternatives for someone not involved in
the discussions.
##########
core/src/main/java/org/apache/calcite/prepare/Prepare.java:
##########
@@ -110,7 +110,7 @@ public abstract class Prepare {
* <p>The default is false, meaning do not expand queries during sql-to-rel,
* but a few tests override and set it to true. After CALCITE-1045
* is fixed, remove those overrides and use false everywhere. */
- public static final TryThreadLocal<@Nullable Boolean> THREAD_EXPAND =
+ public static final TryThreadLocal<Boolean> THREAD_EXPAND =
Review Comment:
If we remove all calls to `Prepare.THREAD_EXPAND` then maybe we can
remove/deprecate the constant altogether.
##########
core/src/test/resources/sql/misc.iq:
##########
@@ -466,8 +466,8 @@ EnumerableCalc(expr#0..6=[{inputs}], expr#7=[IS NULL($t6)],
proj#0..4=[{exprs}],
EnumerableSort(sort0=[$1], dir0=[ASC])
EnumerableTableScan(table=[[hr, emps]])
EnumerableSort(sort0=[$0], dir0=[ASC])
- EnumerableAggregate(group=[{0}], agg#0=[MIN($1)])
- EnumerableCalc(expr#0..3=[{inputs}], expr#4=[true], deptno=[$t0],
$f0=[$t4])
+ EnumerableCalc(expr#0=[{inputs}], expr#1=[true], proj#0..1=[{exprs}])
+ EnumerableAggregate(group=[{0}])
Review Comment:
Looks like a beneficial change in terms of performance; the `Calc` will be
applied on less rows than before (duplicates remoced).
On the other hand, the width of the input row to the aggregate does not play
a huge role assuming that redundant columns are pruned early (didn't check the
implementation).
##########
core/src/test/resources/sql/misc.iq:
##########
@@ -360,12 +360,12 @@ where exists (select 1 from "hr"."emps");
(3 rows)
!ok
-EnumerableNestedLoopJoin(condition=[true], joinType=[semi])
- EnumerableCalc(expr#0..3=[{inputs}], deptno=[$t0])
- EnumerableTableScan(table=[[hr, depts]])
- EnumerableCalc(expr#0=[{inputs}], expr#1=[IS NOT NULL($t0)], $f0=[$t0],
$condition=[$t1])
- EnumerableAggregate(group=[{}], agg#0=[MIN($0)])
- EnumerableCalc(expr#0..4=[{inputs}], expr#5=[true], $f0=[$t5])
+EnumerableCalc(expr#0..1=[{inputs}], deptno=[$t0])
+ EnumerableNestedLoopJoin(condition=[true], joinType=[inner])
Review Comment:
The semantics of the plan seem OK. The `joinType` change from `semi` to
`inner` is a bit worrisome but in this case that the right relation has only
one element it will not make any difference I guess.
##########
core/src/test/java/org/apache/calcite/util/UtilTest.java:
##########
@@ -2345,7 +2345,8 @@ private void checkListToString(String... strings) {
memo1.close();
assertThat(local1.get(), is("foo"));
- final TryThreadLocal<String> local2 = TryThreadLocal.of(null);
+ final TryThreadLocal<@org.checkerframework.checker.nullness.qual.Nullable
String> local2 =
Review Comment:
Full package qualifier shouldn't be necessary.
Actually we should remove `org.jetbrains.annotations.NotNull` and
`org.jetbrains.annotations.Nullable` from everywhere in the project; even here
I think they were added by mistake. We don't really need to do it here; if not
done I will raise a follow-up JIRA afterwards.
##########
core/src/test/java/org/apache/calcite/test/InterpreterTest.java:
##########
@@ -108,27 +110,36 @@ private static class Sql {
private final String sql;
private final SchemaPlus rootSchema;
private final boolean project;
- private final Function<RelBuilder, RelNode> relFn;
+ private final @Nullable Function<RelBuilder, RelNode> relFn;
+ private final UnaryOperator<SqlToRelConverter.Config> sqlToRelTransform;
Review Comment:
The tests in this class are only validating results and not plans. Wouldn't
be better to add the SubQueryRemoveRules somewhere in the fixture (e.g.,
`returnsRows`) and always expand as before? Like that we minimize calls to
`withExpand` and we shift our test coverage towards our recommended expansion
mechanism.
##########
testkit/src/main/java/org/apache/calcite/test/RelOptFixture.java:
##########
@@ -83,8 +83,8 @@ class RelOptFixture {
null, RelSupplier.NONE, null, null,
ImmutableMap.of(), (f, r) -> r, (f, r) -> r, false, false)
.withFactory(f ->
- f.withValidatorConfig(c ->
- c.withIdentifierExpansion(true)))
+ f.withValidatorConfig(c -> c.withIdentifierExpansion(true))
+ .withSqlToRelConfig(c -> c.withExpand(true)))
Review Comment:
There is a big bunch of tests using this fixture. I suppose you kept it
enabled to avoid many plan changes but like that we are essentially testing
more the non-suggested code path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
