You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2021/03/26 10:00:23 UTC
[GitHub] [calcite] jibiyr opened a new pull request #2382: [CALCITE-4548] Support convert subQuery for SqlToRelConverter#convertExpression (jibiyr)
jibiyr opened a new pull request #2382:
URL: https://github.com/apache/calcite/pull/2382
the public entry point(SqlToRelConverter#convertExpression) should support to convert InWithConstantList
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jibiyr commented on a change in pull request #2382: [CALCITE-4548] Support convert subQuery for SqlToRelConverter#convertExpression (jibiyr)
Posted by GitBox <gi...@apache.org>.
jibiyr commented on a change in pull request #2382:
URL: https://github.com/apache/calcite/pull/2382#discussion_r606561287
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelTestBase.java
##########
@@ -657,6 +664,32 @@ public RelNode trimRelNode(RelNode relNode) {
return relNode;
}
+ private Pair<SqlToRelConverter, SqlValidator> createSqlToRelConverter() {
+ final RelDataTypeFactory typeFactory = getTypeFactory();
+ final Prepare.CatalogReader catalogReader =
Review comment:
@danny0405 Dear danny, I have made the change according to our discussion. I'm sorry to bother you on vacation
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #2382: [CALCITE-4548] Support convert subQuery for SqlToRelConverter#convertExpression (jibiyr)
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2382:
URL: https://github.com/apache/calcite/pull/2382#discussion_r604543460
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelTestBase.java
##########
@@ -169,6 +170,13 @@ public static void assertValid(RelNode rel) {
*/
RelRoot convertSqlToRel(String sql);
+ /**
+ * Converts a expression string to {@link RexNode}.
+ *
+ * @param expr expression
+ */
+ RexNode convertExpressionToRel(String expr);
+
Review comment:
`convertExpressionToRel` -> `convertExpressionToRex`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jibiyr commented on a change in pull request #2382: [CALCITE-4548] Support convert subQuery for SqlToRelConverter#convertExpression (jibiyr)
Posted by GitBox <gi...@apache.org>.
jibiyr commented on a change in pull request #2382:
URL: https://github.com/apache/calcite/pull/2382#discussion_r613178973
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelTestBase.java
##########
@@ -770,7 +795,32 @@ public void assertConvertsTo(
assertConvertsTo(sql, plan, false);
}
+ public void assertConvertsTo(String sql,
+ String plan,
+ boolean trim) {
+ assertConvertsTo(sql, plan, false, true);
+ }
+
public void assertConvertsTo(
+ String sql,
+ String plan,
+ boolean trim,
+ boolean query) {
+ if (query) {
+ assertSqlConvertsTo(sql, plan, trim);
+ } else {
+ assertExprConvertsTo(sql);
+ }
+ }
+
+ private void assertExprConvertsTo(
+ String expr) {
Review comment:
Thanks @danny0405 , I had fixed my idea codeStyle. and I added a test case which is named testInWithConstantList in SqlToRelConverterTest . It can cover the code which I made change.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #2382: [CALCITE-4548] Support convert subQuery for SqlToRelConverter#convertExpression (jibiyr)
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2382:
URL: https://github.com/apache/calcite/pull/2382#discussion_r606048187
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelTestBase.java
##########
@@ -169,6 +170,13 @@ public static void assertValid(RelNode rel) {
*/
RelRoot convertSqlToRel(String sql);
+ /**
+ * Converts a expression string to {@link RexNode}.
+ *
+ * @param expr expression
Review comment:
param comments should start with uppercase character. `expression` => `The expression`.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jibiyr commented on a change in pull request #2382: [CALCITE-4548] Support convert subQuery for SqlToRelConverter#convertExpression (jibiyr)
Posted by GitBox <gi...@apache.org>.
jibiyr commented on a change in pull request #2382:
URL: https://github.com/apache/calcite/pull/2382#discussion_r602884116
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java
##########
@@ -4223,10 +4234,12 @@ public void visit(RelNode node, int ordinal, @Nullable RelNode parent) {
private final boolean trim;
private final UnaryOperator<SqlToRelConverter.Config> config;
private final SqlConformance conformance;
+ private final boolean query;
+
Review comment:
I want to use it to mark the input string , like the inner class Sql of SqlValidatorTest used.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #2382: [CALCITE-4548] Support convert subQuery for SqlToRelConverter#convertExpression (jibiyr)
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2382:
URL: https://github.com/apache/calcite/pull/2382#discussion_r606046774
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelTestBase.java
##########
@@ -169,6 +170,13 @@ public static void assertValid(RelNode rel) {
*/
RelRoot convertSqlToRel(String sql);
+ /**
+ * Converts a expression string to {@link RexNode}.
+ *
+ * @param expr expression
Review comment:
a => an
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jibiyr commented on a change in pull request #2382: [CALCITE-4548] Support convert subQuery for SqlToRelConverter#convertExpression (jibiyr)
Posted by GitBox <gi...@apache.org>.
jibiyr commented on a change in pull request #2382:
URL: https://github.com/apache/calcite/pull/2382#discussion_r606047590
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelTestBase.java
##########
@@ -169,6 +170,13 @@ public static void assertValid(RelNode rel) {
*/
RelRoot convertSqlToRel(String sql);
+ /**
+ * Converts a expression string to {@link RexNode}.
+ *
+ * @param expr expression
Review comment:
Thank for you reminding me
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #2382: [CALCITE-4548] Support convert subQuery for SqlToRelConverter#convertExpression (jibiyr)
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2382:
URL: https://github.com/apache/calcite/pull/2382#discussion_r606046774
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelTestBase.java
##########
@@ -169,6 +170,13 @@ public static void assertValid(RelNode rel) {
*/
RelRoot convertSqlToRel(String sql);
+ /**
+ * Converts a expression string to {@link RexNode}.
+ *
+ * @param expr expression
Review comment:
a => an, param comments should start with uppercase character.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #2382: [CALCITE-4548] Support convert subQuery for SqlToRelConverter#convertExpression (jibiyr)
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2382:
URL: https://github.com/apache/calcite/pull/2382#discussion_r602882842
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java
##########
@@ -4223,10 +4234,12 @@ public void visit(RelNode node, int ordinal, @Nullable RelNode parent) {
private final boolean trim;
private final UnaryOperator<SqlToRelConverter.Config> config;
private final SqlConformance conformance;
+ private final boolean query;
+
Review comment:
Why we add a flag there ? Can we eliminate that.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #2382: [CALCITE-4548] Support convert subQuery for SqlToRelConverter#convertExpression (jibiyr)
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2382:
URL: https://github.com/apache/calcite/pull/2382#discussion_r612921434
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelTestBase.java
##########
@@ -592,26 +617,11 @@ public RelRoot convertSqlToRel(String sql) {
}
final RelDataTypeFactory typeFactory = getTypeFactory();
final Prepare.CatalogReader catalogReader =
- createCatalogReader(typeFactory);
+ createCatalogReader(typeFactory);
Review comment:
Fix the indentation.
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelTestBase.java
##########
@@ -770,7 +795,32 @@ public void assertConvertsTo(
assertConvertsTo(sql, plan, false);
}
+ public void assertConvertsTo(String sql,
+ String plan,
Review comment:
Fix the indentation.
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelTestBase.java
##########
@@ -215,10 +223,25 @@ void assertConvertsTo(
* @param plan Expected plan
* @param trim Whether to trim columns that are not needed
*/
+ void assertConvertsTo(
+ String sql,
Review comment:
Fix the indentation.
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelTestBase.java
##########
@@ -636,27 +646,35 @@ public RelNode trimRelNode(RelNode relNode) {
final SqlValidator validator =
createValidator(
catalogReader, typeFactory);
- final Context context = getContext();
- final CalciteConnectionConfig calciteConfig =
- context.unwrap(CalciteConnectionConfig.class);
- if (calciteConfig != null) {
- validator.transform(config ->
- config.withDefaultNullCollation(calciteConfig.defaultNullCollation()));
- }
- final SqlToRelConverter.Config config =
- configTransform.apply(SqlToRelConverter.config());
final SqlToRelConverter converter =
createSqlToRelConverter(
validator,
- catalogReader,
- typeFactory,
- config);
+ catalogReader);
relNode = converter.flattenTypes(relNode, true);
relNode = converter.trimUnusedFields(true, relNode);
return relNode;
}
+ private SqlToRelConverter createSqlToRelConverter(SqlValidator validator,
+ Prepare.CatalogReader catalogReader) {
Review comment:
Fix the indentation.
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelTestBase.java
##########
@@ -770,7 +795,32 @@ public void assertConvertsTo(
assertConvertsTo(sql, plan, false);
}
+ public void assertConvertsTo(String sql,
+ String plan,
+ boolean trim) {
+ assertConvertsTo(sql, plan, false, true);
+ }
+
public void assertConvertsTo(
+ String sql,
+ String plan,
+ boolean trim,
+ boolean query) {
+ if (query) {
+ assertSqlConvertsTo(sql, plan, trim);
+ } else {
+ assertExprConvertsTo(sql);
+ }
+ }
+
+ private void assertExprConvertsTo(
+ String expr) {
Review comment:
```java
private void assertExprConvertsTo(
String expr,
String plan) {
String expr2 = getDiffRepos().expand("sql", expr);
RexNode rex = convertExprToRex(expr2);
assertNotNull(rex);
// NOTE jvs 28-Mar-2006: insert leading newline so
// that plans come out nicely stacked instead of first
// line immediately after CDATA start
String actual = NL + rex.toString();
diffRepos.assertEquals("plan", plan, actual);
}
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jibiyr commented on a change in pull request #2382: [CALCITE-4548] Support convert subQuery for SqlToRelConverter#convertExpression (jibiyr)
Posted by GitBox <gi...@apache.org>.
jibiyr commented on a change in pull request #2382:
URL: https://github.com/apache/calcite/pull/2382#discussion_r604544104
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelTestBase.java
##########
@@ -214,11 +222,13 @@ void assertConvertsTo(
* @param sql SQL query
* @param plan Expected plan
* @param trim Whether to trim columns that are not needed
+ * @param query True if {@code sql} is a query, false if it is an expression
*/
void assertConvertsTo(
String sql,
String plan,
- boolean trim);
+ boolean trim,
+ boolean query);
Review comment:
yes, maybe it is better to add param
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #2382: [CALCITE-4548] Support convert subQuery for SqlToRelConverter#convertExpression (jibiyr)
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2382:
URL: https://github.com/apache/calcite/pull/2382#discussion_r606178577
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelTestBase.java
##########
@@ -657,6 +664,32 @@ public RelNode trimRelNode(RelNode relNode) {
return relNode;
}
+ private Pair<SqlToRelConverter, SqlValidator> createSqlToRelConverter() {
+ final RelDataTypeFactory typeFactory = getTypeFactory();
+ final Prepare.CatalogReader catalogReader =
Review comment:
No, we better put the validator creation and converter creation in 2 methods.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #2382: [CALCITE-4548] Support convert subQuery for SqlToRelConverter#convertExpression (jibiyr)
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2382:
URL: https://github.com/apache/calcite/pull/2382#discussion_r604543460
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelTestBase.java
##########
@@ -169,6 +170,13 @@ public static void assertValid(RelNode rel) {
*/
RelRoot convertSqlToRel(String sql);
+ /**
+ * Converts a expression string to {@link RexNode}.
+ *
+ * @param expr expression
+ */
+ RexNode convertExpressionToRel(String expr);
+
Review comment:
`convertExpressionToRel` -> `convertExprToRex`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #2382: [CALCITE-4548] Support convert subQuery for SqlToRelConverter#convertExpression (jibiyr)
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2382:
URL: https://github.com/apache/calcite/pull/2382#discussion_r604543714
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelTestBase.java
##########
@@ -214,11 +222,13 @@ void assertConvertsTo(
* @param sql SQL query
* @param plan Expected plan
* @param trim Whether to trim columns that are not needed
+ * @param query True if {@code sql} is a query, false if it is an expression
*/
void assertConvertsTo(
String sql,
String plan,
- boolean trim);
+ boolean trim,
+ boolean query);
Review comment:
Add a new method seems more reasonable to me.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #2382: [CALCITE-4548] Support convert subQuery for SqlToRelConverter#convertExpression (jibiyr)
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2382:
URL: https://github.com/apache/calcite/pull/2382#discussion_r606047402
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelTestBase.java
##########
@@ -796,6 +853,43 @@ public void assertConvertsTo(
diffRepos.assertEquals("plan", plan, actual);
}
+ public RexNode convertExprToRex(String expr) {
+ Objects.requireNonNull(expr, "expr");
+ final SqlNode sqlQuery;
+ try {
+ sqlQuery = parseExpression(expr);
+ } catch (RuntimeException | Error e) {
+ throw e;
+ } catch (Exception e) {
+ throw TestUtil.rethrow(e);
+ }
+ final RelDataTypeFactory typeFactory = getTypeFactory();
+ final Prepare.CatalogReader catalogReader =
Review comment:
Can we reuse the logic for creating `SqlToRelConverter` ?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jibiyr commented on a change in pull request #2382: [CALCITE-4548] Support convert subQuery for SqlToRelConverter#convertExpression (jibiyr)
Posted by GitBox <gi...@apache.org>.
jibiyr commented on a change in pull request #2382:
URL: https://github.com/apache/calcite/pull/2382#discussion_r604549387
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelTestBase.java
##########
@@ -214,11 +222,13 @@ void assertConvertsTo(
* @param sql SQL query
* @param plan Expected plan
* @param trim Whether to trim columns that are not needed
+ * @param query True if {@code sql} is a query, false if it is an expression
*/
void assertConvertsTo(
String sql,
String plan,
- boolean trim);
+ boolean trim,
+ boolean query);
Review comment:
Dear danny , I have made a change
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 closed pull request #2382: [CALCITE-4548] Support convert subQuery for SqlToRelConverter#convertExpression (jibiyr)
Posted by GitBox <gi...@apache.org>.
danny0405 closed pull request #2382:
URL: https://github.com/apache/calcite/pull/2382
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jibiyr commented on a change in pull request #2382: [CALCITE-4548] Support convert subQuery for SqlToRelConverter#convertExpression (jibiyr)
Posted by GitBox <gi...@apache.org>.
jibiyr commented on a change in pull request #2382:
URL: https://github.com/apache/calcite/pull/2382#discussion_r606052282
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelTestBase.java
##########
@@ -796,6 +853,43 @@ public void assertConvertsTo(
diffRepos.assertEquals("plan", plan, actual);
}
+ public RexNode convertExprToRex(String expr) {
+ Objects.requireNonNull(expr, "expr");
+ final SqlNode sqlQuery;
+ try {
+ sqlQuery = parseExpression(expr);
+ } catch (RuntimeException | Error e) {
+ throw e;
+ } catch (Exception e) {
+ throw TestUtil.rethrow(e);
+ }
+ final RelDataTypeFactory typeFactory = getTypeFactory();
+ final Prepare.CatalogReader catalogReader =
Review comment:
The duplicate code build both validator and SqlToRelConverter . Is it a good idea to let this code return Pair.of(SqlToRelConveter,SqlValidator)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jibiyr commented on a change in pull request #2382: [CALCITE-4548] Support convert subQuery for SqlToRelConverter#convertExpression (jibiyr)
Posted by GitBox <gi...@apache.org>.
jibiyr commented on a change in pull request #2382:
URL: https://github.com/apache/calcite/pull/2382#discussion_r602884668
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java
##########
@@ -4223,10 +4234,12 @@ public void visit(RelNode node, int ordinal, @Nullable RelNode parent) {
private final boolean trim;
private final UnaryOperator<SqlToRelConverter.Config> config;
private final SqlConformance conformance;
+ private final boolean query;
+
Review comment:
and we can use like **expr().ok** to test expression later. so I think maybe we need the flag. and thanks for your reply danny
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jibiyr commented on a change in pull request #2382: [CALCITE-4548] Support convert subQuery for SqlToRelConverter#convertExpression (jibiyr)
Posted by GitBox <gi...@apache.org>.
jibiyr commented on a change in pull request #2382:
URL: https://github.com/apache/calcite/pull/2382#discussion_r603795725
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java
##########
@@ -4223,10 +4234,12 @@ public void visit(RelNode node, int ordinal, @Nullable RelNode parent) {
private final boolean trim;
private final UnaryOperator<SqlToRelConverter.Config> config;
private final SqlConformance conformance;
+ private final boolean query;
+
Review comment:
@danny0405 Please take a look. Thanks.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org