Skip to content

sql: Fix: Don't do join identity elision when it would produce wrong results#35414

Merged
def- merged 1 commit intoMaterializeInc:mainfrom
def-:pr-join-identity
Mar 13, 2026
Merged

sql: Fix: Don't do join identity elision when it would produce wrong results#35414
def- merged 1 commit intoMaterializeInc:mainfrom
def-:pr-join-identity

Conversation

@def-
Copy link
Copy Markdown
Contributor

@def- def- commented Mar 11, 2026

Introduced in #9623. Leads to wrong results, see the attached SLT, which fails like this without the product code change:

    SELECT * FROM (SELECT) AS sub LEFT JOIN empty_t ON true
    OutputFailure:test/sqllogictest/join-identity-elision.slt:35
            expected: Values(["NULL", "NULL"])
            actually: Values([])
            actual raw: []
    ----
    SELECT * FROM empty_t RIGHT JOIN (SELECT) AS sub ON true
    OutputFailure:test/sqllogictest/join-identity-elision.slt:50
            expected: Values(["NULL", "NULL"])
            actually: Values([])
            actual raw: []
    ----
    SELECT * FROM (SELECT) AS sub FULL JOIN empty_t ON true
    OutputFailure:test/sqllogictest/join-identity-elision.slt:58
            expected: Values(["NULL", "NULL"])
            actually: Values([])
            actual raw: []
    ----
    SELECT * FROM empty_t FULL JOIN (SELECT) AS sub ON true
    OutputFailure:test/sqllogictest/join-identity-elision.slt:66
            expected: Values(["NULL", "NULL"])
            actually: Values([])
            actual raw: []
    ----
    FAIL: output-failure=4 success=5 total=9

@def- def- requested a review from mgree March 11, 2026 09:30
@def- def- requested a review from a team as a code owner March 11, 2026 09:30
@def- def- requested a review from ohbadiah March 11, 2026 09:30
@def- def- changed the title sql: Fix: Don't do join identity elision when it would produce wrong resutls sql: Fix: Don't do join identity elision when it would produce wrong results Mar 11, 2026
@github-actions
Copy link
Copy Markdown
Contributor

Thanks for opening this PR! Here are a few tips to help make the review process smooth for everyone.

PR title guidelines

  • Use imperative mood: "Fix X" not "Fixed X" or "Fixes X"
  • Be specific: "Fix panic in catalog sync when controller restarts" not "Fix bug" or "Update catalog code"
  • Prefix with area if helpful: compute: , storage: , adapter: , sql:

Pre-merge checklist

  • The PR title is descriptive and will make sense in the git log.
  • This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
  • If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).

Copy link
Copy Markdown
Contributor

@mgree mgree left a comment

Choose a reason for hiding this comment

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

Tiny style question/suggestion. Looks good to me!

How did you find this?

Comment thread src/sql/src/plan/hir.rs Outdated
@def- def- force-pushed the pr-join-identity branch from 2764720 to f32efcd Compare March 11, 2026 15:56
@def-
Copy link
Copy Markdown
Contributor Author

def- commented Mar 11, 2026

I'm doing something very similar to @aljoscha (see #skunkworks) to burn my remaining Claude tokens every week. My current prompt.md was:

Our goal is to find bugs in Materialize, like wrong results, deadlocks, panics. We don't care about finding function/postgres compatibility, casting problems at the moment. Systematically go through the relevant parts of the source code (src/), mark in @prompt.md which areas have been worked on and which are missing. Analyze one area of the code per session, take note by only appending to @log.md about bugs you have found.

In each session try analyzing one area of code, note down the results in our log file and stop! I'll start a new session to try and tackle the next thing.

Then I look for anything interesting in log.md and iterate from there.

For the future we probably want some prompt like that running against each PR before merging, but there are still lots of false positives, so I wouldn't want to directly subject our developers to that. Might be ok to send the potential issues to myself so I can triage and then report the actual ones. For now still a lot left to go through in the existing code.

@def- def- merged commit 77b8764 into MaterializeInc:main Mar 13, 2026
126 checks passed
@def- def- deleted the pr-join-identity branch March 13, 2026 06:27
antiguru pushed a commit to antiguru/materialize that referenced this pull request Mar 26, 2026
…results (MaterializeInc#35414)

Introduced in MaterializeInc#9623.
Leads to wrong results, see the attached SLT, which fails like this
without the product code change:
```
    SELECT * FROM (SELECT) AS sub LEFT JOIN empty_t ON true
    OutputFailure:test/sqllogictest/join-identity-elision.slt:35
            expected: Values(["NULL", "NULL"])
            actually: Values([])
            actual raw: []
    ----
    SELECT * FROM empty_t RIGHT JOIN (SELECT) AS sub ON true
    OutputFailure:test/sqllogictest/join-identity-elision.slt:50
            expected: Values(["NULL", "NULL"])
            actually: Values([])
            actual raw: []
    ----
    SELECT * FROM (SELECT) AS sub FULL JOIN empty_t ON true
    OutputFailure:test/sqllogictest/join-identity-elision.slt:58
            expected: Values(["NULL", "NULL"])
            actually: Values([])
            actual raw: []
    ----
    SELECT * FROM empty_t FULL JOIN (SELECT) AS sub ON true
    OutputFailure:test/sqllogictest/join-identity-elision.slt:66
            expected: Values(["NULL", "NULL"])
            actually: Values([])
            actual raw: []
    ----
    FAIL: output-failure=4 success=5 total=9
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants