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/06/14 07:38:54 UTC
[GitHub] [calcite] njriasan opened a new pull request #2435: [Calcite-4642] Fixed a bug in Planner where provided typeSystems were ignored
njriasan opened a new pull request #2435:
URL: https://github.com/apache/calcite/pull/2435
Fixes a bug in planner where the typeSystem being used wasn't the one provided by the config. Added a test demonstrating the support on typeSystems that have different behavior on unioning chars.
--
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] njriasan commented on pull request #2435: [Calcite-4642] Fixed a bug in Planner where provided typeSystems were ignored
Posted by GitBox <gi...@apache.org>.
njriasan commented on pull request #2435:
URL: https://github.com/apache/calcite/pull/2435#issuecomment-859787559
--
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] julianhyde commented on a change in pull request #2435: [CALCITE-4642] Use TypeSystem from Config in Planner
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #2435:
URL: https://github.com/apache/calcite/pull/2435#discussion_r662560116
##########
File path: core/src/test/java/org/apache/calcite/tools/PlannerTest.java
##########
@@ -1525,4 +1527,67 @@ private void checkView(String sql, Matcher<String> matcher)
final RelRoot root = planner.rel(validate);
assertThat(toString(root.rel), matcher);
}
+
+ /** Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-4642">[CALCITE-4642]
+ * Checks that custom type systems can be registered in a planner by
+ * comparing options for converting unions of chars.</a>.
+ */
+ @Test void testCustomTypeSystem() throws Exception {
+ final String sql = "select Case when DEPTNO <> 30 then 'hi' else 'world' end from dept";
+ final String expectedVarying = "LogicalProject("
+ + "EXPR$0=["
+ + "CASE(<>($0, 30),"
+ + " 'hi':VARCHAR(5), "
+ + "'world':VARCHAR(5))])\n"
+ + " LogicalValues("
+ + "tuples=[[{ 10, 'Sales' },"
+ + " { 20, 'Marketing' },"
+ + " { 30, 'Engineering' },"
+ + " { 40, 'Empty' }]])\n";
Review comment:
Yeah, that's valid. It's difficult to see the relational operators 'LogicalProject' and 'LogicalValues'. Sometimes I ensure that continuation lines have no preceding spaces at, which visually distinguishes them from the indented relational operators. Like this:
```
"LogicalProject(EXPR$0=[CASE(<>($0, 30), "
+ "'hi ', "
+ "'world')])\n"
+ " LogicalValues("
+ "tuples=[[{ 10, 'Sales ' }, "
+ "{ 20, 'Marketing ' }, "
+ "{ 30, 'Engineering' }, "
+ "{ 40, 'Empty ' }]])\n";
```
It works well in large, very-indented plans.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] julianhyde commented on a change in pull request #2435: [CALCITE-4642] Use TypeSystem from Config in Planner
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #2435:
URL: https://github.com/apache/calcite/pull/2435#discussion_r662554204
##########
File path: core/src/test/java/org/apache/calcite/tools/PlannerTest.java
##########
@@ -1525,4 +1527,67 @@ private void checkView(String sql, Matcher<String> matcher)
final RelRoot root = planner.rel(validate);
assertThat(toString(root.rel), matcher);
}
+
+ /** Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-4642">[CALCITE-4642]
+ * Checks that custom type systems can be registered in a planner by
+ * comparing options for converting unions of chars.</a>.
+ */
+ @Test void testCustomTypeSystem() throws Exception {
+ final String sql = "select Case when DEPTNO <> 30 then 'hi' else 'world' end from dept";
+ final String expectedVarying = "LogicalProject("
+ + "EXPR$0=["
+ + "CASE(<>($0, 30),"
+ + " 'hi':VARCHAR(5), "
+ + "'world':VARCHAR(5))])\n"
+ + " LogicalValues("
+ + "tuples=[[{ 10, 'Sales' },"
+ + " { 20, 'Marketing' },"
+ + " { 30, 'Engineering' },"
+ + " { 40, 'Empty' }]])\n";
Review comment:
I'm in favor of inserting line-breaks by hand to keep lines short. I think the previous is fine.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2435: [CALCITE-4642] Use TypeSystem from Config in Planner
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2435:
URL: https://github.com/apache/calcite/pull/2435#discussion_r662556820
##########
File path: core/src/test/java/org/apache/calcite/tools/PlannerTest.java
##########
@@ -1525,4 +1527,67 @@ private void checkView(String sql, Matcher<String> matcher)
final RelRoot root = planner.rel(validate);
assertThat(toString(root.rel), matcher);
}
+
+ /** Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-4642">[CALCITE-4642]
+ * Checks that custom type systems can be registered in a planner by
+ * comparing options for converting unions of chars.</a>.
+ */
+ @Test void testCustomTypeSystem() throws Exception {
+ final String sql = "select Case when DEPTNO <> 30 then 'hi' else 'world' end from dept";
+ final String expectedVarying = "LogicalProject("
+ + "EXPR$0=["
+ + "CASE(<>($0, 30),"
+ + " 'hi':VARCHAR(5), "
+ + "'world':VARCHAR(5))])\n"
+ + " LogicalValues("
+ + "tuples=[[{ 10, 'Sales' },"
+ + " { 20, 'Marketing' },"
+ + " { 30, 'Engineering' },"
+ + " { 40, 'Empty' }]])\n";
Review comment:
Ok, whatever. It took me a while to understand the plan, as I had to search for `\n`, so I asked.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] julianhyde commented on pull request #2435: [CALCITE-4642] Use TypeSystem from Config in Planner
Posted by GitBox <gi...@apache.org>.
julianhyde commented on pull request #2435:
URL: https://github.com/apache/calcite/pull/2435#issuecomment-859942767
I'd remove "Helper function that checks that", which is superfluous.
Change the last parameter from `String expectedRelExpr` to `Matcher<String> planMatcher` and people will start using the function as an assert.
Put the helper function before the class. We tend to put inner classes and interfaces last.
Anyway. These are very pedantic points. It's a nice contribution; thank you!
--
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] zabetak closed pull request #2435: [CALCITE-4642] Use TypeSystem from Config in Planner
Posted by GitBox <gi...@apache.org>.
zabetak closed pull request #2435:
URL: https://github.com/apache/calcite/pull/2435
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] njriasan commented on pull request #2435: [CALCITE-4642] Use TypeSystem from Config in Planner
Posted by GitBox <gi...@apache.org>.
njriasan commented on pull request #2435:
URL: https://github.com/apache/calcite/pull/2435#issuecomment-861140507
Thank you for the feedback everyone. I have made the desired changes. Could someone approve running the workflows?
--
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] njriasan commented on a change in pull request #2435: [CALCITE-4642] Use TypeSystem from Config in Planner
Posted by GitBox <gi...@apache.org>.
njriasan commented on a change in pull request #2435:
URL: https://github.com/apache/calcite/pull/2435#discussion_r665995049
##########
File path: core/src/test/java/org/apache/calcite/tools/PlannerTest.java
##########
@@ -1525,4 +1527,67 @@ private void checkView(String sql, Matcher<String> matcher)
final RelRoot root = planner.rel(validate);
assertThat(toString(root.rel), matcher);
}
+
+ /** Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-4642">[CALCITE-4642]
+ * Checks that custom type systems can be registered in a planner by
+ * comparing options for converting unions of chars.</a>.
+ */
+ @Test void testCustomTypeSystem() throws Exception {
+ final String sql = "select Case when DEPTNO <> 30 then 'hi' else 'world' end from dept";
+ final String expectedVarying = "LogicalProject("
+ + "EXPR$0=["
+ + "CASE(<>($0, 30),"
+ + " 'hi':VARCHAR(5), "
+ + "'world':VARCHAR(5))])\n"
+ + " LogicalValues("
+ + "tuples=[[{ 10, 'Sales' },"
+ + " { 20, 'Marketing' },"
+ + " { 30, 'Engineering' },"
+ + " { 40, 'Empty' }]])\n";
+ final String expectedDefault = "LogicalProject("
+ + "EXPR$0=["
+ + "CASE(<>($0, 30),"
+ + " 'hi ', "
+ + "'world')])\n"
+ + " LogicalValues("
+ + "tuples=[[{ 10, 'Sales ' },"
+ + " { 20, 'Marketing ' },"
+ + " { 30, 'Engineering' },"
+ + " { 40, 'Empty ' }]])\n";
Review comment:
Thanks for the suggestion. I'll merge this change to make it more readable.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] njriasan edited a comment on pull request #2435: [CALCITE-4642] Use TypeSystem from Config in Planner
Posted by GitBox <gi...@apache.org>.
njriasan edited a comment on pull request #2435:
URL: https://github.com/apache/calcite/pull/2435#issuecomment-872503573
@vlsi what is the remaining process for getting this PR merged?
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2435: [CALCITE-4642] Use TypeSystem from Config in Planner
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2435:
URL: https://github.com/apache/calcite/pull/2435#discussion_r662566263
##########
File path: core/src/test/java/org/apache/calcite/tools/PlannerTest.java
##########
@@ -1525,4 +1527,67 @@ private void checkView(String sql, Matcher<String> matcher)
final RelRoot root = planner.rel(validate);
assertThat(toString(root.rel), matcher);
}
+
+ /** Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-4642">[CALCITE-4642]
+ * Checks that custom type systems can be registered in a planner by
+ * comparing options for converting unions of chars.</a>.
+ */
+ @Test void testCustomTypeSystem() throws Exception {
+ final String sql = "select Case when DEPTNO <> 30 then 'hi' else 'world' end from dept";
+ final String expectedVarying = "LogicalProject("
+ + "EXPR$0=["
+ + "CASE(<>($0, 30),"
+ + " 'hi':VARCHAR(5), "
+ + "'world':VARCHAR(5))])\n"
+ + " LogicalValues("
+ + "tuples=[[{ 10, 'Sales' },"
+ + " { 20, 'Marketing' },"
+ + " { 30, 'Engineering' },"
+ + " { 40, 'Empty' }]])\n";
Review comment:
Exactly. I do see the reason to wrap the plan line after 200-300 chars, however, the current case is not worth the discussion.
I think non-wrapped is readable, and the wraps make it harder to read.
However, I do not object, and the rest in the patch looks great.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] njriasan commented on pull request #2435: [CALCITE-4642] Use TypeSystem from Config in Planner
Posted by GitBox <gi...@apache.org>.
njriasan commented on pull request #2435:
URL: https://github.com/apache/calcite/pull/2435#issuecomment-888999077
Can we merge this?
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] njriasan commented on pull request #2435: [CALCITE-4642] Use TypeSystem from Config in Planner
Posted by GitBox <gi...@apache.org>.
njriasan commented on pull request #2435:
URL: https://github.com/apache/calcite/pull/2435#issuecomment-872503573
@vlsi what is the process for getting a PR merged?
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2435: [CALCITE-4642] Use TypeSystem from Config in Planner
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2435:
URL: https://github.com/apache/calcite/pull/2435#discussion_r662551297
##########
File path: core/src/test/java/org/apache/calcite/tools/PlannerTest.java
##########
@@ -1525,4 +1527,67 @@ private void checkView(String sql, Matcher<String> matcher)
final RelRoot root = planner.rel(validate);
assertThat(toString(root.rel), matcher);
}
+
+ /** Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-4642">[CALCITE-4642]
+ * Checks that custom type systems can be registered in a planner by
+ * comparing options for converting unions of chars.</a>.
+ */
+ @Test void testCustomTypeSystem() throws Exception {
+ final String sql = "select Case when DEPTNO <> 30 then 'hi' else 'world' end from dept";
+ final String expectedVarying = "LogicalProject("
+ + "EXPR$0=["
+ + "CASE(<>($0, 30),"
+ + " 'hi':VARCHAR(5), "
+ + "'world':VARCHAR(5))])\n"
+ + " LogicalValues("
+ + "tuples=[[{ 10, 'Sales' },"
+ + " { 20, 'Marketing' },"
+ + " { 30, 'Engineering' },"
+ + " { 40, 'Empty' }]])\n";
+ final String expectedDefault = "LogicalProject("
+ + "EXPR$0=["
+ + "CASE(<>($0, 30),"
+ + " 'hi ', "
+ + "'world')])\n"
+ + " LogicalValues("
+ + "tuples=[[{ 10, 'Sales ' },"
+ + " { 20, 'Marketing ' },"
+ + " { 30, 'Engineering' },"
+ + " { 40, 'Empty ' }]])\n";
Review comment:
```suggestion
final String expectedDefault = ""
+ "LogicalProject(EXPR$0=[CASE(<>($0, 30), 'hi ', 'world')])\n"
+ " LogicalValues(tuples=[[{ 10, 'Sales ' }, { 20, 'Marketing ' }, { 30, 'Engineering' }, { 40, 'Empty ' }]])\n";
```
##########
File path: core/src/test/java/org/apache/calcite/tools/PlannerTest.java
##########
@@ -1525,4 +1527,67 @@ private void checkView(String sql, Matcher<String> matcher)
final RelRoot root = planner.rel(validate);
assertThat(toString(root.rel), matcher);
}
+
+ /** Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-4642">[CALCITE-4642]
+ * Checks that custom type systems can be registered in a planner by
+ * comparing options for converting unions of chars.</a>.
+ */
+ @Test void testCustomTypeSystem() throws Exception {
+ final String sql = "select Case when DEPTNO <> 30 then 'hi' else 'world' end from dept";
+ final String expectedVarying = "LogicalProject("
+ + "EXPR$0=["
+ + "CASE(<>($0, 30),"
+ + " 'hi':VARCHAR(5), "
+ + "'world':VARCHAR(5))])\n"
+ + " LogicalValues("
+ + "tuples=[[{ 10, 'Sales' },"
+ + " { 20, 'Marketing' },"
+ + " { 30, 'Engineering' },"
+ + " { 40, 'Empty' }]])\n";
Review comment:
```suggestion
final String expectedVarying = ""
+ "LogicalProject(EXPR$0=[CASE(<>($0, 30), 'hi':VARCHAR(5), 'world':VARCHAR(5))])\n"
+ " LogicalValues(tuples=[[{ 10, 'Sales' }, { 20, 'Marketing' }, { 30, 'Engineering' }, { 40, 'Empty' }]])\n";
```
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on pull request #2435: [Calcite-4642] Fixed a bug in Planner where provided typeSystems were ignored
Posted by GitBox <gi...@apache.org>.
vlsi commented on pull request #2435:
URL: https://github.com/apache/calcite/pull/2435#issuecomment-859871875
Please use upper case for `[Calcite-4642]`, and please remove `fixed a bug` from the commit title/PR title.
In practice, `fixed a bug` adds little value, and it would be better to spend the letters explaining what is changed.
For instance: `Use TypeSystem from Config in Planner`
--
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] njriasan commented on pull request #2435: [CALCITE-4642] Use TypeSystem from Config in Planner
Posted by GitBox <gi...@apache.org>.
njriasan commented on pull request #2435:
URL: https://github.com/apache/calcite/pull/2435#issuecomment-861722334
It looks like all the workflows are passing. Are there any additional steps I need to take in order to get this PR merged?
--
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] vlsi commented on a change in pull request #2435: [Calcite-4642] Fixed a bug in Planner where provided typeSystems were ignored
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2435:
URL: https://github.com/apache/calcite/pull/2435#discussion_r650171208
##########
File path: core/src/test/java/org/apache/calcite/tools/PlannerTest.java
##########
@@ -1525,4 +1527,65 @@ private void checkView(String sql, Matcher<String> matcher)
final RelRoot root = planner.rel(validate);
assertThat(toString(root.rel), matcher);
}
+
+ /**
+ * Helper function that checks that a Planner uses the correct type system when provided.
+ */
+ private void checkTypeSystem(String sql, RelDataTypeSystem typeSystem, String expectedRelExpr)
Review comment:
Small notes:
1. The naming does not reflect the actual implementation.
The better name would be `assertValidatedPlan`.
2. It is better place the method _after_ the test method, so the code is easier to read from top to bottom (higher-level methods shoud come before low-level ones, especially when lower-level methods have just one usage)
--
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] njriasan edited a comment on pull request #2435: [CALCITE-4642] Use TypeSystem from Config in Planner
Posted by GitBox <gi...@apache.org>.
njriasan edited a comment on pull request #2435:
URL: https://github.com/apache/calcite/pull/2435#issuecomment-872503573
@vlsi what is the process for getting this PR merged?
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org