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/11/06 01:51:19 UTC
[GitHub] [calcite-avatica] sunjincheng121 opened a new pull request #161: [CALCITE-4877] Make the exception information more explicit for instantiate plugin.
sunjincheng121 opened a new pull request #161:
URL: https://github.com/apache/calcite-avatica/pull/161
I want make the exception information more explicit for instantiate plugin in this PR. such as when we want REATE SCHEMA or CREATE TABLE supported. and we do not add the class to the class path we got the exception as follows:
`Error: Error while executing SQL "CREATE TABLE t (i INTEGER, j VARCHAR(10))": Property 'org.apache.calcite.sql.parser.ddl.SqlDdlParserImpl#FACTORY' not valid for plugin type org.apache.calcite.sql.parser.SqlParserImplFactory (state=,code=0)`
would be great if we can make the error message more explicit , such as:
`"Property 'org.apache.calcite.sql.parser.ddl.SqlDdlParserImpl#FACTORY' not valid as 'org.apache.calcite.sql.parser.ddl.SqlDdlParserImpl' not found in the classpath." `
--
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-avatica] vlsi merged pull request #161: [CALCITE-4877] Make the exception information more explicit for instantiate plugin.
Posted by GitBox <gi...@apache.org>.
vlsi merged pull request #161:
URL: https://github.com/apache/calcite-avatica/pull/161
--
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-avatica] vlsi commented on pull request #161: [CALCITE-4877] Make the exception information more explicit for instantiate plugin.
Posted by GitBox <gi...@apache.org>.
vlsi commented on pull request #161:
URL: https://github.com/apache/calcite-avatica/pull/161#issuecomment-962487531
Thanks. I think the changes are good, however would you please adjust test data to fix test failures?
```
FAILURE 1.1sec, org.apache.calcite.test.ServerQuidemTest > test(String)[5], [5] sql/schema.iq
org.opentest4j.AssertionFailedError: Files differ: /home/runner/work/calcite-avatica/calcite/server/build/resources/test/sql/surefire/sql/schema.iq /home/runner/work/calcite-avatica/calcite/server/build/resources/test/sql/schema.iq
69c69,229
< Property 'com.example.BadSchemaFactory' not valid for plugin type org.apache.calcite.schema.SchemaFactory
---
> java.sql.SQLException: Error while executing SQL "create foreign schema fs library 'com.example.BadSchemaFactory'": Property 'com.example.BadSchemaFactory' not valid as 'com.example.BadSchemaFactory' not found in the classpath
```
--
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-avatica] sunjincheng121 commented on pull request #161: [CALCITE-4877] Make the exception information more explicit for instantiate plugin.
Posted by GitBox <gi...@apache.org>.
sunjincheng121 commented on pull request #161:
URL: https://github.com/apache/calcite-avatica/pull/161#issuecomment-962527969
> Thanks. I think the changes are good, however would you please adjust test data to fix test failures?
```
FAILURE 1.1sec, org.apache.calcite.test.ServerQuidemTest > test(String)[5], [5] sql/schema.iq
org.opentest4j.AssertionFailedError: Files differ: /home/runner/work/calcite-avatica/calcite/server/build/resources/test/sql/surefire/sql/schema.iq /home/runner/work/calcite-avatica/calcite/server/build/resources/test/sql/schema.iq
69c69,229
< Property 'com.example.BadSchemaFactory' not valid for plugin type org.apache.calcite.schema.SchemaFactory
---
> java.sql.SQLException: Error while executing SQL "create foreign schema fs library 'com.example.BadSchemaFactory'": Property 'com.example.BadSchemaFactory' not valid as 'com.example.BadSchemaFactory' not found in the classpath
```
Thanks for your feedback @vlsi
I think the test failure is caused by the interdependence between the calcite project and the current calcite-avatica project. After the the PR （ https://github.com/apache/calcite/pull/2600 ) merged, the error will be solved automatically? If there is any thing with my understanding, please correct 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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite-avatica] sunjincheng121 edited a comment on pull request #161: [CALCITE-4877] Make the exception information more explicit for instantiate plugin.
Posted by GitBox <gi...@apache.org>.
sunjincheng121 edited a comment on pull request #161:
URL: https://github.com/apache/calcite-avatica/pull/161#issuecomment-962527969
> Thanks. I think the changes are good, however would you please adjust test data to fix test failures?
```
FAILURE 1.1sec, org.apache.calcite.test.ServerQuidemTest > test(String)[5], [5] sql/schema.iq
org.opentest4j.AssertionFailedError: Files differ: /home/runner/work/calcite-avatica/calcite/server/build/resources/test/sql/surefire/sql/schema.iq /home/runner/work/calcite-avatica/calcite/server/build/resources/test/sql/schema.iq
69c69,229
< Property 'com.example.BadSchemaFactory' not valid for plugin type org.apache.calcite.schema.SchemaFactory
---
> java.sql.SQLException: Error while executing SQL "create foreign schema fs library 'com.example.BadSchemaFactory'": Property 'com.example.BadSchemaFactory' not valid as 'com.example.BadSchemaFactory' not found in the classpath
```
Thanks for your feedback @vlsi
The test failure is caused by the follows two files:
- [org.apache.calcite.test.ServerQuidemTest](https://github.com/apache/calcite/blob/master/server/src/test/java/org/apache/calcite/test/ServerQuidemTest.java)
- [resources/test/sql/schema.iq](https://github.com/apache/calcite/blob/master/server/src/test/resources/sql/schema.iq)
the location of those two file are in the calcite project, i.e. the root cause of the test failure is due to interdependence between the calcite project and the current calcite-avatica project. After the the PR （ https://github.com/apache/calcite/pull/2600 ) merged, the error will be solved automatically. What do you think?
If there is any thing with my understanding, please correct me. Thank you @vlsi
--
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-avatica] sunjincheng121 commented on a change in pull request #161: [CALCITE-4877] Make the exception information more explicit for instantiate plugin.
Posted by GitBox <gi...@apache.org>.
sunjincheng121 commented on a change in pull request #161:
URL: https://github.com/apache/calcite-avatica/pull/161#discussion_r744184028
##########
File path: core/src/main/java/org/apache/calcite/avatica/AvaticaUtils.java
##########
@@ -235,10 +232,29 @@ public static Class box(Class clazz) {
} catch (NoSuchFieldException e) {
// ignore
}
+ assert pluginClass.isAssignableFrom(clazz);
Review comment:
Thanks for sharing your thoughts on the use of assert @asolimando , 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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite-avatica] sunjincheng121 edited a comment on pull request #161: [CALCITE-4877] Make the exception information more explicit for instantiate plugin.
Posted by GitBox <gi...@apache.org>.
sunjincheng121 edited a comment on pull request #161:
URL: https://github.com/apache/calcite-avatica/pull/161#issuecomment-962378109
`Error: 1.3sec org.apache.calcite.test.ServerQuidemTest > test(String)[5], [5] sql/schema.iq` will be fixed here: https://github.com/apache/calcite/pull/2600
--
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-avatica] vlsi commented on a change in pull request #161: [CALCITE-4877] Make the exception information more explicit for instantiate plugin.
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #161:
URL: https://github.com/apache/calcite-avatica/pull/161#discussion_r744097540
##########
File path: core/src/main/java/org/apache/calcite/avatica/AvaticaUtils.java
##########
@@ -235,10 +232,29 @@ public static Class box(Class clazz) {
} catch (NoSuchFieldException e) {
// ignore
}
+ assert pluginClass.isAssignableFrom(clazz);
Review comment:
Assertions are not enabled by default. Would you please replace the assertion with a regular if check?
--
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-avatica] sunjincheng121 commented on pull request #161: [CALCITE-4877] Make the exception information more explicit for instantiate plugin.
Posted by GitBox <gi...@apache.org>.
sunjincheng121 commented on pull request #161:
URL: https://github.com/apache/calcite-avatica/pull/161#issuecomment-962378109
`Error: 1.3sec org.apache.calcite.test.ServerQuidemTest > test(String)[5], [5] sql/schema.iq` will be fixed later.
--
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-avatica] asolimando commented on a change in pull request #161: [CALCITE-4877] Make the exception information more explicit for instantiate plugin.
Posted by GitBox <gi...@apache.org>.
asolimando commented on a change in pull request #161:
URL: https://github.com/apache/calcite-avatica/pull/161#discussion_r744143347
##########
File path: core/src/main/java/org/apache/calcite/avatica/AvaticaUtils.java
##########
@@ -235,10 +232,29 @@ public static Class box(Class clazz) {
} catch (NoSuchFieldException e) {
// ignore
}
+ assert pluginClass.isAssignableFrom(clazz);
Review comment:
My rule of thumb is to use assertions for situations that should never happen, it's more of a "documentation" for me where I state expectations that follow logically but are maybe not evident, all the other cases need checks+exception
--
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-avatica] sunjincheng121 commented on a change in pull request #161: [CALCITE-4877] Make the exception information more explicit for instantiate plugin.
Posted by GitBox <gi...@apache.org>.
sunjincheng121 commented on a change in pull request #161:
URL: https://github.com/apache/calcite-avatica/pull/161#discussion_r744184028
##########
File path: core/src/main/java/org/apache/calcite/avatica/AvaticaUtils.java
##########
@@ -235,10 +232,29 @@ public static Class box(Class clazz) {
} catch (NoSuchFieldException e) {
// ignore
}
+ assert pluginClass.isAssignableFrom(clazz);
Review comment:
I see. Thanks for sharing your thoughts on the use of assert @asolimando , 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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite-avatica] sunjincheng121 commented on pull request #161: [CALCITE-4877] Make the exception information more explicit for instantiate plugin.
Posted by GitBox <gi...@apache.org>.
sunjincheng121 commented on pull request #161:
URL: https://github.com/apache/calcite-avatica/pull/161#issuecomment-962421381
Thanks for you review @vlsi !
I have updated PR accordingly, I appreciate if you can have another look, Thank you!
Best，
Jincheng
--
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-avatica] sunjincheng121 commented on a change in pull request #161: [CALCITE-4877] Make the exception information more explicit for instantiate plugin.
Posted by GitBox <gi...@apache.org>.
sunjincheng121 commented on a change in pull request #161:
URL: https://github.com/apache/calcite-avatica/pull/161#discussion_r744099965
##########
File path: core/src/main/java/org/apache/calcite/avatica/AvaticaUtils.java
##########
@@ -235,10 +232,29 @@ public static Class box(Class clazz) {
} catch (NoSuchFieldException e) {
// ignore
}
+ assert pluginClass.isAssignableFrom(clazz);
Review comment:
+1 for your suggestion.
I do not want add assert here, just moved the position from up line to here . Agree with you, I do not like use assert to do the logical judgment. but I see that the project seems to favor to use assert. For example, in the following classes: `avaticaresultset`, `avaticasite`, `avaticaconnection`, `builtinconnectionproperty`, `connectionconfigimpl` etc.
Do you know any principles for using assert in this project? // I'm new to this project :)
--
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