NH-3901, NH-3846 - Correct IndexOfGenerator index offsets

[PleasantD]

This translates the IndexOf (locate) result back into 0-based index space (NH-3901) and translates the startAt position into 1-based index space (NH-3846).

[hazzik]

Tests are failing. I think the correct collection assertion is 'Is.EquivalentTo'

[PleasantD]

That is odd. All of the tests pass locally. And I don't want EquivalentTo(), because that doesn't validate order, which should be the same for both collections.

[PleasantD]

I found (and fixed) a subtler bug with the way that postgres (and SybaseASE15) calculate the position when a starting at an offset.

But I still can't get the tests to pass for test methods that take a character.
The tests are failing on the default build, which is short-circuiting the tests of the other DB types.
What DB type is used by default?

As a note, these tests used to never actually execute anything to the DB or assert anything. So maybe they never were working.

[hazzik]

By default it's Microsoft Sql Server.
The tests are failing because types of asserted collections do not match.
Is.Equivalent shall check the order.

[PleasantD]

No, check the tests. They are failing because the results of the query are different.
And EquivalentTo will check that collections are equivalent, same items in any order
http://www.nunit.org/index.php?p=collectionConstraints&r=2.6.1

[hazzik]

Yes, sorry. I rechecked the documentation and you are right. Is equivalent
checks same amount of elements in any order.

[hazzik]

You can run the tests same way as team city by using ShowBuildMenu.bat
(option I. - TeamCity build menu).

[PleasantD]

Yes, and I have now.
The difference seems to be that when running the character version the match parameter is type StringFixedLength (4000) while the string version is String (4000)

[PleasantD]

Parameter types are inferred for any constants with non-null values. The problem is that (for MSSQL, and maybe others) the inference is a fixed length string. Which is, as far as I can tell, never correct. So I changed the ExpressionParameterVisitor to generate the NamedParameter with a string value instead and everything seems much better.

[hazzik]

I think you need to change HqlGenerator instead of how the function renders.

[PleasantD]

I needed to fix both. There are a few different bugs here because the tests never actually checked for expected results.

The changes to IndexOfGenerator fix the offset issue. SQL "locate" uses 1-based indexing with 0 as the return value for NOT FOUND, so we need to add one to the startAt index and subtract 1 from the result to get the expected C# results.

The changes to PositionSubstringFunction and CharIndexFunction fix a different (and subtler, and undocumented(!)) bug that appears when the dialect doesn't directly support "locate" with a startAt value. The dialect supports it by searching for the IndexOf in a substring and then subtracting the startAt offset. However, as it was written it did not take into account a result of 0 (NOT FOUND) in the substring. If the IndexOf in the substring returns 0 then the entire block should return 0. This is then wrapped by the subtract 1 from above to return -1 to C#. I don't see how I could do this inside of IndexOfGenerator, as this applies only to dialects that don't support a offset for "locate".

Finally, the changes to ExpressionParameterVisitor fix a problem with the char versions of IndexOf where the parameter is being sent as a fixed length string instead of a varchar. If the parameter is sent/interpreted incorrectly (SQL Server, I think) then we wind up trying to locate a in alberta. The changes here just ensure all char parameters are sent as string, which is always interpreted as a varchar, as far as I can tell.

[hazzik]

But in HQL you do not need to extract -1, as it shall have the SQL meaning
of return value, not C#'s
On Sat, 5 Nov 2016 at 9:37 AM, Duncan Munro notifications@github.com
wrote:

I needed to fix both. There are a few different bugs here because the
tests never actually checked for expected results.

The changes to IndexOfGenerator fix the offset issue. SQL "locate" uses
1-based indexing with 0 as the return value for NOT FOUND, so we need to
add one to the startAt index and subtract 1 from the result to get the
expected C# results.

The changes to PositionSubstringFunction and CharIndexFunction fix a
different (and subtler) bug that appears when the dialect doesn't directly
support "locate" with a startAt value. The dialect supports it by
searching for the IndexOf in a substring and then subtracting the startAt
offset. However, as it was written it did not take into account a result of
0 (NOT FOUND) in the substring. If the IndexOf in the substring returns 0
then the entire block should return 0. This is then wrapped by the subtract
1 from above to return -1 to C#. I don't see how I could do this inside of
IndexOfGenerator, as this applies only to dialects that don't support a
offset for "locate".

Finally, the changes to ExpressionParameterVisitor fix a problem with the
char versions of IndexOf where the parameter is being sent as a fixed
length string instead of a varchar. If the parameter is sent/interpreted
incorrectly (SQL Server, I think) then we wind up trying to locate a in
alberta. The changes here just ensure all char parameters are sent as
string, which is always interpreted as a varchar, as far as I can tell.

—
You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#505 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAI1lwViwIoCBdfZfctrFaKcB0QL-m92ks5q65eKgaJpZM4JraSM
.

[PleasantD]

I would argue (and @patearl agrees) that translating in the SQL is better.

Here's a trivial example:
I store some known characters and expected offsets in a table -> ['-', 3]. These are generated in C# with 0-index offsets. Then I store some strings in another table. ["TEST-1", "PASS-2", "BAD-3"].

Now I want to generate a query where the characters and offsets from the first table are used to parse and return values from the second table. Since the offsets are 0-based (as is all my data, coming from C#), I shouldn't need to know that the query is going to be executed in SQL and have to add any adjustment. Therefore the SQL generated should include the -1 offset to correct to C# space.

[PleasantD]

Actually... now that I'm using my brain, that was an overly complicated example.
The simple example is:

session.Query<Entity>().Where(e => e.Name.IndexOf("A") > -1);

Since the entire expression will be translated to HQL (and then to SQL), the -1 offset needs to be applied in HQL otherwise operation will be incorrect.

Or am I misunderstanding something?

[hazzik]

I agree that -1 shall be generated, but I think it shall be done on before hql. So, it shall generate the HQL tree similar to the following hql query:

"select e from Entity where hql_indexOf(e.Name, 'A') - 1 > -1"

Where hql_indexOf is the function which will be translated to just CHARINDEX in MS SQL.

What I'm trying to say here:

1. Linq shall generate the correct query result
2. HQL shall be close to SQL as much as possible, as it is considered a subset of SQL.

[hazzik]

Ok, I understood your changes now. Good to go.

[oskarb]

Please rebase on master. Various long-failing tests have been fixed.

[PleasantD]

Rebase done

[PleasantD]

Will this be included in 4.1.0?

[oskarb]

I wonder if this can be experienced as a breaking change to existing code that have been tested with current behaviour.

[PleasantD]

It could be. People would have written expressions like query.Where(x => x.Name.IndexOf('A') > 0) and gotten incorrect results and would've had to modify them to be query.Where(x => x.Name.IndexOf('a') > 1).

However, if they were doing this, you'd think that this would have been noticed earlier. My issue is that the expressions are now not equivalent, so to do this kind of in-place patch I'd need to swap expressions depending on whether the execution NH or memory.

[oskarb]

Thanks for this fix!