Do not evaluate IF's predicate twice

[d-krushinsky]

Problem:

with current "short-circuit" approach predicate value can be evaluated twice, which can lead to performance issues (in case when predicate is huge and complex), and also can lead to issues in code that depends on dependencies array (#197)

for example doing some heavy operation inside of predicate (making DB call, net request or just doing huge amount of calculations)

require "dentaku"
require "benchmark"

# simulate heavy predicate
heavy_func = ->(value) {
  puts "sleep for 0.5 sec"
  sleep(0.5)
  value
}

calculator = Dentaku::Calculator.new
calculator.add_function("heavy_func", :numeric, heavy_func)

time = Benchmark.measure do
  calculator.evaluate("IF(heavy_func(2) > 1, 42, 0)")
end
puts time.real

will result in

sleep for 0.5 sec
sleep for 0.5 sec
1.0005817290002597

or using some randomness #dependencies can return different values

require "dentaku"

random_func = ->() { rand(1..10) }
calculator = Dentaku::Calculator.new
calculator.add_function("random", :numeric, random_func)

puts calculator.dependencies("IF(random() > 5, value_a, value_b)").inspect

will result in either

["value_a"]

or

["value_b"]

Proposal

do not evaluate predicate at all and build dependencies for all args of IF each time

[rubysolo]

Thanks for the PR and the well-constructed examples — you've hit on something we deliberated over quite a bit for 4.0.

I'm going to decline this approach, though, because it reverses a decision we just committed to for the 4.0 release (currently at 4.0.0.pre). The short version: dependencies(context) is intentionally resolution-aware. When a guard position (an IF predicate, a CASE switch, an AND/OR operand) can already be resolved from the supplied context, it's evaluated so that branches that cannot be taken are pruned. That's what makes this work:

calculator.evaluate!('IF(x > 5, y, z)', x: 7, y: 1)  #=> 1

z is never needed, so no UnboundVariableError is raised. With this PR, evaluate!'s unbound-variable check would see z in the dependency list and raise — a regression against the short-circuit semantics 4.0 documents (and which #234 asked for). It would also make IF inconsistent with CASE, AND, and OR, which all still prune in guard positions.

For your dependencies use case (#197): the static view you want already exists — call dependencies(expression) without a context and every identifier the formula mentions is listed, all branches included. Resolution-aware pruning only kicks in for guards whose own dependencies are satisfied by the context you pass.

On random(): 4.0's README now documents a purity contract — Dentaku treats formulas as pure, and custom functions must not rely on side effects or call counts. A nondeterministic function in a guard position is outside that contract, so nondeterministic dependencies output there is expected behavior rather than a bug.

That said, your first complaint is completely legitimate: a heavy predicate really is executed twice under evaluate! (once during the dependency pre-check, once during evaluation), and the purity contract makes that semantically fine but doesn't make it cheap. That seems worth exploring — but as a performance optimization that preserves the pruning semantics (e.g., memoizing guard values across the dependency check and evaluation), rather than by removing pruning. If you'd be interested in reworking this PR in that direction, I'd happily review it.

Thanks again for digging into this!

[d-krushinsky]

Dentaku treats formulas as pure, and custom functions must not rely on side effects or call counts

Could you please explain what are the benefits of this decision?
Our formulas and custom functions depends on user input, but we calculate dependencies before formula is used to show what should be provided by user, to do that we build dependencies for mock-user that doesn't have any state
here is example of how we use it

USER_ATTR_FUNC = ->(attr_name, default = 0) { Current.user.attributes[attr_name] || default }

calculator = Dentaku::Calculator.new
calculator.add_function(:user_attr, :numeric, USER_ATTR_FUNC)

# Build dependencies for formula to send through API
formula = "IF(user_attr('level') > 50, high_level_value, low_level_value)" # we are storing formulas in db, but here I wil omit that
Current.user = User.new # mock-user
deps = calculator.dependencies(formula)
# we are expecting ["high_level_value", "low_level_value"] so other services will know what should be provided
# but in reality we will get only ["low_level_value"] since function will fallback to default 0 value for mock-user

We also have functions for random float and random integer, and other functions that can return different values depends on some external factors which may vary from time to time, so from my point of view this decision just reduce amount of possible use-cases of this gem

call dependencies(expression) without a context and every identifier the formula mentions is listed

unfortunately with our use-case above this will not return every identifier

[rubysolo]

That consideration with a custom function makes sense. What if we added a way to mark a function as non-pure that would prevent evaluation in the validation phase? Your example would become something like:

USER_ATTR_FUNC = ->(attr_name, default = 0) { Current.user.attributes[attr_name] || default }

calculator = Dentaku::Calculator.new
calculator.add_function(:user_attr, :numeric, USER_ATTR_FUNC, non_pure: true)

Then Dentaku would skip the short circuit eval and return dependencies from both branches of the IF.

[d-krushinsky]

Imho non_pure flag feels like a hack and will just add more complexity to gem's logic and will eat a little bit of performance since it's an extra check on each IF (with v4.0 I guess also on each OR, AND, etc.), and we have very complex formulas (1500+ AST tokens) with a lot of IF/OR/AND so for us this extra check can be noticeable

[rubysolo]

Those are both fair points.

Regarding performance: purity is a property of the parsed AST, so it would be computed once per node at construction and memoized — the check at dependency-resolution time is a boolean read per guard, not an extra tree walk. Compared to the existing tokenization/parsing (and the dependency walk itself), the overhead should be negligible, especially with AST caching. (But I will measure.) EDIT: The preformance difference I'm seeing is between 1 and 2 percent.

On whether the flag is a "hack" — I'd argue the opposite. As the definer of a custom function, you're the only one who knows whether it's pure / side-effect-free; the gem can't infer that from a lambda. A registration-time declaration is how spreadsheets handle this exact problem (e.g. Excel's "volatile" functions — NOW(), RAND()). The only alternative I see is treating all custom functions as opaque in guard positions, which would disable short-circuiting even for users whose functions are pure. And it's worth noting the flag would also fix the double-evaluation in your original benchmark: since the dependency pre-check would skip a non-pure predicate, heavy_func would run once per evaluate! instead of twice. The caveat to flagging functions is that for any conditional whose predicate uses a non-pure function, evaluate! can't know which branch will be taken, so variables from both branches must be bound to avoid an UnboundVariableError.

Separately — for your dependency-listing use case specifically, I think the cleaner answer is a method that returns all identifiers from the AST, regardless of branching. It wouldn't evaluate anything or require you to opt-in to the new flag (although the flag would give you the performance benefit of not double-eval'ing your function). That would give consumers the complete list of possible inputs directly. Would that combination cover your cases?

[d-krushinsky]

I think the cleaner answer is a method that returns all identifiers from the AST, regardless of branching

Yeah this will be nice (that's what I initially thought #dependencies are doing 🙃)

[d-krushinsky]

for our use case I wrote custom ast cache with entry that wraps root node like this (along with IF monkey-patch to always return full list of deps)

class Entry < SimpleDelegator
  attr_reader :version

  def initialize(obj, version: nil)
    super(obj)
    dependencies({})
    @version = version
  end

  def dependencies(context = {})
    @dependencies ||= __getobj__.dependencies({})
    @dependencies - context.keys # #dependencies should return unbounded deps
  end
end

so we calculate dependencies only once since #dependencies is not cheap call, for huge formulas it really make a big difference in performance

and even with this cache+IF patch #evaluate! will still raise Dentaku::UnboundVariableError

I think you are right and volatile is not a "hack" as I called it, but I think it make more sense if you cache values of AST nodes and constantly re-calculate this formula (like Excel spreadsheet does) and you want to some how invalidate that cache (e.g. if value that node is dependent on is changed or if node depends on non-pure function), but right now (if I understand dentaku code correctly) there is no "node value caching". So for me still "volatile" looks like a workaround for short-circuit deps calcualtion which is not necessary since even without that evaluate! will check if there are some unbounded deps (raised in Dentaku::AST::Identifier#value, but only issue here is that it will raise it only for one identifier)