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/05 11:06:10 UTC
[GitHub] [calcite] rubenada opened a new pull request #2095: [CALCITE-4156] ReflectiveRelMetadataProvider constructor should throw an exception (instead of assertion) when called with an empty map
rubenada opened a new pull request #2095:
URL: https://github.com/apache/calcite/pull/2095
Jira ticket: [CALCITE-4156](https://issues.apache.org/jira/browse/CALCITE-4156)
ReflectiveRelMetadataProvider's constructor verifies that it is not created with an empty map, using an assertion. However, this is not the most reliable way of verifying this situation, since assertions can be deactivated. In such scenario, we could silently end up having an invalid ReflectiveRelMetadataProvider, with no actual methods attached.
Also, since the map is private and has no getter, there is no way for a caller module to verify this situation on its side.
For this reason, it is proposed a minor change: replace the assertion with an IllegalArgumentException, which will work in 100% of the cases and will always prevent constructing an invalid ReflectiveRelMetadataProvider.
----------------------------------------------------------------
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] rubenada commented on a change in pull request #2095: [CALCITE-4156] ReflectiveRelMetadataProvider constructor should throw an exception (instead of assertion) when called with an empty map
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2095:
URL: https://github.com/apache/calcite/pull/2095#discussion_r466679069
##########
File path: core/src/main/java/org/apache/calcite/rel/metadata/ReflectiveRelMetadataProvider.java
##########
@@ -78,7 +78,9 @@ protected ReflectiveRelMetadataProvider(
ConcurrentMap<Class<RelNode>, UnboundMetadata> map,
Class<? extends Metadata> metadataClass0,
Multimap<Method, MetadataHandler> handlerMap) {
- assert !map.isEmpty() : "are your methods named wrong?";
+ if (map.isEmpty()) {
+ throw new IllegalArgumentException("ReflectiveRelMetadataProvider methods map is empty");
Review comment:
Thanks for the feedback @amaliujia , you are right, I am considering open a follow-up PR to change it into `Preconditions.checkArgument`
----------------------------------------------------------------
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 #2095: [CALCITE-4156] ReflectiveRelMetadataProvider constructor should throw an exception (instead of assertion) when called with an empty map
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2095:
URL: https://github.com/apache/calcite/pull/2095#discussion_r466171330
##########
File path: core/src/main/java/org/apache/calcite/rel/metadata/ReflectiveRelMetadataProvider.java
##########
@@ -78,7 +78,9 @@ protected ReflectiveRelMetadataProvider(
ConcurrentMap<Class<RelNode>, UnboundMetadata> map,
Class<? extends Metadata> metadataClass0,
Multimap<Method, MetadataHandler> handlerMap) {
- assert !map.isEmpty() : "are your methods named wrong?";
+ if (map.isEmpty()) {
+ throw new IllegalArgumentException("ReflectiveRelMetadataProvider methods map is empty");
+ }
Review comment:
There are many codes in Calcite use assert, should we also eliminate them ?
----------------------------------------------------------------
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] rubenada merged pull request #2095: [CALCITE-4156] ReflectiveRelMetadataProvider constructor should throw an exception (instead of assertion) when called with an empty map
Posted by GitBox <gi...@apache.org>.
rubenada merged pull request #2095:
URL: https://github.com/apache/calcite/pull/2095
----------------------------------------------------------------
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] rubenada commented on a change in pull request #2095: [CALCITE-4156] ReflectiveRelMetadataProvider constructor should throw an exception (instead of assertion) when called with an empty map
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2095:
URL: https://github.com/apache/calcite/pull/2095#discussion_r466191307
##########
File path: core/src/main/java/org/apache/calcite/rel/metadata/ReflectiveRelMetadataProvider.java
##########
@@ -78,7 +78,9 @@ protected ReflectiveRelMetadataProvider(
ConcurrentMap<Class<RelNode>, UnboundMetadata> map,
Class<? extends Metadata> metadataClass0,
Multimap<Method, MetadataHandler> handlerMap) {
- assert !map.isEmpty() : "are your methods named wrong?";
+ if (map.isEmpty()) {
+ throw new IllegalArgumentException("ReflectiveRelMetadataProvider methods map is empty");
+ }
Review comment:
No, I think generally assertions are a valid mechanism, but in this particular case in my opinion an exception is safer, for the reasons I mention in the Jira ticket: if assertions are deactivated we will not get later a runtime exception in this scenario, we will just end up silently constructing an invalid, empty ReflectiveRelMetadataProvider; and there is no mechanism in the caller module to verify this situation on its side. Hence this specific change to avoid this situation in 100% of the cases.
----------------------------------------------------------------
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] rubenada commented on a change in pull request #2095: [CALCITE-4156] ReflectiveRelMetadataProvider constructor should throw an exception (instead of assertion) when called with an empty map
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2095:
URL: https://github.com/apache/calcite/pull/2095#discussion_r466881560
##########
File path: core/src/main/java/org/apache/calcite/rel/metadata/ReflectiveRelMetadataProvider.java
##########
@@ -78,7 +78,9 @@ protected ReflectiveRelMetadataProvider(
ConcurrentMap<Class<RelNode>, UnboundMetadata> map,
Class<? extends Metadata> metadataClass0,
Multimap<Method, MetadataHandler> handlerMap) {
- assert !map.isEmpty() : "are your methods named wrong?";
+ if (map.isEmpty()) {
+ throw new IllegalArgumentException("ReflectiveRelMetadataProvider methods map is empty");
Review comment:
@amaliujia please check https://github.com/apache/calcite/pull/2100
----------------------------------------------------------------
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 #2095: [CALCITE-4156] ReflectiveRelMetadataProvider constructor should throw an exception (instead of assertion) when called with an empty map
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2095:
URL: https://github.com/apache/calcite/pull/2095#discussion_r466268939
##########
File path: core/src/main/java/org/apache/calcite/rel/metadata/ReflectiveRelMetadataProvider.java
##########
@@ -78,7 +78,9 @@ protected ReflectiveRelMetadataProvider(
ConcurrentMap<Class<RelNode>, UnboundMetadata> map,
Class<? extends Metadata> metadataClass0,
Multimap<Method, MetadataHandler> handlerMap) {
- assert !map.isEmpty() : "are your methods named wrong?";
+ if (map.isEmpty()) {
+ throw new IllegalArgumentException("ReflectiveRelMetadataProvider methods map is empty");
+ }
Review comment:
`No, I think generally assertions are a valid mechanism` I'm not convinced by this, we may need to reorganize our assertion usage. Can you log an issue 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 #2095: [CALCITE-4156] ReflectiveRelMetadataProvider constructor should throw an exception (instead of assertion) when called with an empty map
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #2095:
URL: https://github.com/apache/calcite/pull/2095#discussion_r466578677
##########
File path: core/src/main/java/org/apache/calcite/rel/metadata/ReflectiveRelMetadataProvider.java
##########
@@ -78,7 +78,9 @@ protected ReflectiveRelMetadataProvider(
ConcurrentMap<Class<RelNode>, UnboundMetadata> map,
Class<? extends Metadata> metadataClass0,
Multimap<Method, MetadataHandler> handlerMap) {
- assert !map.isEmpty() : "are your methods named wrong?";
+ if (map.isEmpty()) {
+ throw new IllegalArgumentException("ReflectiveRelMetadataProvider methods map is empty");
Review comment:
Sorry I missed your PR.
Why not use `CheckArgument` in Guava?
----------------------------------------------------------------
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] rubenada commented on a change in pull request #2095: [CALCITE-4156] ReflectiveRelMetadataProvider constructor should throw an exception (instead of assertion) when called with an empty map
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2095:
URL: https://github.com/apache/calcite/pull/2095#discussion_r466293874
##########
File path: core/src/main/java/org/apache/calcite/rel/metadata/ReflectiveRelMetadataProvider.java
##########
@@ -78,7 +78,9 @@ protected ReflectiveRelMetadataProvider(
ConcurrentMap<Class<RelNode>, UnboundMetadata> map,
Class<? extends Metadata> metadataClass0,
Multimap<Method, MetadataHandler> handlerMap) {
- assert !map.isEmpty() : "are your methods named wrong?";
+ if (map.isEmpty()) {
+ throw new IllegalArgumentException("ReflectiveRelMetadataProvider methods map is empty");
+ }
Review comment:
Ok, @danny0405 . I will merge the current PR and open a separate ticket to discuss / review the general usage of assertions in the code
----------------------------------------------------------------
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