Skip to content
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

Support raw SQL projections in Select and OrderBy statements. #1927

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rwasef1830
Copy link
Contributor

Fixes bug #1926
Added for Select and OrderBy. It's a bit hacky but works and 2 passing unit tests enclosed.
From what I see, the LINQ parsing code and visitors need a general refactor.

@rwasef1830
Copy link
Contributor Author

rwasef1830 commented Oct 13, 2021

The linq provider code really needs a big refactoring, it's structured in a disjointed way with islands of functionality scattered in different visitors... when in reality it should be one big visitor which builds the SQL model in memory and is re-entrant (ie: any subexpression is reprocessed by the same visitor to allow complex recursive patterns) and then the final SQL model is applied onto the command builder (look at how Npgsql LINQ provider works).

I will re-review my code again to fit in better with the existing pattern and make it less of a hack, but in my opinion some sort of refactor and visitor unification is due.

@mysticmind
Copy link
Member

@rwasef1830 Acknowledge your comment regarding refactoring and structuring the LINQ provider code base. Possibly this would be part of 4.1.0 milestone.

Are you going to make some further refactoring on this PR before we review?

@rwasef1830 rwasef1830 marked this pull request as ready for review November 7, 2021 09:16
@rwasef1830
Copy link
Contributor Author

@mysticmind I can't find a way to make this better without a big surgery inside Marten. You can review it as-is and see what you think.

@mysticmind
Copy link
Member

@mysticmind I can't find a way to make this better without a big surgery inside Marten. You can review it as-is and see what you think.

Your prior comment "I will re-review my code again to fit in better with the existing pattern and make it less of a hack", nudged me to think that you would review and do some additional refactoring. As you indicated, will review the PR in the current form and keep you posted.

@jeremydmiller
Copy link
Member

jeremydmiller commented Dec 21, 2021

@rwasef1830 Just seeing this: "The linq provider code really needs a big refactoring"

Dude, the Linq support just got a massive restructuring in V4. Not gonna disagree that it could be cleaner, but I'll strongly disagree with you that it should be one giant visitor.

But we'd be very receptive to any concrete suggestions you have or if you'd even want to take on some of that work. The Select() clause parsing support especially is very old code that I haven't made any attempt to improve in years.

@rwasef1830
Copy link
Contributor Author

@jeremydmiller Even it's not going to be a big giant visitor, there's a very big part that needs to be a common base class of most visitors since any kind of expression can occur anywhere (in select or where or other clauses). At the moment, every clause has a separate visitor, some of which are aware of certain kinds of expressions that others are not even though these expressions can occur in all.

I'm not free now to take on this work these days, but it's an interesting challenge. I might take it on in the future.

@jeremydmiller
Copy link
Member

@rwasef1830 I'll agree with your last comment, and the newer parts would be using RelinqExpressionVisitor as a base for that very reason. The Select() transform parsing is Marten <V1 code that hasn't been touched much.

"I'm not free now to take on this work these days, but it's an interesting challenge. I might take it on in the future." -- and I was going to ask if you'd be interested in helping with that. You've done some pretty significant PRs already, and it sounds like you'd be able to. I'd like to grow the Marten core team next year if you'd ever be interested in that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants