Skip to content

refactor: compile the selected_cols for the ResultNode #1765

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

Merged
merged 6 commits into from
May 28, 2025

Conversation

chelsea-lin
Copy link
Contributor

No description provided.

@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: bigquery Issues related to the googleapis/python-bigquery-dataframes API. labels May 21, 2025
@chelsea-lin chelsea-lin force-pushed the main_chelsealin_sqlglot_selections branch from 813b50e to b8a7e33 Compare May 21, 2025 23:55
@chelsea-lin chelsea-lin marked this pull request as ready for review May 21, 2025 23:56
@chelsea-lin chelsea-lin requested review from a team as code owners May 21, 2025 23:56
@chelsea-lin chelsea-lin force-pushed the main_chelsealin_sqlglot_selections branch from b8a7e33 to 02959ef Compare May 22, 2025 17:52
If it is, returns a tuple containing the alias name and original column name.
Returns `None` otherwise.
"""
if not isinstance(expr, sge.Alias):
Copy link
Contributor

Choose a reason for hiding this comment

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

non-rhetorical question: is it possible to check alias at the BFET level?

We have something like this:

class AliasedRef(typing.NamedTuple):
ref: ex.DerefOp
id: identifiers.ColumnId
@classmethod
def identity(cls, id: identifiers.ColumnId) -> AliasedRef:
return cls(ex.DerefOp(id), id)
def remap_vars(
self, mappings: Mapping[identifiers.ColumnId, identifiers.ColumnId]
) -> AliasedRef:
return AliasedRef(self.ref, mappings.get(self.id, self.id))
def remap_refs(
self, mappings: Mapping[identifiers.ColumnId, identifiers.ColumnId]
) -> AliasedRef:
return AliasedRef(ex.DerefOp(mappings.get(self.ref.id, self.ref.id)), self.id)

Checking BigFrames objects instead of SQLGlot ones might make or architecture cleaner.

This suggestion is totally optional, though.

Copy link
Contributor Author

@chelsea-lin chelsea-lin May 22, 2025

Choose a reason for hiding this comment

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

That's valid concerns. I created b/419595879 to revisit the issues considering the complexity of current architecture.

@chelsea-lin chelsea-lin force-pushed the main_chelsealin_sqlglot_selections branch from 02959ef to 36b98f7 Compare May 22, 2025 20:03
@chelsea-lin chelsea-lin requested a review from sycai May 22, 2025 21:53
@chelsea-lin chelsea-lin added the kokoro:run Add this label to force Kokoro to re-run the tests. label May 23, 2025
@bigframes-bot bigframes-bot removed the kokoro:run Add this label to force Kokoro to re-run the tests. label May 27, 2025
@chelsea-lin chelsea-lin merged commit ad5b98c into main May 28, 2025
23 of 24 checks passed
@chelsea-lin chelsea-lin deleted the main_chelsealin_sqlglot_selections branch May 28, 2025 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the googleapis/python-bigquery-dataframes API. size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants