Epic: removing $step.eval()

[benjie]

I've never liked the eval system, but it seemed a requirement in the way that Grafast was architected.

As it turns out, a lot of the need for eval should be solved by the "global dependencies" feature we released in beta 22 (i.e. unary steps #1973).

Getting rid of eval entirely would be a massive step in the right direction for Grafast, it would take us closer to "one operation has one plan" rather than "one operation has at least one and potentially infinite plans" which is the current status quo.

However, this will be a significant undertaking because we need to rewrite a whole bunch of stuff to do with PgSelectStep and other steps that rely on eval currently. We may also need to factor in alternative approaches to plan resolvers. And even more so, we may need to factor it into query planning itself, since query planning performs evals to find out if certain paths are required or not - hopefully those can be addressed by "early exit" in combination with conditional OutputPlan fields.

Here's a rough plan I outlined for this:

---

Move connections/collections over to using "unary" steps rather than eval; we need this so that we can inhibit zeroes (but also because eval is evil):

• Make the limit (in the SQL) a placeholder rather than a literal
  • Can we actually have LIMIT 7 as a placeholder? This would be ideal because if the argument is null we want to omit LIMIT entirely rather than adding LIMIT 9999999999 or whatever
• At execute, get the associated unary step’s value, and look it up in an LRU cache
• If no match, build SQL using this new placeholder and write it to the LRU cache
• Execute as normal.

---

Change @skip/@include from using eval to instead using unary steps and early exit:

• At a skip/include boundary, all future steps should be STOPPED (like for polymorphism) if the skip/include doesn’t match.
• Output plans should factor in the skip/include into their logic; they can do this inline without the need to create duplicate output plans.
• As an optimization: we could perform the skip/include in OutputPlans using a bitmask; this would mean we could have at most 32 skip/include variables but that should be enough for most people. Failing that, we can do it as an array of ints and then go arbitrarily large.

---

Make it so argument "undefined" doesn’t use eval either...

• This one is going to be a challenge!

---

With unary steps and early exit, perhaps we can get rid of the need for eval entirely?

• Have eval log an error the first time it is used in a new position (development only!)
• Try and remove all other internal usages.
• Update documentation to remove references to eval.

---

Once eval no longer exists, we can implement that connection()’s clonedPlan.setFirst(plan) can pass through an inhibitOnZero version of the step. This wasn’t possible before because it had to be an input step so it could be eval’d.

• Create an "inhibitOnZero" (or similar) step - perhaps one that accepts conditionals?
• Make it so that connection’s first/last is wrapped in this
• "Trap" the inhibit at the nodes field, and return an empty array instead

---

As above, so below:

• Use "inhibitOnZero" and "trap" to apply to simple collections too - just limit the "first" argument

---

Note, if we achieve this then we should close #1950.

Notes from when I was working on early exit/global dependencies.

• Commit 79aa80f is probably raisable as its own PR.
  • Grafast: move to bitwise flag-based system internally #2014

Early exit foundations are laid, now it’s time to integrate into the rest of the system.

• Create an "inhibitOnNull" step (should be a wrapper around some underlying internal step)
• Introduce an inhibitOnNull step in the foo-by-node-id accessor fields - if the spec doesn’t match then no database fetch should happen
• The current issue is that the steps are not being correctly scanned, and hence not flagged as being null, and hence not being caught. IsSyncAndSafe saves us from having to look for promises or instanceof GrafastError but we still need to loop and look for nulls. Also applies to unbatchedExecute calls.
  • Flags are being returned by executeStep, need to ensure that they’re correct
• Fiddle with StepTracker.addStepDependency so that the "early exit"-ness is absorbed into the dependency adding, and doesn’t appear as a step in the final diagram
• Test that it works - the query plan should be identical, the result should be identical, but some of the SQL should be removed
• Raise this as a PR
  • Grafast: introduce inhibitOnNull #2015

MAJOR BUG:

• We’re not copying through the acceptFlags when we copy dependencies
• e.g. when cloning a connection’s collection step
• addDependency({ step, skipDeduplication, acceptFlags, ... })
• .getDep(0) => { step, skipDeduplication, acceptFlags, ... }
• What if .getDep itself returned a flagged step if any non-default flags were in use? That’d fix it!

Something to check that we handle correctly:

• Need to scan for $$inhibit along with null when scanning through step results and determining the flags
• Fix the error so the message comes through
• If a nodeID is nullable, then we need a way of allowing the null to mean null, but decoding it as an invalid value to throw an error - essentially inhibit if the spec is null UNLESS the underlying id itself is null.
  • Perhaps trap() that specific condition? Maybe decoding the wrong value should throw rather than inhibit?
  • Return GrafastInhibit
  • InhibitOnNullIffSecondPlanNotNull($spec, $nodeId)
  • Trap(FLAG_INHIBIT, $spec, null, {if: is($nodeId, null)})
  • InhibitOnCondition($first, [$a, $b], (f, a, b) => f <= 0 || a || b)
• How do flags factor into deduplicate? Must the flags/onReject match? I think so!
• What’s wrong with the description of __Flag steps
• Raise PR
  • Conditional trapping of inhibits; assertion; improve Relay NodeID compatibility #2019
  • Ready for merge

To make debugging easier:

• Add any non-default dependency information to the plan diagram, somehow
  • I’m thinking an annotation like the __FlagStep .toStringMeta, added to the dependency line. "rejectNull&allowError onReject: Error..." but only where it differs from the defaults.
• Raise this as a PR
  • Render flagged dependencies in mermaid #2022

Trap with replacement value

• ISSUE: list(assertNotNull(constant(null))) currently optimizes to constant([null]) which is WRONG!
  • If we’re having this happen, so will everyone else.
  • Solution: do not inline __FlagStep during planning.
  • Need to get new test file to pass
  • Optimization: add a phase after optimize where all the __FlagStep are inlined where possible.
  • Enable coercion of trapped values #2024
• makeWrapPlansPlugin around a function that returns null or array
• Replace null with empty array.
• Raise the PR

GrafastError shouldn’t be needed any more; we have error flag now?

• Remove all usages of GrafastError
  • $$inhibited is now a FlaggedValue
  • Errors should be wrapped with flagError()
• Raise this as a PR
• (DO NOT DELETE GrafastError class, we have a new case for it now, see next)

Make errors just values

• Don’t check for instanceof Error, instead look for GrafastError directly
• Get rid of TrappedError; no need.
• GraphQL resolver will need to re-emulate the error behavior. Should also do lists, based on type of field.

Other things before next release:

• Grafserv plugin system
  • Add support for loading validation rules like depth-limit
  • Add support for envelop
  • Need to ensure all execute() and subscribe() calls only accept one arg, and explicitly deprecate multi-arg form
• Detect presets that aren’t; scan for keys starting with a capital letter. Same for plugins.

Documentation:

• inhibitOnNull
• assertNotNull
• trap
• condition
• plan diagram rejectNull/trapError/trapInhibited
• What even is inhibit?
• Raise PR