Merge pull request #43 from wlmcewen/master

AC-1604 code review tweaks

lib/sparkql/evaluator.rb

@@ -1,10 +1,23 @@
 # Using an instance of ExpressionResolver to resolve the individual expressions,
 # this class will evaluate the rest of a parsed sparkql string to true or false.
-# Namely, this class will handle all the nesting, boolean algebra, and dropped 
+# Namely, this class will handle all the nesting, boolean algebra, and dropped
 # fields. Plus, it has some optimizations built in to skip the processing for
 # any expressions that don't contribute to the net result of the filter.
 class Sparkql::Evaluator
 
+  # The struct here mimics some of the parser information about an expression,
+  # but should not be confused for an expression. Nodes reduce the expressions
+  # to a result based on conjunction logic, and only one exists per block group.
+  Node = Struct.new(
+    :level,
+    :block_group,
+    :conjunction,
+    :conjunction_level,
+    :match,
+    :good_ors,
+    :expressions,
+    :unary)
+
   attr_reader :processed_count
 
   def initialize expression_resolver
@@ -13,15 +26,7 @@ def initialize expression_resolver
 
   def evaluate(expressions)
     @processed_count = 0
-    @index = {
-      level: 0,
-      block_group: 0,
-      conjunction: "And",
-      conjunction_level: 0,
-      match: true,
-      good_ors: false,
-      expressions: 0
-    }
+    @index = Node.new(0, 0, "And", 0, true, false, 0, nil)
     @groups = [@index]
     expressions.each do |expression|
       handle_group(expression)
@@ -66,8 +71,8 @@ def evaluate_node(node, result)
     end
     if node[:conjunction] == 'Not' &&
        (node[:conjunction_level] == node[:level] ||
-      node[:conjunction_level] == @index[:level])
-      @index[:match] = !result
+        node[:conjunction_level] == @index[:level])
+      @index[:match] = !result if @index[:match]
     elsif node[:conjunction] == 'And' || @index[:expressions] == 0
       @index[:match] = result if @index[:match]
     elsif node[:conjunction] == 'Or' && result
@@ -106,18 +111,12 @@ def skip?(expression)
   end
 
   def new_group(expression)
-    {
-      level: expression[:level],
-      block_group: expression[:block_group],
-      conjunction: expression[:conjunction],
-      conjunction_level: expression[:conjunction_level],
-      match: true,
-      good_ors: false,
-      expressions: 0
-    }
+    Node.new(expression[:level], expression[:block_group],
+      expression[:conjunction], expression[:conjunction_level],
+      true, false, 0, nil)
   end
 
-  # When the last expression was dropped, we need to repair the filter by 
+  # When the last expression was dropped, we need to repair the filter by
   # stealing the conjunction of that dropped field.
   def adjust_expression_for_dropped_field(expression)
     if @dropped_expression.nil?
@@ -129,8 +128,8 @@ def adjust_expression_for_dropped_field(expression)
     @dropped_expression = nil
   end
 
-  # This is similar to the cleanup step, but happens when we return from a 
-  # nesting level. Before we can proceed, we need wrap up the result of the 
+  # This is similar to the cleanup step, but happens when we return from a
+  # nesting level. Before we can proceed, we need wrap up the result of the
   # nested group.
   def smoosh_group(expression)
     until  @groups.last[:block_group] == expression[:block_group]

lib/sparkql/expression_resolver.rb

@@ -1,6 +1,12 @@
 # Base class for handling expression resolution
 class Sparkql::ExpressionResolver
 
+  # Accepted results from the resolve method:
+  # * true and false reflect the expression's boolean result (as all expressions
+  #   should).
+  # * :drop is a special symbol indicating that the expression should be omitted
+  #   from the filter. Special rules apply for a dropped expression, such as
+  #   keeping the conjunction of the dropped expression.
   VALID_RESULTS = [true, false, :drop]
 
   # Evaluate the result of this expression. Allows for any of the values in

test/unit/evaluator_test.rb

@@ -21,6 +21,46 @@ def test_conjunction
     assert !sample("Test Eq false Or Test Eq false")
   end
 
+  # One passing or expression in the set should always affirm a match this tests
+  # every permutation of one passing expression
+  def test_ors_stay_good
+    5.times do |i|
+      expressions = []
+      5.times do |j|
+        expressions << "Test Eq #{i == j}"
+      end
+      filter = expressions.join(" Or ")
+      assert sample(filter), "Filter: #{filter}"
+    end
+  end
+
+  # One failing AND expression in a set should always fail. Here we ensure every
+  # permutation of one failing
+  def test_ands_stay_bad
+    5.times do |i|
+      expressions = []
+      5.times do |j|
+        expressions << "Test Eq #{i != j}"
+      end
+      filter = expressions.join(" And ")
+      assert !sample(filter), "Filter: #{filter}"
+    end
+  end
+
+  # One failing Not expression in a set should always fail. Here we ensure every
+  # permutation of one failing
+  def test_nots_stay_bad
+    5.times do |i|
+      expressions = []
+      5.times do |j|
+        expressions << "Test Eq #{i == j}"
+      end
+      # Add the unary not to the front!
+      filter = "Not " + expressions.join(" Not ")
+      assert !sample(filter), "Filter: #{filter}"
+    end
+  end
+
   def test_dropped_field_handling
     assert sample("Test Eq 'Drop' And Test Eq true")
     assert !sample("Test Eq 'Drop' And Test Eq false")
@@ -57,6 +97,24 @@ def test_nots
     assert !sample("Test Eq true Not (Not Test Eq false)")
     assert sample("Not (Not Test Eq true)")
     assert sample("Not (Not(Not Test Eq true))")
+    assert !sample("Test Eq false And Test Eq true Not Test Eq false") 
+  end
+
+  def test_examples
+    # This one is based on a real life example that had problems.
+    #
+    # CurrentPrice Bt 130000.00,180000.00 And PropertySubType Eq 'Single Family Residence' And 
+    # SchoolDistrict Eq 'Byron Center','Grandville','Jenison' And MlsStatus Eq 'Active' And 
+    # BathsTotal Bt 1.50,9999.00 And BedsTotal Bt 3,99 And PropertyType Eq 'A' 
+    # Not "Garage"."Garage2" Eq 'No' And "Pool"."OutdoorAbove" Eq true 
+    # And "Pool"."OutdoorInground" Eq true Not "Substructure"."Michigan Basement" Eq true
+
+    assert !sample("Test Eq false And Test Eq true And " + 
+      "Test Eq false And Test Eq true And " + 
+      "Test Eq true And Test Eq true And Test Eq true " + 
+      "Not Test Eq false And Test Eq false " + 
+      "And Test Eq false Not Test Eq false"
+    )
   end
 
   def test_optimizations