-
Notifications
You must be signed in to change notification settings - Fork 71
fix relational scoping bug introduced in #6495 #6498
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
Conversation
This commit fixes a bug where ORDER BY scope was improperly handled because the logic favored the input table over the column aliases but the materialized output table has priority over the input table once the SELECT body scope is left. This would typically be handled by first looking into the output table but we can't do this because we do expression matching on the sem-tree expressions and we can't mix and match input expressions with the output expressions. The fix is to look in the columns first (producing input table expressions) and matching those expressions to the grouped output table columns with the expression matcher.
|
This should fix the intermittent test failure in #6496 |
compiler/semantic/scope.go
Outdated
| // SELECT body (e.g., in ORDER BY), so we need to first check | ||
| // the select column names before the inputs. This is handled by | ||
| // other SQLs by looking in the output table, but we can't do that | ||
| // since we must can't mix output columns and input columns |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // since we must can't mix output columns and input columns | |
| // since we can't mix output columns and input columns |
| inputs: | ||
| - name: query.spq | ||
| data: | | ||
| pragma pg=false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this what you want? I was expecting pg=true based on the file name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, you're right. The bug was when PG scope was off and it was the output table causing the problem. I'll rename the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And remove pg=false
compiler/semantic/scope.go
Outdated
| // the binding of identifiers changes and query results can change. By | ||
| // resolving lateral aliases first, we avoid this problem. | ||
| // resolving lateral aliases first, we avoid this problem. This can | ||
| // be overridden with "pragma pg" to favor Postgres precedence. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
| // be overridden with "pragma pg" to favor Postgres precedence. | |
| // be overridden with "pragma pg" to favor PostgreSQL precedence. |
This commit fixes a bug where ORDER BY scope was improperly handled because the logic favored the input table over the column aliases but the materialized output table has priority over the input table once the SELECT body scope is left. This would typically be handled by first looking into the output table but we can't do this because we do expression matching on the sem-tree expressions and we can't mix and match input expressions with the output expressions. The fix is to look in the columns first (producing input table expressions) and matching those expressions to the grouped output table columns with the expression matcher.