Paging for MsSql2012 changed to the new recommended standard syntax (replaces #78) #117

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@csharper2010

Created new MSSQL2012 Dialect. Implemented new paging syntax with
select xxx from yyy order by zzz offset @p1 rows fetch first @p2 rows only
Existing paging unit tests work fine with it.

Recreated because of the concurrent changes on MSSQL2005Dialect.

@csharper2010

Closed original Pull request #78

@hazzik
Member
hazzik commented May 25, 2012

Some test are failing

@csharper2010

Sorry, I'll check that.

@hazzik
Member
hazzik commented May 25, 2012

No problems.

@oskarb oskarb and 1 other commented on an outdated diff May 28, 2012
src/NHibernate/Dialect/MsSql2012Dialect.cs
+ {
+ //unmatched paren count
+ int unmatchedParen = 0;
+
+ //increment the counts based in the opening and closing parens in the statement
+ foreach (char item in statement)
+ {
+ switch (item)
+ {
+ case '(':
+ unmatchedParen++;
+ break;
+ case ')':
+ unmatchedParen--;
+ break;
+ }
@oskarb
oskarb May 28, 2012 NHibernate member

This algorithm will return true for cases like "... ) ... ( ...". A possible fix would be to check that unmatchedParen never becomes negative.

@csharper2010
csharper2010 May 28, 2012

Yep, you're right. The parser for matching parentheses is not perfect. But then the MsSql2005 dialect suffers from the same problems because the method is a copy from there to get a quick start in trying that out.
Also when using direct SQL queries, eventually contained comments with parentheses in them might lead to incorrect results. Are there more such simple parsers somewhere in the NHibernate codebase? Maybe this aspect could be covered more centrally.

@oskarb
oskarb May 29, 2012 NHibernate member

There is some code in #63 for more advanced SQL query parsing. I've not gotten around to review it. I just discovered there is actually an implementation for MSSQL 2012 in there as well.

@oskarb oskarb and 2 others commented on an outdated diff May 28, 2012
src/NHibernate/Dialect/MsSql2005Dialect.cs
@@ -33,6 +34,13 @@ protected override void RegisterKeywords()
RegisterKeyword("xml");
}
+ protected override void RegisterFunctions()
+ {
+ base.RegisterFunctions();
+ RegisterFunction("extract", new SQLFunctionTemplate(NHibernateUtil.Int32, "datepart(?1, ?3)"));
+ RegisterFunction("bit_length", new SQLFunctionTemplate(NHibernateUtil.Int32, "datalength(?1) * 8"));
+ }
@oskarb
oskarb May 28, 2012 NHibernate member

There are some code in HqlFunctions.cs in the test suite regarding dialects that don't support certain standard functions that should be tweaked for this.

@oskarb
oskarb May 28, 2012 NHibernate member

Oh, and these are not really related to Sql Server 2012 are they? Please create a separate Jira issue for these, to make sure they show up in the release notes. Also, according to the docs datepart and datalength are both supported already in MSSQL 2000 so they should go in that dialect instead.

@csharper2010
csharper2010 May 28, 2012

Actually I don't need those functions. I've just added them when doing my first git experiments. They are in MsSql2005Dialect and maybe they would even work in earlier releases. I'll drop that change out of the file. It should be handled separately.

@hazzik
hazzik May 29, 2012 NHibernate member

I've pushed these functions to master already.

@hazzik
Member
hazzik commented May 29, 2012

@csharper2010 I've commited MsSql2012Dialect with sequences support, so probably you will want to rebase your commit on top of the master and force push or open new request.

Test which are failing with your implementation:

  1. NHibernate.Test.Linq.MethodCallTests.CanSelectComponentsIntoObjectArray
  2. NHibernate.Test.NHSpecificTest.NH2214.Fixture.PagedQueryWithDistinctAndOrderingByNonProjectedColumn

Also we are using tabs for indents.

To not open new pull request you could force push to sql2012 branch and this pull request will be overriden.

@csharper2010 csharper2010 Paging for MsSql2012 changed to the new recommended standard syntax
Implementation based on current master branch MSSQL2012 Dialect. Implemented new paging syntax with
select xxx from yyy order by zzz offset @p1 rows fetch first @p2 rows only
Existing paging unit tests work fine with it except one that is semantically unclear to me.
28a5c5f
@csharper2010

@hazzik
NHibernate.Test.Linq.MethodCallTests.CanSelectComponentsIntoObjectArray: is only red because the test assumes that rows are given in insertion order without any order by specified. There are three rows in the table and the test takes the first. So it is not guaranteed to be correct. The paging implementation in MsSql2012Dialect does sort by the first column if no order by is specified at all. One can argue if that's a good thing. Maybe the dialect should fall back to the top (n) syntax if there's no offset and no order by. But still the Test case should be changed.

NHibernate.Test.NHSpecificTest.NH2214.Fixture.PagedQueryWithDistinctAndOrderingByNonProjectedColumn: The test fails because not an NHibernateException but a GenericAdoException is thrown. MsSql2005DialectQueryPager does explicit check that the order by columns in a distinct query are part of the select list. The simple implementation in MsSql2012Dialect does not do this check beforehand but then the query fails for the same reason while executing. And I assume that other dialects also don't do this check explicitly so maybe it's better to change the test.

@oskarb and @hazzik
Maybe it would make sense to integrate the tokenizer of #63.
The simple implementation of the query paging in MsSql2012Dialect of that pull request looks promising. I'm not sure if it does the right thing if there is an order by in a subquery but not in the outermost select like in
SELECT * FROM A LEFT JOIN (SELECT top 7 * FROM B ORDER BY name) AS B on A.Name = B.Name

@ggeurts
ggeurts commented Jun 9, 2012

The query paging implementations in #63 are subquery (and comments) aware. It only operates on non-nested SQL SELECT statements and ignores anything that is nested in brackets.

@hazzik
Member
hazzik commented Sep 30, 2012

Merged #63

@hazzik hazzik closed this Sep 30, 2012
@hazzik hazzik added a commit that referenced this pull request Sep 30, 2012
@hazzik hazzik Add test for csharper2010's note at #117 a91fac6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment