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 Apr 13, 2023
1 parent 60e3091 commit 416bade
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 14 deletions.
28 changes: 22 additions & 6 deletions lib/miq_expression.rb
Original file line number Diff line number Diff line change
Expand Up @@ -379,11 +379,11 @@ def operator_hash(operator, children, unary: false)
# sql1, ruby1 | sql1 | ruby1 |
# ruby1, ruby2 | | ruby1, ruby1 |
#
def prune_exp_children(children, mode)
def prune_exp_children(children, mode, swap:)
seen = MODE_NONE
filtered_children = []
children.each do |child|
child_exp, child_seen = prune_exp(child, mode)
child_exp, child_seen = prune_exp(child, mode, :swap => swap)
seen |= child_seen
filtered_children << child_exp if child_exp
end
Expand Down Expand Up @@ -423,19 +423,35 @@ def prune_exp_children(children, mode)
# The OR case uses all nodes that match the input mode with one exception:
# mixed mode expressions are completely applied in ruby to keep the same logical result.
#
def prune_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 follow the AND logic if we are not swapping (and vice versa)
#
def prune_exp(exp, mode, swap: false)
operator = exp.keys.first
down_operator = operator.downcase
case down_operator
when "and", "or"
children, seen = prune_exp_children(exp[operator], mode)
if down_operator == "and" || seen != MODE_BOTH
children, seen = prune_exp_children(exp[operator], mode, :swap => swap)
if (down_operator == "and") != swap || seen != MODE_BOTH
[operator_hash(operator, children), seen]
else
[mode == MODE_RUBY ? exp : nil, seen]
end
when "not", "!"
children, seen = prune_exp(exp[operator], mode)
children, seen = prune_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_pruned_exp("NOT" => {"OR" => [sql_field, ruby_field.clone]})).to be_nil
# TODO: eq("NOT" => sql_field)
expect(sql_pruned_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_pruned_exp("NOT" => {"AND" => [sql_field, ruby_field.clone]})).to eq("NOT" => sql_field)
# TODO: be_nil
expect(sql_pruned_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_pruned_exp("NOT" => {"OR" => [sql_field, ruby_field.clone]})).to eq("NOT" => {"OR" => [sql_field, ruby_field.clone]})
# TODO: eq("NOT" => ruby_field)
expect(ruby_pruned_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_pruned_exp("NOT" => {"AND" => [sql_field, ruby_field.clone]})).to eq("NOT" => ruby_field)
# TODO: eq("NOT" => {"AND" => [sql_field, ruby_field]})
expect(ruby_pruned_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 416bade

Please sign in to comment.