Skip to content

Commit

Permalink
MiqExpression introduce demorgan's law
Browse files Browse the repository at this point in the history
Without this change, we were incorrectly filtering records
This mostly worked when the ruby expression handled all expressions
but it especially breaks down when the two are kept separate
  • Loading branch information
kbrock committed Feb 13, 2023
1 parent 6913607 commit 0b44ee2
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 20 deletions.
37 changes: 25 additions & 12 deletions lib/miq_expression.rb
Original file line number Diff line number Diff line change
Expand Up @@ -360,11 +360,11 @@ def operator_hash(operator, children, unary: false)
#
# in complicated cases, the child_exp can be different from the child nodes
#
def reduce_exp_children(children, mode)
def reduce_exp_children(children, mode, swap:)
seen = MODE_NONE
filtered_children = []
children.each do |child|
child_exp, child_seen = reduce_exp(child, mode)
child_exp, child_seen = reduce_exp(child, mode, :swap => swap)
seen |= child_seen
filtered_children << child_exp if child_exp
end
Expand Down Expand Up @@ -417,21 +417,34 @@ def reduce_exp_children(children, mode)
#
# In the AND case, the sql nodes line up with the filtered child nodes
#
def reduce_exp(exp, mode)
#
# exp |==>| exp (mode=sql) |and| exp (mode=ruby)
# -------------------|---|----------------|---|-----------------
# !(sql OR sql) |==>| !(sql OR sql) |AND|
# !(sql OR ruby) |==>| !(sql) |AND| !(ruby)
# !(ruby1 OR ruby2) |==>| |AND| !(ruby1 OR ruby2)
#
# exp |==>| exp (mode=sql) |and| exp (mode=ruby)
# -------------------|---|----------------|---|-----------------
# !(sql AND sql) |==>| !(sql AND sql) |AND|
# !(sql AND ruby) |==>| |AND| !(sql AND ruby)
# !(ruby1 AND ruby2) |==>| |AND| !(ruby1 AND ruby2)
#
# Inside a NOT, the OR acts like the AND, and the AND acts like the OR
# so swap is introduced to handle Demorgan's law
#
def reduce_exp(exp, mode, swap: false)
operator = exp.keys.first
case operator.downcase
when "and"
children, seen = reduce_exp_children(exp[operator], mode)
[operator_hash(operator, children), seen]
when "or"
children, seen = reduce_exp_children(exp[operator], mode)
if seen == MODE_BOTH
[mode == MODE_RUBY ? exp : nil, seen]
else
when "and", "or"
children, seen = reduce_exp_children(exp[operator], mode, :swap => swap)
if (operator.downcase == "and") != swap || seen != MODE_BOTH
[operator_hash(operator, children), seen]
else
[mode == MODE_RUBY ? exp : nil, seen]
end
when "not", "!"
children, seen = reduce_exp(exp[operator], mode)
children, seen = reduce_exp(exp[operator], mode, :swap => !swap)
[operator_hash(operator, children, :unary => true), seen]
else
if sql_supports_atom?(exp)
Expand Down
12 changes: 4 additions & 8 deletions spec/lib/miq_expression_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -267,13 +267,11 @@
end

it "!(sql OR ruby) => (!(sql) AND !(ruby)) => !(sql)" do
expect(sql_reduced_exp("NOT" => {"OR" => [sql_field, ruby_field.clone]})).to be_nil
# TODO: eq("NOT" => sql_field)
expect(sql_reduced_exp("NOT" => {"OR" => [sql_field, ruby_field.clone]})).to eq("NOT" => sql_field)
end

it "!(sql AND ruby) => (!(sql) OR !(ruby)) => nil" do
expect(sql_reduced_exp("NOT" => {"AND" => [sql_field, ruby_field.clone]})).to eq("NOT" => sql_field)
# TODO: be_nil
expect(sql_reduced_exp("NOT" => {"AND" => [sql_field, ruby_field.clone]})).to be_nil
end
end

Expand Down Expand Up @@ -331,13 +329,11 @@
end

it "!(sql OR ruby) => (!(sql) AND !(ruby)) => !(ruby)" do
expect(ruby_reduced_exp("NOT" => {"OR" => [sql_field, ruby_field.clone]})).to eq("NOT" => {"OR" => [sql_field, ruby_field.clone]})
# TODO: eq("NOT" => ruby_field)
expect(ruby_reduced_exp("NOT" => {"OR" => [sql_field, ruby_field.clone]})).to eq("NOT" => ruby_field)
end

it "!(sql AND ruby) => (!(sql) OR !(ruby)) => !(sql AND ruby)" do
expect(ruby_reduced_exp("NOT" => {"AND" => [sql_field, ruby_field.clone]})).to eq("NOT" => ruby_field)
# TODO: eq("NOT" => {"AND" => [sql_field, ruby_field]})
expect(ruby_reduced_exp("NOT" => {"AND" => [sql_field, ruby_field.clone]})).to eq("NOT" => {"AND" => [sql_field, ruby_field]})
end
end
end
Expand Down

0 comments on commit 0b44ee2

Please sign in to comment.