accumulo-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] ctubbsii commented on issue #901: Fixes #888 - AccumuloInputFormat.fetchColumns should accept empty col…
Date Tue, 15 Jan 2019 23:29:52 GMT
ctubbsii commented on issue #901: Fixes #888 - AccumuloInputFormat.fetchColumns should accept
empty col…
URL: https://github.com/apache/accumulo/pull/901#issuecomment-454591914
 
 
   Hi @jzgithub1 ; Upon reviewing this issue, I'm not sure this (#888) should be "fixed" at
all. The point of the issue was to address the "fluent" use case, to make it easier to do
things like:
   
   ```java
     builder.fetchColumns(getUserInput()).store();
     // INSTEAD OF:
     // if ((input = getUserInput()) != null) {
     //   builder.fetchColumns(input).store();
     // } else {
     //   builder.store();
     // }
   }
   
   private Collection<String> getUserInput() {
     // some method that can return an empty collection
   }
   ```
   
   However, in doing so, it also permits this very very confusing behavior:
   
   ```java
     builder.fetchColumns(Collection.emptySet()).store();
   ```
   
   where you are *literally* telling the API to fetch **NO** columns, and it is doing the
exact opposite from what you said. This isn't good semantics.
   
   I thought of two other suggestions for how this could be changed to support the fluent
case and have clear semantics:
   
   ```java
     builder.fetchColumnsOrAll(getUserInput()).store(); // OR
     builder.fetchColumns(Optional.of(getUserInput())).store();
   ```
   
   In these, it's a lot more clear that there's some "special behavior" in the case of `null`
or `Optional.empty()`, instead of literally doing the opposite of what the user asked the
API to do.
   
   However, it's not clear that either of these two suggestions are better than the `IllegalArgumentException`
that was there originally. Perhaps somebody else has a good suggestion for clear API semantics
*AND* that supports the fluent use case. If not, while I appreciate that you spent time on
it, I'm inclined to think that it's no longer a good idea.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


With regards,
Apache Git Services

Mime
View raw message