Skip to content

Conversation

Tmonster
Copy link
Contributor

Fixes #5614

This was a tough one.

When the where binder attempted to bind the 'text' alias in the where clause, the column_alias_binder would bind it with a depth of 0 (something I added a long time ago). This was incorrect. We should be binding c0 with the correct depth count.

When it has a depth of 0, it is not planned as a correlated subquery. With a depth of 1 it is, this will fix the issue, but it would break some other correlated subquery tests. Specifically tests with a having clause.

Thankfully, the tests were only broken in such a way that the error was incorrect. After looking at the code, I decided to propagate up correlated subquery binding errors instead of stalling them. The previous flow was the following

  • attempt to bind an expression as if it wasn't a correlated subquery,
  • if some error X is encountered, attempt to bind the expression as if it is in a correlated subquery,
  • if some error Y is encountered, continue to attempt correlatedSubquery binding until there are no more context Binders.
  • If the expression cannot be bound, throw error X.

The new flow will return error Y (or the last seen error from correlatedSubquery binding).

This has the benefit of fixing some lateral join errors that were a bit cryptic

@github-actions github-actions bot marked this pull request as draft October 30, 2023 12:43
@Tmonster Tmonster marked this pull request as ready for review October 30, 2023 14:41
@github-actions github-actions bot marked this pull request as draft November 1, 2023 12:39
@Mytherin Mytherin marked this pull request as ready for review November 8, 2023 15:16
Copy link
Collaborator

@Mytherin Mytherin left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Great that you got this fixed - just one minor comment:

@github-actions github-actions bot marked this pull request as draft November 8, 2023 15:59
@Tmonster Tmonster marked this pull request as ready for review November 8, 2023 15:59
@Mytherin Mytherin merged commit fbb72d8 into duckdb:feature Nov 9, 2023
@Mytherin
Copy link
Collaborator

Mytherin commented Nov 9, 2023

Thanks!

krlmlr added a commit to duckdb/duckdb-r that referenced this pull request Dec 11, 2023
Merge pull request duckdb/duckdb#9392 from lnkuiper/parquet_encryption
Merge pull request duckdb/duckdb#9461 from hawkfish/merge-sort-trees
Merge pull request duckdb/duckdb#8788 from alnkesq/capi_create_enum_type
Merge pull request duckdb/duckdb#9513 from Tmonster/5614-database-invalidated
Merge pull request duckdb/duckdb#9622 from Mytherin/typescoping
Merge pull request duckdb/duckdb#9615 from hawkfish/strptime-infinity
Merge pull request duckdb/duckdb#9627 from Mytherin/attachifnotexists
Merge pull request duckdb/duckdb#9648 from samansmink/add-keep-alive-toggle
Merge pull request duckdb/duckdb#9638 from taniabogatsch/bench-refactor
Merge pull request duckdb/duckdb#9651 from Mytherin/getenv
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants