Skip to content

Conversation

@lewismc
Copy link
Member

@lewismc lewismc commented Jan 7, 2016

This pull request implements the FilterList#filter function to provide us with both Operator.MUST_PASS_ALL (AND) and Operator.MUST_PASS_ONE (OR) query functionality.
It also introduces a new test class TestFilterList to test basic FilterList functionality as well as both Operator.MUST_PASS_ALL (AND) and Operator.MUST_PASS_ONE (OR) query functionality.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good, just being annoying here, but would it look more readable if we have a switch statement instead? We could use an Enum with values to do the trick ;)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it would, I'll update the PR thanks Renaldo.

On Monday, January 11, 2016, Renato Marroquin notifications@github.com
wrote:

In gora-core/src/main/java/org/apache/gora/filter/FilterList.java
#48 (comment):

  •  for (Filter<K, T> filter: filters) {
    
  •    if (!filter.filter(key, persistent)) {
    
  •      return !filtered;
    
  •    }
    
  •  }
    
  •  //AND
    
  • } else if (operator.equals(Operator.MUST_PASS_ALL)) {
  •  for (Filter<K, T> filter: filters) {
    
  •    if (filter.filter(key, persistent)) {
    
  •      return !filtered;
    
  •    }
    
  •  }
    
  • } else {
  •  throw new IllegalStateException(operator + " not yet implemented!");
    
  • }
  • return filtered;
    }

I think this looks good, just being annoying here, but would it look more
readable if we have a switch statement instead?


Reply to this email directly or view it on GitHub
https://github.com/apache/gora/pull/48/files#r49316977.

Lewis

@renato2099
Copy link
Contributor

but overall LGTM
maybe adding a new JIRA for testing the filter operation over gora-core implementation? And thanks for doing this Lewis! great work!

@lewismc
Copy link
Member Author

lewismc commented Jan 11, 2016

@renato2099 OK I updated the PR with the switch block which looks much better. This is also updated and merged with master.

@renato2099
Copy link
Contributor

awesome @lewismc ! thanks!

@asfgit asfgit merged commit 3326f58 into apache:master Jan 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants