Skip to content

Honor null-on-empty for-each quantifiers outside select boxes - #4489

Open
robert-brunel wants to merge 1 commit into
mainfrom
apple/robert-brunel/issue3431
Open

robert-brunel wants to merge 1 commit into
mainfrom
apple/robert-brunel/issue3431

Conversation

@robert-brunel

@robert-brunel robert-brunel commented Aug 21, 2026 •

Copy link
Copy Markdown
Contributor

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.

@robert-brunel robert-brunel self-assigned this Aug 21, 2026
@robert-brunel robert-brunel added the enhancement New feature or request label Aug 21, 2026

@alecgrieser alecgrieser left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
robert-brunel force-pushed the apple/robert-brunel/issue3431 branch 2 times, most recently from 0cf625b to 8380a3a Compare September 11, 2026 16:01
@robert-brunel

Copy link
Copy Markdown
Contributor Author

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 alecgrieser left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread yaml-tests/src/test/resources/join-tests-outer.yamsql Outdated
@robert-brunel
robert-brunel force-pushed the apple/robert-brunel/issue3431 branch 2 times, most recently from ad75bf3 to 45ac76d Compare September 15, 2026 18:11
@robert-brunel
robert-brunel added this pull request to stack #4658 September 23, 2026 18:02
Comment thread yaml-tests/src/test/resources/join-tests-outer.yamsql
@robert-brunel
robert-brunel force-pushed the apple/robert-brunel/issue3431 branch from d6f2a07 to c20e372 Compare September 29, 2026 19:49
@robert-brunel
robert-brunel removed this pull request from stack #4658 September 29, 2026 19:50
@robert-brunel
robert-brunel changed the base branch from main to apple/robert-brunel/noe-3 September 29, 2026 19:50
@robert-brunel
robert-brunel added this pull request to stack #4691 September 29, 2026 19:50
@robert-brunel
robert-brunel force-pushed the apple/robert-brunel/issue3431 branch from c20e372 to d44e94f Compare October 5, 2026 15:28
@robert-brunel
robert-brunel force-pushed the apple/robert-brunel/issue3431 branch 2 times, most recently from 4bec3cf to 4f64101 Compare October 9, 2026 17:15
Base automatically changed from apple/robert-brunel/noe-3 to main October 9, 2026 18:45
@robert-brunel
robert-brunel force-pushed the apple/robert-brunel/issue3431 branch 2 times, most recently from 3d4ab09 to a5b0bc6 Compare October 12, 2026 16:17
@robert-brunel
robert-brunel force-pushed the apple/robert-brunel/issue3431 branch from a5b0bc6 to 4f45af0 Compare October 13, 2026 10:31
@github-actions

Copy link
Copy Markdown

📊 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

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

For-each quantifiers with null-on-empty semantics only work as input to SelectExpressions

3 participants