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 2020/08/14 09:47:27 UTC
[GitHub] [calcite] yanlin-Lynn opened a new pull request #2110: [CALCITE-4177] Throw exception when deserialize SqlOperator fails, do not return null
yanlin-Lynn opened a new pull request #2110:
URL: https://github.com/apache/calcite/pull/2110
See jira [CALCITE-4177](https://issues.apache.org/jira/browse/CALCITE-4177)
----------------------------------------------------------------
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] liyafan82 commented on a change in pull request #2110: [CALCITE-4177] Throw exception when deserialize SqlOperator fails, do not return null
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #2110:
URL: https://github.com/apache/calcite/pull/2110#discussion_r472054313
##########
File path: core/src/test/java/org/apache/calcite/plan/RelWriterTest.java
##########
@@ -1131,6 +1132,36 @@ private RelNode mockCountOver(String table,
assertThat(s, isLinux(expected));
}
+ @Test void testDeserializeInvalidOperatorName() {
+ final FrameworkConfig config = RelBuilderTest.config().build();
+ final RelBuilder builder = RelBuilder.create(config);
+ final RelNode rel = builder
+ .scan("EMP")
+ .project(
+ builder.field("JOB"),
+ builder.field("SAL"))
+ .aggregate(
+ builder.groupKey("JOB"),
+ builder.max("max_sal", builder.field("SAL")),
+ builder.min("min_sal", builder.field("SAL")))
+ .project(
+ builder.field("max_sal"),
+ builder.field("min_sal"))
+ .build();
+ final RelJsonWriter jsonWriter = new RelJsonWriter();
+ rel.explain(jsonWriter);
+ // mock a non exist SqlOperator
+ String relJson = jsonWriter.asString().replace("\"name\": \"MAX\"", "\"name\": \"MAXS\"");
+ try {
+ deserializeAndDumpToTextFormat(getSchema(rel), relJson);
+ fail();
+ } catch (Exception e) {
Review comment:
Maybe we should catch CalciteException here?
----------------------------------------------------------------
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] hsyuan commented on pull request #2110: [CALCITE-4177] Throw exception when deserialize SqlOperator fails, do not return null
Posted by GitBox <gi...@apache.org>.
hsyuan commented on pull request #2110:
URL: https://github.com/apache/calcite/pull/2110#issuecomment-922660075
Closed in https://github.com/apache/calcite/commit/ce86af83caae335d1b47def55354d42a517f45d0.
--
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] yanlin-Lynn commented on pull request #2110: [CALCITE-4177] Throw exception when deserialize SqlOperator fails, do not return null
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on pull request #2110:
URL: https://github.com/apache/calcite/pull/2110#issuecomment-674772265
> We better also add some tests there ~
Thanks for review. But I didn't get your point, there is a test case already.
----------------------------------------------------------------
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] yanlin-Lynn commented on a change in pull request #2110: [CALCITE-4177] Throw exception when deserialize SqlOperator fails, do not return null
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on a change in pull request #2110:
URL: https://github.com/apache/calcite/pull/2110#discussion_r473703746
##########
File path: core/src/test/java/org/apache/calcite/plan/RelWriterTest.java
##########
@@ -1131,6 +1132,36 @@ private RelNode mockCountOver(String table,
assertThat(s, isLinux(expected));
}
+ @Test void testDeserializeInvalidOperatorName() {
+ final FrameworkConfig config = RelBuilderTest.config().build();
+ final RelBuilder builder = RelBuilder.create(config);
+ final RelNode rel = builder
+ .scan("EMP")
+ .project(
+ builder.field("JOB"),
+ builder.field("SAL"))
+ .aggregate(
+ builder.groupKey("JOB"),
+ builder.max("max_sal", builder.field("SAL")),
+ builder.min("min_sal", builder.field("SAL")))
+ .project(
+ builder.field("max_sal"),
+ builder.field("min_sal"))
+ .build();
+ final RelJsonWriter jsonWriter = new RelJsonWriter();
+ rel.explain(jsonWriter);
+ // mock a non exist SqlOperator
+ String relJson = jsonWriter.asString().replace("\"name\": \"MAX\"", "\"name\": \"MAXS\"");
+ try {
+ deserializeAndDumpToTextFormat(getSchema(rel), relJson);
+ fail();
+ } catch (Exception e) {
+ assertThat(e.getMessage(),
+ isLinux("org.apache.calcite.runtime.CalciteException: "
+ + "No operator for 'MAXS' with kind: 'MAX', syntax: 'FUNCTION'"));
+ }
Review comment:
Sure
----------------------------------------------------------------
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] chunweilei commented on a change in pull request #2110: [CALCITE-4177] Throw exception when deserialize SqlOperator fails, do not return null
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #2110:
URL: https://github.com/apache/calcite/pull/2110#discussion_r477022639
##########
File path: core/src/main/java/org/apache/calcite/rel/externalize/RelJson.java
##########
@@ -678,7 +678,8 @@ SqlOperator toOp(Map<String, Object> map) {
if (class_ != null) {
return AvaticaUtils.instantiatePlugin(SqlOperator.class, class_);
}
- return null;
+ throw new RuntimeException("No operator for " + name + " with kind: "
+ + kind + ", and syntax: " + syntax);
Review comment:
Agree with @danny0405.
----------------------------------------------------------------
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 #2110: [CALCITE-4177] Throw exception when deserialize SqlOperator fails, do not return null
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2110:
URL: https://github.com/apache/calcite/pull/2110#discussion_r476072339
##########
File path: core/src/main/java/org/apache/calcite/rel/externalize/RelJson.java
##########
@@ -678,7 +678,8 @@ SqlOperator toOp(Map<String, Object> map) {
if (class_ != null) {
return AvaticaUtils.instantiatePlugin(SqlOperator.class, class_);
}
- return null;
+ throw new RuntimeException("No operator for " + name + " with kind: "
+ + kind + ", and syntax: " + syntax);
Review comment:
Is this a breaking change ? Do you know the background that it returns null before ?
----------------------------------------------------------------
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] liyafan82 commented on a change in pull request #2110: [CALCITE-4177] Throw exception when deserialize SqlOperator fails, do not return null
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #2110:
URL: https://github.com/apache/calcite/pull/2110#discussion_r472055176
##########
File path: core/src/test/java/org/apache/calcite/plan/RelWriterTest.java
##########
@@ -1131,6 +1132,36 @@ private RelNode mockCountOver(String table,
assertThat(s, isLinux(expected));
}
+ @Test void testDeserializeInvalidOperatorName() {
+ final FrameworkConfig config = RelBuilderTest.config().build();
+ final RelBuilder builder = RelBuilder.create(config);
+ final RelNode rel = builder
+ .scan("EMP")
+ .project(
+ builder.field("JOB"),
+ builder.field("SAL"))
+ .aggregate(
+ builder.groupKey("JOB"),
+ builder.max("max_sal", builder.field("SAL")),
+ builder.min("min_sal", builder.field("SAL")))
+ .project(
+ builder.field("max_sal"),
+ builder.field("min_sal"))
+ .build();
+ final RelJsonWriter jsonWriter = new RelJsonWriter();
+ rel.explain(jsonWriter);
+ // mock a non exist SqlOperator
+ String relJson = jsonWriter.asString().replace("\"name\": \"MAX\"", "\"name\": \"MAXS\"");
+ try {
+ deserializeAndDumpToTextFormat(getSchema(rel), relJson);
+ fail();
+ } catch (Exception e) {
+ assertThat(e.getMessage(),
+ isLinux("org.apache.calcite.runtime.CalciteException: "
+ + "No operator for 'MAXS' with kind: 'MAX', syntax: 'FUNCTION'"));
+ }
Review comment:
Maybe we can use JUnit's `assertThrows` utility here?
Some examples can be found in org.apache.calcite.util.mapping.MappingTest.
----------------------------------------------------------------
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] yanlin-Lynn commented on a change in pull request #2110: [CALCITE-4177] Throw exception when deserialize SqlOperator fails, do not return null
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on a change in pull request #2110:
URL: https://github.com/apache/calcite/pull/2110#discussion_r471349483
##########
File path: core/src/main/java/org/apache/calcite/rel/externalize/RelJson.java
##########
@@ -678,7 +678,8 @@ SqlOperator toOp(Map<String, Object> map) {
if (class_ != null) {
return AvaticaUtils.instantiatePlugin(SqlOperator.class, class_);
}
- return null;
+ throw new RuntimeException("No operator for " + name + " with kind: "
+ + kind + ", and syntax: " + syntax);
Review comment:
yes, I'll update
----------------------------------------------------------------
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] yanlin-Lynn commented on a change in pull request #2110: [CALCITE-4177] Throw exception when deserialize SqlOperator fails, do not return null
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on a change in pull request #2110:
URL: https://github.com/apache/calcite/pull/2110#discussion_r473703312
##########
File path: core/src/test/java/org/apache/calcite/plan/RelWriterTest.java
##########
@@ -1131,6 +1132,36 @@ private RelNode mockCountOver(String table,
assertThat(s, isLinux(expected));
}
+ @Test void testDeserializeInvalidOperatorName() {
+ final FrameworkConfig config = RelBuilderTest.config().build();
+ final RelBuilder builder = RelBuilder.create(config);
+ final RelNode rel = builder
+ .scan("EMP")
+ .project(
+ builder.field("JOB"),
+ builder.field("SAL"))
+ .aggregate(
+ builder.groupKey("JOB"),
+ builder.max("max_sal", builder.field("SAL")),
+ builder.min("min_sal", builder.field("SAL")))
+ .project(
+ builder.field("max_sal"),
+ builder.field("min_sal"))
+ .build();
+ final RelJsonWriter jsonWriter = new RelJsonWriter();
+ rel.explain(jsonWriter);
+ // mock a non exist SqlOperator
+ String relJson = jsonWriter.asString().replace("\"name\": \"MAX\"", "\"name\": \"MAXS\"");
+ try {
+ deserializeAndDumpToTextFormat(getSchema(rel), relJson);
+ fail();
+ } catch (Exception e) {
Review comment:
The stack trace for this case is like this, `CalciteException` wrapped with `RuntimeException`
```
java.lang.RuntimeException: org.apache.calcite.runtime.CalciteException: No operator for 'MAXS' with kind: 'MAX', syntax: 'FUNCTION'
at org.apache.calcite.tools.Frameworks.withPrepare(Frameworks.java:182)
at org.apache.calcite.tools.Frameworks.withPlanner(Frameworks.java:126)
at org.apache.calcite.tools.Frameworks.withPlanner(Frameworks.java:144)
at org.apache.calcite.plan.RelWriterTest.deserializeAndDumpToTextFormat(RelWriterTest.java:891)
at org.apache.calcite.plan.RelWriterTest.testDeserializeInvalidOperatorName(RelWriterTest.java:1157)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:675)
at org.junit.jupiter.engine.execution.MethodInvocation.proceed(MethodInvocation.java:60)
at org.junit.jupiter.engine.execution.InvocationInterceptorChain$ValidatingInvocation.proceed(InvocationInterceptorChain.java:125)
at org.junit.jupiter.engine.extension.TimeoutExtension.intercept(TimeoutExtension.java:139)
at org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestableMethod(TimeoutExtension.java:131)
at org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestMethod(TimeoutExtension.java:81)
at org.junit.jupiter.engine.execution.ExecutableInvoker$ReflectiveInterceptorCall.lambda$ofVoidMethod$0(ExecutableInvoker.java:115)
at org.junit.jupiter.engine.execution.ExecutableInvoker.lambda$invoke$0(ExecutableInvoker.java:105)
at org.junit.jupiter.engine.execution.InvocationInterceptorChain$InterceptedInvocation.proceed(InvocationInterceptorChain.java:104)
at org.junit.jupiter.engine.execution.InvocationInterceptorChain.proceed(InvocationInterceptorChain.java:62)
at org.junit.jupiter.engine.execution.InvocationInterceptorChain.chainAndInvoke(InvocationInterceptorChain.java:43)
at org.junit.jupiter.engine.execution.InvocationInterceptorChain.invoke(InvocationInterceptorChain.java:35)
at org.junit.jupiter.engine.execution.ExecutableInvoker.invoke(ExecutableInvoker.java:104)
at org.junit.jupiter.engine.execution.ExecutableInvoker.invoke(ExecutableInvoker.java:98)
at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.lambda$invokeTestMethod$6(TestMethodTestDescriptor.java:202)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.invokeTestMethod(TestMethodTestDescriptor.java:198)
at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:135)
at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:69)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$5(NodeTestTask.java:135)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$7(NodeTestTask.java:125)
at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:135)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:123)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:122)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:80)
at java.util.ArrayList.forEach(ArrayList.java:1257)
at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:38)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$5(NodeTestTask.java:139)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$7(NodeTestTask.java:125)
at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:135)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:123)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:122)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:80)
at java.util.ArrayList.forEach(ArrayList.java:1257)
at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:38)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$5(NodeTestTask.java:139)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$7(NodeTestTask.java:125)
at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:135)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:123)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:122)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:80)
at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.submit(SameThreadHierarchicalTestExecutorService.java:32)
at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.execute(HierarchicalTestExecutor.java:57)
at org.junit.platform.engine.support.hierarchical.HierarchicalTestEngine.execute(HierarchicalTestEngine.java:51)
at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:248)
at org.junit.platform.launcher.core.DefaultLauncher.lambda$execute$5(DefaultLauncher.java:211)
at org.junit.platform.launcher.core.DefaultLauncher.withInterceptedStreams(DefaultLauncher.java:226)
at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:199)
at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:132)
at com.intellij.junit5.JUnit5IdeaTestRunner.startRunnerWithArgs(JUnit5IdeaTestRunner.java:69)
at com.intellij.rt.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:33)
at com.intellij.rt.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:230)
at com.intellij.rt.junit.JUnitStarter.main(JUnitStarter.java:58)
Caused by: org.apache.calcite.runtime.CalciteException: No operator for 'MAXS' with kind: 'MAX', syntax: 'FUNCTION'
at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
at org.apache.calcite.runtime.Resources$ExInstWithCause.ex(Resources.java:467)
at org.apache.calcite.runtime.Resources$ExInst.ex(Resources.java:560)
at org.apache.calcite.rel.externalize.RelJson.toOp(RelJson.java:682)
at org.apache.calcite.rel.externalize.RelJson.toAggregation(RelJson.java:686)
at org.apache.calcite.rel.externalize.RelJsonReader.toAggCall(RelJsonReader.java:281)
at org.apache.calcite.rel.externalize.RelJsonReader.access$500(RelJsonReader.java:59)
at org.apache.calcite.rel.externalize.RelJsonReader$2.getAggregateCalls(RelJsonReader.java:172)
at org.apache.calcite.rel.core.Aggregate.<init>(Aggregate.java:220)
at org.apache.calcite.rel.logical.LogicalAggregate.<init>(LogicalAggregate.java:105)
at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
at org.apache.calcite.rel.externalize.RelJsonReader.readRel(RelJsonReader.java:264)
at org.apache.calcite.rel.externalize.RelJsonReader.readRels(RelJsonReader.java:91)
at org.apache.calcite.rel.externalize.RelJsonReader.read(RelJsonReader.java:85)
at org.apache.calcite.plan.RelWriterTest.lambda$deserializeAndDumpToTextFormat$6(RelWriterTest.java:896)
at org.apache.calcite.tools.Frameworks.lambda$withPlanner$0(Frameworks.java:131)
at org.apache.calcite.prepare.CalcitePrepareImpl.perform(CalcitePrepareImpl.java:914)
at org.apache.calcite.tools.Frameworks.withPrepare(Frameworks.java:180)
... 67 more
```
----------------------------------------------------------------
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 pull request #2110: [CALCITE-4177] Throw exception when deserialize SqlOperator fails, do not return null
Posted by GitBox <gi...@apache.org>.
danny0405 commented on pull request #2110:
URL: https://github.com/apache/calcite/pull/2110#issuecomment-675242513
> > We better also add some tests there ~
>
> Thanks for review. But I didn't get your point, there is a test case already.
Oops, i didn't saw 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] xy2953396112 commented on pull request #2110: [CALCITE-4177] Throw exception when deserialize SqlOperator fails, do not return null
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on pull request #2110:
URL: https://github.com/apache/calcite/pull/2110#issuecomment-686504234
LGTM~, Can you fix the CI?
----------------------------------------------------------------
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] hsyuan closed pull request #2110: [CALCITE-4177] Throw exception when deserialize SqlOperator fails, do not return null
Posted by GitBox <gi...@apache.org>.
hsyuan closed pull request #2110:
URL: https://github.com/apache/calcite/pull/2110
--
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] yanlin-Lynn commented on a change in pull request #2110: [CALCITE-4177] Throw exception when deserialize SqlOperator fails, do not return null
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on a change in pull request #2110:
URL: https://github.com/apache/calcite/pull/2110#discussion_r479777370
##########
File path: core/src/main/java/org/apache/calcite/rel/externalize/RelJson.java
##########
@@ -678,7 +678,8 @@ SqlOperator toOp(Map<String, Object> map) {
if (class_ != null) {
return AvaticaUtils.instantiatePlugin(SqlOperator.class, class_);
}
- return null;
+ throw new RuntimeException("No operator for " + name + " with kind: "
+ + kind + ", and syntax: " + syntax);
Review comment:
Yes, this breaks a little. I don't know the background why it returns null before.
Hopes someone can help to give the background, but I doubt there exists some special reason, because null will always cause NPE somewhere else.
----------------------------------------------------------------
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 pull request #2110: [CALCITE-4177] Throw exception when deserialize SqlOperator fails, do not return null
Posted by GitBox <gi...@apache.org>.
danny0405 commented on pull request #2110:
URL: https://github.com/apache/calcite/pull/2110#issuecomment-674618850
We better also add some tests there ~
----------------------------------------------------------------
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] amaliujia commented on a change in pull request #2110: [CALCITE-4177] Throw exception when deserialize SqlOperator fails, do not return null
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #2110:
URL: https://github.com/apache/calcite/pull/2110#discussion_r470818718
##########
File path: core/src/main/java/org/apache/calcite/rel/externalize/RelJson.java
##########
@@ -678,7 +678,8 @@ SqlOperator toOp(Map<String, Object> map) {
if (class_ != null) {
return AvaticaUtils.instantiatePlugin(SqlOperator.class, class_);
}
- return null;
+ throw new RuntimeException("No operator for " + name + " with kind: "
+ + kind + ", and syntax: " + syntax);
Review comment:
Will it better to move this exception in `CalciteResource.java`?
----------------------------------------------------------------
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] yanlin-Lynn commented on a change in pull request #2110: [CALCITE-4177] Throw exception when deserialize SqlOperator fails, do not return null
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on a change in pull request #2110:
URL: https://github.com/apache/calcite/pull/2110#discussion_r473703312
##########
File path: core/src/test/java/org/apache/calcite/plan/RelWriterTest.java
##########
@@ -1131,6 +1132,36 @@ private RelNode mockCountOver(String table,
assertThat(s, isLinux(expected));
}
+ @Test void testDeserializeInvalidOperatorName() {
+ final FrameworkConfig config = RelBuilderTest.config().build();
+ final RelBuilder builder = RelBuilder.create(config);
+ final RelNode rel = builder
+ .scan("EMP")
+ .project(
+ builder.field("JOB"),
+ builder.field("SAL"))
+ .aggregate(
+ builder.groupKey("JOB"),
+ builder.max("max_sal", builder.field("SAL")),
+ builder.min("min_sal", builder.field("SAL")))
+ .project(
+ builder.field("max_sal"),
+ builder.field("min_sal"))
+ .build();
+ final RelJsonWriter jsonWriter = new RelJsonWriter();
+ rel.explain(jsonWriter);
+ // mock a non exist SqlOperator
+ String relJson = jsonWriter.asString().replace("\"name\": \"MAX\"", "\"name\": \"MAXS\"");
+ try {
+ deserializeAndDumpToTextFormat(getSchema(rel), relJson);
+ fail();
+ } catch (Exception e) {
Review comment:
Sure, I'll update
----------------------------------------------------------------
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] yanlin-Lynn commented on a change in pull request #2110: [CALCITE-4177] Throw exception when deserialize SqlOperator fails, do not return null
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on a change in pull request #2110:
URL: https://github.com/apache/calcite/pull/2110#discussion_r473703746
##########
File path: core/src/test/java/org/apache/calcite/plan/RelWriterTest.java
##########
@@ -1131,6 +1132,36 @@ private RelNode mockCountOver(String table,
assertThat(s, isLinux(expected));
}
+ @Test void testDeserializeInvalidOperatorName() {
+ final FrameworkConfig config = RelBuilderTest.config().build();
+ final RelBuilder builder = RelBuilder.create(config);
+ final RelNode rel = builder
+ .scan("EMP")
+ .project(
+ builder.field("JOB"),
+ builder.field("SAL"))
+ .aggregate(
+ builder.groupKey("JOB"),
+ builder.max("max_sal", builder.field("SAL")),
+ builder.min("min_sal", builder.field("SAL")))
+ .project(
+ builder.field("max_sal"),
+ builder.field("min_sal"))
+ .build();
+ final RelJsonWriter jsonWriter = new RelJsonWriter();
+ rel.explain(jsonWriter);
+ // mock a non exist SqlOperator
+ String relJson = jsonWriter.asString().replace("\"name\": \"MAX\"", "\"name\": \"MAXS\"");
+ try {
+ deserializeAndDumpToTextFormat(getSchema(rel), relJson);
+ fail();
+ } catch (Exception e) {
+ assertThat(e.getMessage(),
+ isLinux("org.apache.calcite.runtime.CalciteException: "
+ + "No operator for 'MAXS' with kind: 'MAX', syntax: 'FUNCTION'"));
+ }
Review comment:
Sure, Thanks for advice.
----------------------------------------------------------------
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