-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Minor: Simplify IdentTaker
#13609
Minor: Simplify IdentTaker
#13609
Conversation
@@ -622,21 +622,37 @@ pub fn object_name_to_table_reference( | |||
idents_to_table_reference(idents, enable_normalization) | |||
} | |||
|
|||
struct IdentTaker(Vec<Ident>); | |||
struct IdentTaker { | |||
normalizer: IdentNormalizer, |
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 point of the PR is to move normalizer into IdentTaker
so it isn't created again on each row
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, thanks @alamb
} | ||
|
||
/// Returns the number of remaining identifiers | ||
fn len(&self) -> usize { |
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.
👍
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! Thanks @alamb
Thanks @jonahgao for reviewing |
Which issue does this PR close?
Follow on to #13605
Rationale for this change
As suggested by @jonahgao on https://github.com/apache/datafusion/pull/13605/files#r1863695929
What changes are included in this PR?
Are these changes tested?
Yes, by CI
Are there any user-facing changes?
No, these are all internal apis