-
Notifications
You must be signed in to change notification settings - Fork 3
Call Expression Alias Patch #459
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
juankx-bodo
left a comment
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.
LGTM
hadia206
left a comment
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 job John.
Please address comment before merging.
pydough/conversion/column_bubbler.py
Outdated
| input_expr.name.startswith('"') | ||
| and input_expr.name.endswith('"') | ||
| or input_expr.name.startswith("`") | ||
| and input_expr.name.endswith("`") |
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.
Why use input_expr.name instead of input_name here?
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.
No particular reason but I'm gonna use it
pydough/conversion/column_bubbler.py
Outdated
| input_name: str = input_expr.name | ||
| quoted: bool = 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.
I think the better fix here is to not do any of this special handling, but instead just remove all characters from input_name except letters/numbers/underscores. We don't have to worry about conflicts because the point of this function is that it returns potential alternative names, and it only uses if them if those names are not already in use.
This resolves #458