[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

[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