[GitHub] [spark] cloud-fan edited a comment on issue #24910: [SPARK-28108][SQL] Simplify OrcFilters

[GitBox <gi...@apache.org>]

cloud-fan edited a comment on issue #24910: [SPARK-28108][SQL] Simplify OrcFilters
URL: https://github.com/apache/spark/pull/24910#issuecomment-503830569
 
 
   > separate the logic of building a convertible filter tree and the actual SearchArgument builder, since the two procedures are different and their return types are different... The code is more readable.
   
   I don't quite agree with this. The benefit of putting them together is: they share a similar procedure and it's easy to maintain and read if these 2 are combined. For example, when we want to support a new leaf predicate, we will always support it in both "building a convertible filter tree" and the "actual SearchArgument builder".
   
   One idea: we can separate the `And`/`Or`/`Not` part, and keep the leaf predicate part combined.
   ```
   def buildLeafSearchArguments... = {
     case EqualTo(...) => Some(...)
     ...
     case _ => None
   }
   
   def trimUnconvertibleFilters... = {
     def helper()... = {
       case And...
       case Or...
       ...
       case other => if (buildLeafSearchArguments(other).isDefined) Some(other) else None
     }
     ...
   }
   
   def buildSearchArgument... = {
     case And...
     ...
     case other => buildLeafSearchArguments(other)
   }
   ```
   
   Then we can still remove `ActionType` stuff. 

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org