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 2022/02/02 00:59:00 UTC
[GitHub] [calcite] mkou opened a new pull request #2709: [CALCITE-4996] in RelJson, add a new public toRex method which can ov…
mkou opened a new pull request #2709:
URL: https://github.com/apache/calcite/pull/2709
…erride makeInputRef
/!!\ DO NOT MERGE THIS IS A WORK IN PROGRESS
Update: Unless I am mistaken I cannot find anywhere where the `toRex` method on the RelInput is tested so I don't feel very confident in pushing this change and am a bit confused as where I should add my tests
--
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 #2709: [CALCITE-4996] in RelJson, add a new public toRex method which can ov…
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #2709:
URL: https://github.com/apache/calcite/pull/2709#discussion_r798930442
##########
File path: core/src/main/java/org/apache/calcite/rel/externalize/RelJson.java
##########
@@ -776,4 +905,22 @@ private void addRexFieldCollationList(
map.put("syntax", operator.getSyntax().toString());
return map;
}
+
+ /**
Review comment:
I wouldn't mention the 'functional interface'. Document its purpose, which is to translate.
Rename `apply` to something like `translate`. `apply` only made sense when this was a `BiFunction`.
In the method javadoc change `Lambda that defines how to transform` to `Transforms`. Less is more.
Rename `stringObjectMap` to `map`.
##########
File path: core/src/main/java/org/apache/calcite/rel/externalize/RelJson.java
##########
@@ -99,7 +102,143 @@
"org.apache.calcite.adapter.jdbc.JdbcRules$");
public RelJson(@Nullable JsonBuilder jsonBuilder) {
+ this(jsonBuilder, RelJson::inputTranslatorImpl);
+ }
+
+ private static RexNode inputTranslatorImpl(
+ Map<String, Object> stringObjectMap,
+ RexBuilder rexBuilder,
+ List<RelNode> relNodes) {
+ final Integer input = (Integer) stringObjectMap.get("input");
+ if (input != null) {
+ int i = input;
+ for (RelNode inputNode : relNodes) {
+ final RelDataType rowType = inputNode.getRowType();
+ if (i < rowType.getFieldCount()) {
+ final RelDataTypeField field = rowType.getFieldList().get(i);
+ return rexBuilder.makeInputRef(field.getType(), input);
+ }
+ i -= rowType.getFieldCount();
+ }
+ throw new RuntimeException("input field " + input + " is out of range");
+ } else {
+ throw new RuntimeException("input not defined");
+ }
+ }
+
+ public RelJson(@Nullable JsonBuilder jsonBuilder, InputTranslator inputTranslator) {
this.jsonBuilder = jsonBuilder;
+ this.inputTranslator = inputTranslator;
+ }
+
+ /**
+ * Transforms a RexNode tree defined in a map (from a JSON) into a RexNode,
+ * applying a special method to inputs instead of transforming them into inputRef.
+ * @param cluster The optimization environment
+ * @param apply is a InputTranslator lambda that transforms the map representing input
Review comment:
rename `apply` parameter to `translator`
##########
File path: core/src/main/java/org/apache/calcite/rel/externalize/RelJson.java
##########
@@ -99,7 +102,143 @@
"org.apache.calcite.adapter.jdbc.JdbcRules$");
public RelJson(@Nullable JsonBuilder jsonBuilder) {
+ this(jsonBuilder, RelJson::inputTranslatorImpl);
+ }
+
+ private static RexNode inputTranslatorImpl(
+ Map<String, Object> stringObjectMap,
+ RexBuilder rexBuilder,
+ List<RelNode> relNodes) {
+ final Integer input = (Integer) stringObjectMap.get("input");
+ if (input != null) {
+ int i = input;
+ for (RelNode inputNode : relNodes) {
+ final RelDataType rowType = inputNode.getRowType();
+ if (i < rowType.getFieldCount()) {
+ final RelDataTypeField field = rowType.getFieldList().get(i);
+ return rexBuilder.makeInputRef(field.getType(), input);
+ }
+ i -= rowType.getFieldCount();
+ }
+ throw new RuntimeException("input field " + input + " is out of range");
+ } else {
+ throw new RuntimeException("input not defined");
+ }
+ }
+
+ public RelJson(@Nullable JsonBuilder jsonBuilder, InputTranslator inputTranslator) {
this.jsonBuilder = jsonBuilder;
+ this.inputTranslator = inputTranslator;
+ }
+
+ /**
+ * Transforms a RexNode tree defined in a map (from a JSON) into a RexNode,
+ * applying a special method to inputs instead of transforming them into inputRef.
+ * @param cluster The optimization environment
+ * @param apply is a InputTranslator lambda that transforms the map representing input
+ * references into a RexNode
+ * @param o the map derived from a RexNode transformed into a JSON
+ * @return the transformed RexNode
+ */
+ public static RexNode readExpression(
Review comment:
Let's move this method a bit further down the file. It's not the most important method in this class.
The description 'Transforms a RexNode tree' is a bit misleading, given that the input is JSON not RexNode. Maybe something like 'Converts a JSON expression...'?
Convert the anonymous inner class that implements RelInput into a private static inner class. That gives us an opportunity to document it. Also it will make it easier to refactor in future.
##########
File path: core/src/test/java/org/apache/calcite/plan/RelWriterTest.java
##########
@@ -706,6 +717,51 @@
+ " LogicalTableScan(table=[[hr, emps]])\n"));
}
+ @Test void testJsonToRex() throws JsonProcessingException {
+ // Test simple literal without inputs
+ final String jsonString1 = "{\n"
+ + " \"literal\": 10,\n"
+ + " \"type\": {\n"
+ + " \"type\": \"INTEGER\",\n"
+ + " \"nullable\": false\n"
+ + " }\n"
+ + " }\n";
+
+ assertReadRex(jsonString1, "10");
+
+ // Test Binary with an input
+ final String jsonString2 = "{ \"op\": \n"
+ + " { \"name\": \"+\",\n"
+ + " \"kind\": \"PLUS\",\n"
+ + " \"syntax\": \"BINARY\"\n"
+ + " },\n"
+ + " \"operands\": [\n"
+ + " {\n"
+ + " \"input\": 1,\n"
+ + " \"sql\": \"column + 1\"\n"
+ + " },\n"
+ + " {\n"
+ + " \"literal\": 1,\n"
+ + " \"type\": { \"type\": \"INTEGER\", \"nullable\": false }\n"
+ + " }\n"
+ + " ]\n"
+ + " }";
+ assertReadRex(jsonString2, "+(1, 1)");
+ }
+
+ private void assertReadRex(String jsonString1, String expected)
Review comment:
better name would be `assertThatReadExpressionResult`
change parameter to from `String` to `Matcher<String>`. then there will be an `is` at the call site, and the code will read somewhat like English (per junit best practices, e.g. `assertThat(1 + 2, is(3));`
##########
File path: core/src/test/java/org/apache/calcite/plan/RelWriterTest.java
##########
@@ -429,6 +435,11 @@
return Stream.of(SqlExplainFormat.TEXT, SqlExplainFormat.DOT);
}
+ private static RexNode apply(Map<String, Object> map, RexBuilder rexBuilder, List<RelNode> inputs) {
Review comment:
need better name than `apply`. Also a bit of javadoc describing intend. In the javadoc I'd mention that it is intended to implement the translator interface.
--
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 #2709: [CALCITE-4996] in RelJson, add a new public toRex method which can ov…
Posted by GitBox <gi...@apache.org>.
julianhyde commented on pull request #2709:
URL: https://github.com/apache/calcite/pull/2709#issuecomment-1029278857
@asolimando, I see that @mkou added similar description in the Jira case https://issues.apache.org/jira/browse/CALCITE-4996. I think that the Jira case is the right place for discussion.
IMHO each Jira case is its own conversation thread. Cases appear on the dev list when they are logged. If you are interested in seeing how the discussion develops, press the 'watch' button.
--
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 closed pull request #2709: [CALCITE-4996] in RelJson, add a new public toRex method which can ov…
Posted by GitBox <gi...@apache.org>.
julianhyde closed pull request #2709:
URL: https://github.com/apache/calcite/pull/2709
--
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] asolimando commented on pull request #2709: [CALCITE-4996] in RelJson, add a new public toRex method which can ov…
Posted by GitBox <gi...@apache.org>.
asolimando commented on pull request #2709:
URL: https://github.com/apache/calcite/pull/2709#issuecomment-1027738943
@mkou for general discussions and tips I suggest to send an email to the ML, you have better chances to get a reply than someone reading the PR description
--
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