[core] Introduce PredicateUtils to refactor Predicate handle #4469
Conversation
| Preconditions.checkState( | ||
| predicate instanceof CompoundPredicate, | ||
| "PredicateUtils##handleCompoundPredicate should handle with a CompoundPredicate."); | ||
|
|
There was a problem hiding this comment.
PredicateUtils.traverseCompoundPredicate only supports processing Predicates of CompoundPredicate type
| leafName, | ||
| (Predicate p) -> { | ||
| handleLeafPredicate(p, leafName); | ||
| }, |
There was a problem hiding this comment.
Can handleLeafPredicate method also be extracted to the util?
There was a problem hiding this comment.
I had considered about it，this method common logic is less，extract may cost more codes，so keep as current maybe better. Thanks~ @LinMingQiang
|
+1 |
| snapshotIds.add((Long) ((LeafPredicate) p).literals().get(0)); | ||
| } | ||
| }, | ||
| (Predicate p) -> { |
There was a problem hiding this comment.
(Predicate p) -> snapshotIds = null
| String leafName, | ||
| Consumer<Predicate> matchConsumer, | ||
| Consumer<Predicate> unMatchConsumer) { | ||
| Preconditions.checkState( |
There was a problem hiding this comment.
May be use Preconditions.checkArgument instead better?
| schemaIds.add((Long) ((LeafPredicate) p).literals().get(0)); | ||
| } | ||
| }, | ||
| (Predicate p) -> { |
There was a problem hiding this comment.
(Predicate p) -> schemaIds = null
| PredicateUtils.traverseCompoundPredicate( | ||
| predicate, | ||
| leafName, | ||
| (Predicate p) -> { |
There was a problem hiding this comment.
May replaced with expression lambda better?
(Predicate p) -> handleLeafPredicate(p, leafName)
| } | ||
| }, | ||
| (Predicate p) -> { | ||
| schemaIds = null; |
There was a problem hiding this comment.
Why not schemaIds.clear()?
| } | ||
| }, | ||
| (Predicate p) -> { | ||
| snapshotIds = null; |
There was a problem hiding this comment.
Why not snapshotIds.clear()?
There was a problem hiding this comment.
had just set to null which more efficient.
| } | ||
| }, | ||
| (Predicate p) -> { | ||
| predicateMap = null; |
There was a problem hiding this comment.
Why not predicateMap.clear();
There was a problem hiding this comment.
had just set to null which more efficient.
xuzifu666
changed the title
xuzifu666
changed the title
|
@wwj6591812 had addressed, Thanks for review. |
| import java.util.function.Consumer; | ||
|
|
||
| /** A utils to handle {@link Predicate}. */ | ||
| public class PredicateUtils { |
| * @param matchConsumer leafName matched handle | ||
| * @param unMatchConsumer leafName unmatched handle | ||
| */ | ||
| public static void traverseCompoundPredicate( |
| String leafName, | ||
| Consumer<Predicate> matchConsumer, | ||
| Consumer<Predicate> unMatchConsumer) { | ||
| Preconditions.checkArgument( |
There was a problem hiding this comment.
if (predicate is in predicate): just get elements from it.
| * @param matchConsumer leafName matched handle | ||
| * @param unMatchConsumer leafName unmatched handle | ||
| */ | ||
| public static void traverseCompoundPredicate( |
There was a problem hiding this comment.
Just return Optional<List<Object>>, elements of in.
There was a problem hiding this comment.
Get it, would modify as suggestion，Thanks @JingsongLi
There was a problem hiding this comment.
I'm traveling outside, would modify it in these two days.
There was a problem hiding this comment.
move the pr to a new pr #4486 close it first.
Purpose
Linked issue: close #xxx
Tests
API and Format
Documentation