Miq expression removes sql only fields for filters #22347

kbrock · 2023-02-09T05:03:12Z

commits:

added some comments around MiqExpression.
added prune_exp to reduce the expression for a particular mode/language.
using prune_exp for sql (was preprocess_for_sql)
Rbac#filter now uses prune_exp to remove unnecessary sql from ruby.
fix bugs around Demorgan's law (i.e.: !(a || b) == !a && !b)

code	all(ms)	records(ms)	do_eval(ms)	subst(ms)
master	2289	815	144	1294
after	1225	769	37	285
delta	46%	n/a	74%	78%

Dates (via #22358) are on master and were a big win.
Since most of the fields were removed from the ruby expression processing, the whole processing time is approaching the record retrieval timing.

Wish I could have cached @ruby as something other than true, but since YAML.parse stores @ruby==nil as the uninitialized value, I need to store a value other than nil for a computed nil result.

NOTE: the expression was specifically picked because a lot of the fields were sql friendly. less sql friendly expressions will have less gain.

kbrock · 2023-02-13T17:48:22Z

update:

working on cops.
rearranged nodes to extract 2 PRs (so far)

kbrock · 2023-02-13T19:36:08Z

Update:

rebased to take in external Count and dependent PR
fixed whitespace issues and tweaked demorgan commit to remove duplicate code.

kbrock · 2023-02-13T19:53:47Z

update:

fixed refactor issue with renaming preprocess_exp!

kbrock · 2023-02-14T14:22:35Z

update:

added parameter to rbac to turn off prune_sql feature (mostly for performance testing)

kbrock · 2023-02-14T18:41:55Z

update:

Rbac#filtered takes a parameter to turn on/off the ruby filter

lib/miq_expression.rb

Fryguy · 2023-02-21T18:54:38Z

lib/miq_expression.rb

+  # 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)


Is reduce_exp modifying the original exp, or just returning a copy? If the former, can we name it reduce_exp!

original expression is not changed, but it does point to some of the entries from it

Sorry i'm late to this, but seems like we're dividing/filtering based on the mode (ruby/sql). reduce makes me think of the ruby enumerable method and that we're just simplifying. Partitioning by mode is what I think this is doing after reviewing the code. It's too bad partition is also an enumerable method. I guess divide is the best I can offer.

I will rename to prune_exp. We are pruning out the nodes that are no relevant

Fryguy

Naming-wise, seen and swap I find confusing, but I understand why you chose the names. I just know future-me won't understand what those mean. That being said, I can't figure out a better name.

Otherwise, this LGTM and my only questions are really on the comments themselves.

kbrock · 2023-03-08T21:50:05Z

@jrafanie I will pull out the first commit with comments.
They touch a number of files. would be best to have this PR touch as few files as possible

kbrock · 2023-03-09T16:23:28Z

update:

rebased
removed comment commit (down to touching 4 files - 2 code and 2 specs)

@jrafanie I removed that comment so it only touches MiqExpression and that one call from Rbac.

question Would you feel more comfortable if we introduced to_ruby_only - basically to_ruby but with its own @ruby_only cache?

jrafanie · 2023-03-09T18:36:06Z

@Fryguy @kbrock what are your thoughts on this? I think it's good but would probably like to merge and not yet backport it to petrosian so we can test it on master for a bit.

kbrock · 2023-03-10T01:40:03Z

@jrafanie If you feel more comfortable, we can always have @ruby and @ruby_only. Either as 2 methods or the method using the param to determine the source.

Going forward, I would like to build upon this significantly #22397 (fry really wants to 🔥 eval and I don't blame him)

I'm ok keeping on master only.

Will possibly introduce some backport fun in the future as I plan on converting the get_col_info calls over to Field in reports (core, ui classic controller, and views).

kbrock · 2023-03-22T21:05:03Z

@Fryguy is now a good time to get this in?

kbrock · 2023-03-31T01:09:57Z

update:

added spec to ensure that to_ruby busted the cache when different arguments were passed in.

kbrock · 2023-04-13T22:00:11Z

update:

renamed reduce_exp to prune_exp since it prunes the expression according to the desired target (per @jrafanie input)

kbrock · 2023-05-03T02:01:39Z

update:

rebased to latest master
fixed 2 rubocop whitespace issues

kbrock · 2023-05-03T15:36:37Z

update:

pulled busting cache into own commit

The goal is to prune nodes from an expression tree that are not applicable to our target language When generating sql, we only want to process nodes that are sql friendly. In the case that both sql and ruby are in and OR clause, we will need to run this in ruby, so both are removed. Logically you can not split up the entries in the OR clause. When generating ruby, we only want to process ruby nodes. Or in the case that both sql and ruby are needed together, we deal with both. This is based upon the original preprocess_for_sql, but it allows flexibility for choosing to keep ruby OR sql. This all came about because in Rbac we are filtering in both sql and ruby. The sql filter was reduced, but the ruby filter contained both the ruby and sql filters. It doesn't make sense to run the same sql filters when processing ruby since those have already been run. In a followup commit, there is a note around demorgan's law which may help you understand a little more about how these various components are splitup

rbac#filter/search can use an miq expression to filter the results. These expressions can be ruby only, sql, or a combination of both. The issue is the code was smart enough to send the sql the WHERE clause that was sql friendly. But when we were filtering in ruby, we performed all the sql filtering and ruby filtering. For 1k records, this takes a hit. After We send the sql only filters to sql and the ruby only filters to ruby. In cases where the OR mixes sql friendly and ruby friendly parameters, we still perform the sql friendly filters in ruby. (logically it is the only way to do it)

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

kbrock · 2023-05-04T22:13:39Z

update:

moved a change line from one commit to the busting cache commit (all worked, end result the same, but that commit would not have worked without this line change)
rebased

miq-bot · 2023-05-04T22:15:58Z

Checked commits kbrock/manageiq@126eeb6~...029bc1b with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
4 files checked, 0 offenses detected
Everything looks fine. 🍰

kbrock · 2023-05-12T03:44:49Z

Just ran into this today. This will speed up timelines quite a bit.

Fryguy · 2023-05-12T20:15:37Z

LGTM - merging.

kbrock requested review from agrare and Fryguy as code owners February 9, 2023 05:03

miq-bot added the unmergeable label Feb 10, 2023

kbrock force-pushed the miq_expression_to_ruby branch 3 times, most recently from 6bcbc30 to 5640b59 Compare February 13, 2023 07:40

This was referenced Feb 13, 2023

Properly parse MiqExpression::CountField #22355

Merged

Extract MiqExpression#expression_prep from preprocess_for_sql #22356

Merged

kbrock force-pushed the miq_expression_to_ruby branch 2 times, most recently from a73a19c to 2dfaac8 Compare February 13, 2023 17:47

kbrock force-pushed the miq_expression_to_ruby branch from 2dfaac8 to 57394e3 Compare February 13, 2023 18:38

miq-bot removed the unmergeable label Feb 13, 2023

kbrock force-pushed the miq_expression_to_ruby branch from 57394e3 to 07ed0b8 Compare February 13, 2023 19:31

kbrock force-pushed the miq_expression_to_ruby branch from 07ed0b8 to 0b44ee2 Compare February 13, 2023 19:53

kbrock force-pushed the miq_expression_to_ruby branch from 0b44ee2 to 161584c Compare February 14, 2023 18:41

Fryguy assigned jrafanie and Fryguy Feb 15, 2023

Fryguy added the performance label Feb 15, 2023

miq-bot added the unmergeable label Feb 21, 2023

Fryguy reviewed Feb 21, 2023

View reviewed changes

lib/miq_expression.rb Outdated Show resolved Hide resolved

kbrock force-pushed the miq_expression_to_ruby branch from 161584c to 74b09b6 Compare February 21, 2023 17:01

Fryguy reviewed Feb 21, 2023

View reviewed changes

lib/miq_expression.rb Outdated Show resolved Hide resolved

Fryguy reviewed Feb 21, 2023

View reviewed changes

Fryguy approved these changes Feb 21, 2023

View reviewed changes

kbrock force-pushed the miq_expression_to_ruby branch from 74b09b6 to 940b212 Compare February 21, 2023 20:47

kbrock force-pushed the miq_expression_to_ruby branch from 9fce8e8 to 79170ed Compare March 9, 2023 16:21

kbrock added the petrosian/no label Mar 28, 2023

kbrock force-pushed the miq_expression_to_ruby branch 2 times, most recently from f0532de to 36a4f40 Compare March 31, 2023 01:09

kbrock force-pushed the miq_expression_to_ruby branch from 36a4f40 to 416bade Compare April 13, 2023 21:59

kbrock force-pushed the miq_expression_to_ruby branch from 416bade to 1555995 Compare May 3, 2023 02:01

kbrock force-pushed the miq_expression_to_ruby branch from 1555995 to b62ee2b Compare May 3, 2023 15:36

kbrock added 5 commits May 4, 2023 18:12

bust cache for to_ruby when timezone changes

126eeb6

use MiqExpression#reduce_exp for sql

c4afefd

MiqExpression introduce demorgan's law

029bc1b

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

kbrock force-pushed the miq_expression_to_ruby branch from b62ee2b to 029bc1b Compare May 4, 2023 22:12

Fryguy merged commit 52c3b39 into ManageIQ:master May 12, 2023
9 checks passed

Fryguy removed the petrosian/no label May 12, 2023

kbrock deleted the miq_expression_to_ruby branch May 15, 2023 02:21

kbrock added the enhancement label Jun 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Miq expression removes sql only fields for filters #22347

Miq expression removes sql only fields for filters #22347

kbrock commented Feb 9, 2023 •

edited

kbrock commented Feb 13, 2023

kbrock commented Feb 13, 2023

kbrock commented Feb 13, 2023

kbrock commented Feb 14, 2023

kbrock commented Feb 14, 2023

Fryguy Feb 21, 2023

kbrock Feb 21, 2023

jrafanie Mar 1, 2023

kbrock Apr 13, 2023

Fryguy left a comment

kbrock commented Mar 8, 2023

kbrock commented Mar 9, 2023 •

edited

jrafanie commented Mar 9, 2023

kbrock commented Mar 10, 2023 •

edited

kbrock commented Mar 22, 2023

kbrock commented Mar 31, 2023

kbrock commented Apr 13, 2023 •

edited

kbrock commented May 3, 2023

kbrock commented May 3, 2023

kbrock commented May 4, 2023

miq-bot commented May 4, 2023

kbrock commented May 12, 2023

Fryguy commented May 12, 2023

Miq expression removes sql only fields for filters #22347

Miq expression removes sql only fields for filters #22347

Conversation

kbrock commented Feb 9, 2023 • edited

kbrock commented Feb 13, 2023

kbrock commented Feb 13, 2023

kbrock commented Feb 13, 2023

kbrock commented Feb 14, 2023

kbrock commented Feb 14, 2023

Fryguy Feb 21, 2023

Choose a reason for hiding this comment

kbrock Feb 21, 2023

Choose a reason for hiding this comment

jrafanie Mar 1, 2023

Choose a reason for hiding this comment

kbrock Apr 13, 2023

Choose a reason for hiding this comment

Fryguy left a comment

Choose a reason for hiding this comment

kbrock commented Mar 8, 2023

kbrock commented Mar 9, 2023 • edited

jrafanie commented Mar 9, 2023

kbrock commented Mar 10, 2023 • edited

kbrock commented Mar 22, 2023

kbrock commented Mar 31, 2023

kbrock commented Apr 13, 2023 • edited

kbrock commented May 3, 2023

kbrock commented May 3, 2023

kbrock commented May 4, 2023

miq-bot commented May 4, 2023

kbrock commented May 12, 2023

Fryguy commented May 12, 2023

kbrock commented Feb 9, 2023 •

edited

kbrock commented Mar 9, 2023 •

edited

kbrock commented Mar 10, 2023 •

edited

kbrock commented Apr 13, 2023 •

edited