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:
[email protected]
