[robert-brunel]

A ForEach quantifier with the isNullOnEmpty() flag flows a single NULL row when the sub-graph it ranges over is empty. However, so far, the flag is only honored if it sits directly under a SelectExpression, since only ImplementSimpleSelectRule and ImplementNestedLoopJoinRule ever inspect it. Anywhere else in a query graph it is silently ignored, which would lead to incorrect results if NoE ever makes it there.

To address this issue:

• Change RemoveSortRule, ImplementDistinctRule, ImplementUniqueRule, ImplementDistinctUnionRule, ImplementUnorderedUnionRule and ImplementIntersectionRule to correctly honor NoE quantifiers via Quantifier.applyGlue().
• Prevent PredicateToLogicalUnionRule from firing on a NoE quantifier, since the rewrite wouldn’t correctly preserve the semantics. This does not negatively affect index matching, as such a SELECT never has partial matches.
• Change all other rules that match for-each quantifiers but are not NoE-aware to match plain for-each quantifiers only, via plainForEachQuantifier*(). Such a rule now simply does not fire on a NoE quantifier. SplitSelectExtractIndependentQuantifiersRule and InComparisonToExplodeRule keep matching any for-each quantifier, since their rewrites preserve the semantics.
• Re-enable the two null-on-empty tests in FDBSimpleQueryGraphTest that were disabled pending For-each quantifiers with null-on-empty semantics only work as input to SelectExpressions #3431, and add tests there that cover the glue in the distinct, union, unique and sort rules (over both empty and non-empty inputs), and that a rule matching only plain for-each quantifiers refuses a NoE quantifier. Add outer-join queries with OR, IN and UNION ALL to join-tests-outer.yamsql.

Resolves #3431.

[alecgrieser]

A few minor things, but this mostly makes sense to me, though I would defer to @normen662. I think the new sanity check does help, but I am a bit worried that there's a bigger laundry list of places where we aren't quite handling forEachWithDefaultOnNull expressions correctly, though I'm not certain about what a better system for dealing with them would be. I guess I could see removing the default-on-empty facility from Quantifier.ForEach all together, and instead introducing a special logical expression here. But that would be a bigger change, and maybe there's a reason that would be a bad idea that I'm not aware of.

[robert-brunel]

I think the new sanity check does help, but I am a bit worried that there's a bigger laundry list of places where we aren't quite handling forEachWithDefaultOnNull expressions correctly

This worry is now mostly addressed (and the sanity check gone) by the QuantifierMatchers changes.

[alecgrieser]

Overall, I think this makes sense. There are a few minor things, but the general approach looks okay to me, once the test gaps are addressed

[github-actions]

📊 Metrics Diff Analysis Report

Summary

• New queries: 8
• Dropped queries: 0
• Plan changed + metrics changed: 0
• Plan unchanged + metrics changed: 0

ℹ️ About this analysis

This automated analysis compares query planner metrics between the base branch and this PR. It categorizes changes into:

• New queries: Queries added in this PR
• Dropped queries: Queries removed in this PR. These should be reviewed to ensure we are not losing coverage.
• Plan changed + metrics changed: The query plan has changed along with planner metrics.
• Metrics only changed: Same plan but different metrics

The last category in particular may indicate planner regressions that should be investigated.

New Queries

Count of new queries by file:

• yaml-tests/src/test/resources/join-tests-outer.metrics.yaml: 8