Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug with missed columns in the column pruner #1679

Merged
merged 2 commits into from
Feb 23, 2025
Merged

Fix bug with missed columns in the column pruner #1679

merged 2 commits into from
Feb 23, 2025

Conversation

plypaul
Copy link
Contributor

@plypaul plypaul commented Feb 21, 2025

This PR fixes a bug where the column pruner does not correctly keep track of columns that are used in SqlStringExpression and SqlColumnAliasReferenceExpression. Those expressions are different from other expressions as they do not have a table alias, and there is existing case handling for those expressions. The bug is that the handler did not propagate the knowledge that columns from those expressions is required to the available CTEs.

This bug currently does not affect queries due to the way that we wrap access to CTEs in a SELECT, but fixing this as it affects later work.

@cla-bot cla-bot bot added the cla:yes label Feb 21, 2025
@plypaul plypaul changed the title Fix bug with missed columns in the column pruner. Fix bug with missed columns in the column pruner Feb 21, 2025
@plypaul plypaul marked this pull request as ready for review February 21, 2025 07:55
@plypaul plypaul requested a review from a team as a code owner February 21, 2025 07:55
plypaul added a commit that referenced this pull request Feb 21, 2025
This PR renames a few variables and methods in the column pruner classes
in preparation for #1679.
plypaul added a commit that referenced this pull request Feb 21, 2025
This PR consolidates a couple of loops in
`SqlMapRequiredColumnAliasesVisitor` and replaces `.add_alias()` calls
with `.add_aliases()` to reduce nesting. There are no logic changes in
this PR, and this is mainly to make
#1679 a simple change given
the complexity in that class.

There is some additional consolidation that can be done with other
loops, but leaving those out of scope.
Base automatically changed from p--sql--02 to main February 21, 2025 18:13
@plypaul plypaul merged commit 24204e7 into main Feb 23, 2025
11 checks passed
@plypaul plypaul deleted the p--sql--03 branch February 23, 2025 07:29
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