-
Notifications
You must be signed in to change notification settings - Fork 1.7k
chore(deps): Update sqlparser to 0.58 #16456
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
(converted to draft while I'm investigating CI failures) |
(one of the test failures is related to #1898 and would be fixed if that patch is backported to 0.56) |
0.58.0 is released: apache/datafusion-sqlparser-rs#1886 (comment) |
@Dimchikkk would you be able to pick up the work involving upgrade to 0.58 as mentioned? Otherwise I can build upon your work here to get this PR over the line 🙂 |
@Jefffrey yeah, you can take it. |
Moving to draft as I work to build upon this to update to 0.58 |
PR good for review now |
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.
Thank you @Jefffrey and @Dimchikkk . This is pretty epic
The only thing I am worried about is the direct use of "substr"
string -- I left an alternate suggestion. Let me know what you think
datafusion/sql/src/expr/substring.rs
Outdated
not_impl_err!( | ||
"Substring not supported by UserDefinedExtensionPlanners: {substring_args:?}" | ||
) | ||
let fun = self |
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.
This basically hard codes the use of a function named "substr" into the planner
I think the preferred way is to use the ExprPlanner
API, similarly to how plan field access is done here:
datafusion/datafusion/expr/src/planner.rs
Line 134 in d987d2d
fn plan_field_access( |
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'm not familiar with that API, though it seems to be used above already?
datafusion/datafusion/sql/src/expr/substring.rs
Lines 72 to 79 in c22d64b
for planner in self.context_provider.get_expr_planners() { | |
match planner.plan_substring(substring_args)? { | |
PlannerResult::Planned(expr) => return Ok(expr), | |
PlannerResult::Original(args) => { | |
substring_args = args; | |
} | |
} | |
} |
So this code here (that hardcodes the "substr"
string) is more of a fallback I guess? Might need some help understanding this part
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 spent some time debugging and I think the solution is that we needed to register the appropriate planner with the tests. I pushed a commit here: e46e532
Note I also found the name of the extension planner very confusing, and I will make a separate PR to fix it
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.
This makes sense to me, yeah. Was confusing to have that fallback behaviour 😅
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 it looks good now, though of course I am biased!
@Jefffrey let me know what you think
Also, thank you @Dimchikkk for starting this -- I would say it is a good team effort!
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.
thank you
@Jefffrey thanks for helping get this over the line. |
Thanks @Dimchikkk & all |
🚀 |
Which issue does this PR close?
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?