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

Compiler for Oracle queries #56

Closed
jonslo opened this issue Feb 25, 2018 · 20 comments
Closed

Compiler for Oracle queries #56

jonslo opened this issue Feb 25, 2018 · 20 comments

Comments

@jonslo
Copy link

jonslo commented Feb 25, 2018

This library is fantastic... any chance that an Oracle compiler is on the roadmap? Thanks!

@ahmad-moussawi
Copy link
Contributor

Thanks, sadly there is no plan to add support for it in the near future, it would be great if you can help, either by making a pull request, or by giving a hint on what are the major differences, so we can implement one.

@tlaguz
Copy link
Contributor

tlaguz commented Aug 1, 2018

I'm writing Oracle compiler now. I don't know exact changes between pgsql and oracle, so I'm implementing only features I need. I will make PR when it will be ready.

@jonslo If you would like to cooperate on this, please contact me.

@ahmad-moussawi
Copy link
Contributor

@tlaguz that would be awesome, these sections may contains difference
https://sqlkata.com/docs/where-string
https://sqlkata.com/docs/where-date
https://sqlkata.com/docs/limit

If you need any help please let me know

@tlaguz
Copy link
Contributor

tlaguz commented Aug 1, 2018

I've created WIP branch in my fork. You can find compiler class here: https://github.com/tlaguz/querybuilder/blob/oracle/QueryBuilder/Compilers/Oracle11gCompiler.cs

As you can see I've managed to do limit and offset proof of concept.

What do you think about making integration tests against real database? Simple statements are easy to test in unit tests, but limit with offset for example is getting messy.

 db.Query("CUSTOMERS").Limit(5).Offset(10).Get()

produces:

SELECT * FROM (SELECT "a".*, ROWNUM r__ FROM (SELECT * FROM "CUSTOMERS") "a" WHERE ROWNUM <= 15) WHERE r__ > 10

IMO It should be also tested with some ORDER BY and GROUP BY statements to check if everything is compiling correctly. Should this case be tested in unit tests as well?

I created a docker container with Oracle 11 express and example database structure inside for my own testing purposes. I can commit this it if you like. It would be hard to automate these tests, but it eases programming a lot.

@ahmad-moussawi
Copy link
Contributor

mutating the same queries lead to many issues before, especially in this case, an example would be when compiling the same query twice, am not sure if it's still a problem right now,
check the SqlServerCompiler (Legacy approach) i think it's similar
https://github.com/sqlkata/querybuilder/blob/master/QueryBuilder/Compilers/SqlServerCompiler.cs#L17

Related issues:
#108 #82

@tlaguz
Copy link
Contributor

tlaguz commented Aug 1, 2018

Thank you for advice. I will change my code.

I will need unique (in query scope) alias generator to support nested limited queries. The only place I found which is shared between query and its subqueries is Query class itself, because it has AbstractQuery.Parent variable. The problem is the Parent var is not copied by Copy function. Is this an expected behavior?
If it's not, I would propose something like this:

    public abstract class AbstractQuery
    {
        protected AbstractQuery Parent;
        
        public uint AliasCounter { get; protected set; }
        public string GetUniqueAlias => Parent != null ? Parent.GetUniqueAlias : $"SqlKata_{AliasCounter++}__";
    }

What would be the best solution?

@ahmad-moussawi
Copy link
Contributor

While in practice these kind of query nesting should be avoided, but in fact it's possible, I prefer if we keep this to another issue, and try to find a solution for it later, since this will affect the SqlServer compiler also.

we can leave it for now, so we assume that nested limited queries are not supported at the moment.
one approach would be what you have suggested, another maybe using random generator, etc ...

regarding the Parent prop, it was used in previous versions, and currently there is no clear usage for it, so yes maybe it will get removed in future release unless we found a proper use for it.

beside note, will searching I found that Oracle also support the standard FETCH NEXT OFFSET, like in sql server, so following the same strategy as Sql Server compiler will be a good fit here.

What do you think ?

@tlaguz
Copy link
Contributor

tlaguz commented Aug 1, 2018

beside note, will searching I found that Oracle also support the standard FETCH NEXT OFFSET, like in sql server, so following the same strategy as Sql Server compiler will be a good fit here.

Unfortunately this is supported since Oracle 12c. This is why I decided to name compiler class Oracle11gCompiler to highlight database's target version.
There should be also 12c compiler, but 11g is my priority, since most of our clients won't upgrade their databases soon.

@mathijs-dumon
Copy link
Contributor

Any progress on this? How functional is the oracle compiler?

@mathijs-dumon
Copy link
Contributor

Coming back to this, I've pulled the fork and rebased/merged it with the trunk, written out a few unit tests for oracle11g and all seems to be fine. If I get round to finishing the tests, can I make a pull request? Or is the alias issue preventing this?

@ahmad-moussawi
Copy link
Contributor

@mathijs-dumon thanks for the progress, @tlaguz do you have any input on this ?

@tlaguz
Copy link
Contributor

tlaguz commented Oct 20, 2018

#126 issue is still not solved. Oracle have different binding format than every dbms currently supported by the sqlkata, every supported dbms handles @... and Oracle requires :..., so there is no easy way to adapt existing code.

I can't tell if this is the last issue.

@ahmad-moussawi
Copy link
Contributor

Could you make a pull request on a different branch ?

@ahmad-moussawi
Copy link
Contributor

@mathijs-dumon @tlaguz We can safely close this issue now right ?

@mathijs-dumon
Copy link
Contributor

I'd say so, if there are issues, I'll likely commit fixes for them anyway.

@ahmad-moussawi
Copy link
Contributor

That's great!

@iquirino
Copy link

What about Oracle Sequences?

@mathijs-dumon
Copy link
Contributor

maybe using the new db.Statement method? (see #94)
I think it's not yet in the docs

@ahmad-moussawi
Copy link
Contributor

maybe using the new db.Statement method? (see #94)
I think it's not yet in the docs

Actually it's https://sqlkata.com/docs/execution/fetching-records#execute-raw-statements

@iquirino
Copy link

If i use raw statements, why should i use SqlKata?
I'm using SqlKata to generate my dynamic queries...

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

No branches or pull requests

5 participants