Skip to content

Replace Method and CompositeAccess with CompoundFieldAccess - #1716

Merged
iffyio merged 1 commit into
apache:mainfrom
validio-io:compound-expr
Feb 19, 2025
Merged

Replace Method and CompositeAccess with CompoundFieldAccess#1716
iffyio merged 1 commit into
apache:mainfrom
validio-io:compound-expr

Conversation

@iffyio

Copy link
Copy Markdown
Contributor

Continuation of #1551

This moves over two of the special case compound expressions delimited by period to use the new CompoundFieldAccess representation.

It moves the logic to parse the various kinds of compound expression into the same function parse_compound_expr, where previously they were somewhat spread between individual expression parsing and the compound field access parsing.

@iffyio

Copy link
Copy Markdown
ContributorAuthor

FYI @goldmedal

@goldmedalgoldmedal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @iffyio 👍. It looks good to me. I think it's a nice improvement to remove the duplicate definition.

@iffyio
iffyio requested a review from alambFebruary 17, 2025 06:36
match expr {
Expr::Method(Method { expr, method_chain }) => {
assert!(matches!(*expr, Expr::Convert { .. }));
Expr::CompoundFieldAccess { root, access_chain } => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it is nice to have a unified AST representation here

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @iffyio

I read through this PR -- and reviewed the test changes carefully. While I don't claim to understand all of the code, it looks quite a bit nicer and cleaner to me. 🚀

Thank you very much. Nice work

Also thank you @goldmedal for the review

@iffyio

Copy link
Copy Markdown
ContributorAuthor

Thanks for the reviews @alamb@goldmedal 🙏

@alamb

Copy link
Copy Markdown
Contributor

Looks like this needs a merge up from main to resolve some conflicts

@iffyio

Copy link
Copy Markdown
ContributorAuthor

Yeah I'll resolve the conflicts!

@iffyio
iffyio merged commit 3e90a18 into apache:mainFeb 19, 2025
@iffyio
iffyio deleted the compound-expr branch February 19, 2025 17:49
ayman-sigma pushed a commit to sigmacomputing/sqlparser-rs that referenced this pull request Apr 10, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@iffyio@alamb@goldmedal