-
Notifications
You must be signed in to change notification settings - Fork 68
refactor: Enable SELECT * optimization when compiling read-table nodes into sqlglot
#2430
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
base: main
Are you sure you want to change the base?
Conversation
e085050 to
f5a2276
Compare
| nodes.ScanItem( | ||
| identifiers.ColumnId(scan_item.source_id), scan_item.source_id | ||
| ) | ||
| for scan_item in node.scan_list.items |
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.
I think what we really want is to order by the underlying physical schema?
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.
When generating SQL, we should respect the original order of the selected columns and avoid reordering them to match the physical schema. While reordering can reveal more SELECT * optimizations when the query involves intermediate CTEs or subqueries, the current logic sorts the scan_list by the algebraic ordering of column names. This approach hides potential SELECT * optimizations, particularly in use cases like bpd.read_table("table_name")
bigframes/core/nodes.py
Outdated
| @property | ||
| def is_star_selection(self) -> bool: | ||
| physical_names = tuple(item.name for item in self.source.table.physical_schema) | ||
| scan_names = tuple(item.source_id for item in self.scan_list.items) |
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.
the scan list item id must equal source_id as well for this to work as intended
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.
Good points. It should be addressed in the new commit. Thanks.
f5a2276 to
15dad78
Compare
Fixes internal issue 481740136 🦕