Add SQL Anywhere 17 support #1854

pettersoft · 2018-09-20T14:51:09Z

Added a SQL Anywhere 17 driver and SQL Anywhere 17 dialect.

fredericDelaporte

Thanks for your contribution.

I have not yet checked all the tests with my local database instance, but so far this looks overall good. (At least I have used the same driver for my own tests some time ago, see 6aef609.)

fredericDelaporte · 2018-09-21T11:27:53Z

src/NHibernate/Dialect/SybaseSQLAnywhere17Dialect.cs

+	///	</item>
+	/// </list>
+	/// </remarks>
+	public class SybaseSQLAnywhere17Dialect : SybaseSQLAnywhere12Dialect


It seems to me the name Sybase is phased out. Shouldn't it be named SapSQLAnywhere17Dialect?

Yes, that's correct! It should be renamed to Sap instead.

fredericDelaporte · 2018-09-21T11:33:10Z

src/NHibernate/Dialect/SybaseSQLAnywhere17Dialect.cs

+			RegisterKeywords();
+		}
+
+		new protected void RegisterKeywords()


SybaseSQLAnywhere12Dialect implementation is not what we do usually. Maybe that is the right time to fix it: switch these methods (RegisterKeywords and RegisterDateTimeTypeMappings) to override, calling their base implementation, and remove them from constructor call, both in SybaseSQLAnywhere12Dialect and here. SybaseSQLAnywhere10Dialect own constructor will call them, once switched to overrides.

fredericDelaporte · 2018-09-21T11:35:48Z

src/NHibernate/Driver/SybaseSQLAnywhere17Driver.cs

@@ -0,0 +1,19 @@
+namespace NHibernate.Driver
+{
+	public class SybaseSQLAnywhere17Driver : ReflectionBasedDriver


As for the dialect, it should likely be renamed SapSQLAnywhere17Driver.

fredericDelaporte · 2018-09-21T15:06:44Z

src/NHibernate/Dialect/SapSQLAnywhere17Dialect.cs

-			RegisterKeyword("limit");
-			RegisterKeyword("offset");
-			RegisterKeyword("datetimeoffset");
+			base.RegisterKeywords();


So, indeed these keywords were already registered in the base dialect. Better remove completely the override then.

fredericDelaporte · 2018-09-21T15:07:10Z

src/NHibernate/Dialect/SapSQLAnywhere17Dialect.cs

 		{
-			RegisterColumnType(DbType.DateTimeOffset, "DATETIMEOFFSET");
+			base.RegisterDateTimeTypeMappings();


fredericDelaporte · 2018-09-21T15:27:45Z

Numerous tests are failings, and HqlIsThreadSafe_UsingThreads seems to freeze. Granted, previous Anywhere dialects were likely not fixed for all tests either. But now we rather fix tests or at least ignore those due to known limitations of the target database. (This was done for latest dialect additions, see #1662 by example.)

I may directly push here some commits for fixing these tests failures, but of course feel free to fix some of them too.

fredericDelaporte

Here is a bunch of fixes. Not everything is fixed yet.

I have notably issues with queries selecting a string concatenation or a case as a column result. They work fine in Interactive SQL, but for some reasons, the reader (SADataReader) try to convert such column to numeric (string concatenation case) or integer (case when case), which fails. (Unless the result was by chance convertible to a number.)
I have posted here about this behavior, not finding any information.

fredericDelaporte · 2018-09-25T16:17:39Z

src/NHibernate.Test/Ado/GenericBatchingBatcherFixture.cs

@@ -29,7 +29,8 @@ protected override bool AppliesTo(Dialect.Dialect dialect)
 			return !(dialect is FirebirdDialect) &&
 			       !(dialect is Oracle8iDialect) &&
 			       !(dialect is MsSqlCeDialect) &&
-			       !(dialect is HanaDialectBase);
+			       !(dialect is HanaDialectBase) &&
+			       !(dialect is SybaseSQLAnywhere10Dialect);


This batcher is not supported by SQL Anywhere. It seems it does not supports to have multiple inserts in the same query, as described here.

fredericDelaporte · 2018-09-25T16:19:19Z

src/NHibernate.Test/Component/Basic/ComponentWithUniqueConstraintTests.cs

-				comp.Property(p => p.Name);
-				comp.Property(p => p.Dob);
+				comp.Property(p => p.Name, m => m.NotNullable(true));
+				comp.Property(p => p.Dob, m => m.NotNullable(true));
 				comp.Unique(true); // hbm2ddl: Generate a unique constraint in the database


This test does not need those columns to be nullable, so for avoiding troubles with database not supporting null in unique constraint, setting the columns as not nullable.

fredericDelaporte · 2018-09-25T16:24:38Z

src/NHibernate/Dialect/SybaseSQLAnywhere10Dialect.cs

+			// SQL Anywhere does not respect usual priorities with the bitwise not. Unfortunately the HQL parser
+			// furthermore remove "undue" parenthesis according to usual rules. As the bitwise not should have maximal
+			// priority, we can work around this by using a template forcing parenthesis around it.
+			RegisterFunction("bnot", new VarArgsSQLFunction(NHibernateUtil.Int64, "(~", "", ")"));


This is not a breaking change, because the function argument will by default be all that follows the ~. But thanks to this change, if parenthesis are used to explicit the priority, they will be taken into account for supplying the "function" argument, instead of being silently removed by the parser.

fredericDelaporte · 2018-09-25T16:26:49Z

src/NHibernate/Dialect/SybaseSQLAnywhere10Dialect.cs

+				if (limit == null && offset == null)
+					throw new ArgumentException("Cannot limit with neither a limit nor an offset");
+				if (limit == null)
+					throw new NotSupportedException($"Dialect {this} does not support setting an offset without a limit");


This case was previously blowing with a null reference exception. Overriden in the 17 version since SQL Anywhere 12.0.1 (but not 12.0.0) has a syntax allowing the support of this case.

fredericDelaporte · 2018-09-25T16:28:30Z

src/NHibernate/SqlCommand/SqlStringBuilder.cs

 		public SqlStringBuilder Add(SqlString sqlString)
 		{
-			sqlString.Visit(AddingVisitor);
+			sqlString?.Visit(AddingVisitor);


Additional change for avoiding blowing with a null reference exception when the sqlString is null. The usual behavior for string builder being to do nothing on null, doing the same here.

hazzik · 2018-09-26T03:04:28Z

@fredericDelaporte can we setup an automatic build for it?

fredericDelaporte · 2018-09-26T11:00:49Z

On the TeamCity agent, it should be doable. On AppVeyor or Travis, I do no think there is a reasonably lightweight way to set it up on each build.

Update: it is maybe not possible for TeamCity too: there is no NuGet package as far as I know for the data provider, and I have not checked yet if its license could allow us to put it in NHibernate lib folder in Git.

fredericDelaporte · 2018-09-27T11:08:53Z

Update: it is maybe not possible for TeamCity too: there is no NuGet package as far as I know for the data provider, and I have not checked yet if its license could allow us to put it in NHibernate lib folder in Git.

There is a Nuget package, but it is very likely to be unofficial. Looking at the license files available on my local setup, no redistribution is allowed for all components, including client ones. So I do not think we can legally put the data provider in the NHibernate lib folder. (The license allows a single installation for the developer and 25 internal deployments for testing purpose.)

hazzik · 2018-09-27T11:41:00Z

Have you considered installing provider to GAC? We can put a readme file to /lib/teamcity/sqlAnywhere. Explaining how to install the drivers. If it goes with a DbProviderFactory we should be able to instantiate the provider from GAC using the provider name.

fredericDelaporte · 2018-09-27T13:09:28Z

It does, and it is installed in GAC by the SQL Anywhere installer. Nothing to tell in lib/teamcity indeed.

fredericDelaporte · 2018-09-27T20:24:26Z

Now remain failures due to #1855 and #1859 bugs, and failures due to what seems to be a quite poor type inference done by the data-provider on projected columns, causing it to treat columns resulting from a case when as being integers, columns resulting from a string concatenation using the + operator as being decimal, and columns resulting from arithmetic as being decimal(30,6), causing truncation when there was more than 6 fractional digits.
I consider adding a TestDialect property like HasUnreliableTypeInference.

fredericDelaporte · 2018-09-28T16:03:52Z

It appears a hack similar to the one done on Firebird parameters in its driver could solve the bad type inference done by the data-provider on projected columns. This trouble seems to occurs only when the projection uses parameter values. And adding a cast around the parameter to explicit its type in the SQL solves the trouble. (At least in the tests here.)

But I do not think it is worth it. Such a hack implies parsing with reg-ex each generated query, and tweaking it with "word replacements". It is brittle and may breaks some query cases not covered by NHibernate tests.

fredericDelaporte · 2018-09-30T15:46:19Z

I have setup a SAP SQL Anywhere TeamCity build. Its first almost successful build can be seen in 609f033 checks.

22 tests have failed. With c3597a3, only 14 failing tests should remain. Their fixes are in #1855 and #1859. Either I change them for being ignored, or this PR has to wait for these other PR merges.

The Anywhere build is paused, and I am launching it manually. Enabling it would likely cause all other branches builds to fail, unless they get updated with 0545b45 changes.

fredericDelaporte · 2018-10-04T18:14:05Z

Rebased for resolving conflicts with latest merges, and test suite fixes squashed together.

hazzik

LGTM

fredericDelaporte · 2018-10-31T11:01:07Z

It does still depend on #1855 and #1859. I should have switched it to WIP.

fredericDelaporte · 2018-11-07T13:20:59Z

Rebased, Anywhere TeamCity build queued.

fredericDelaporte · 2018-11-08T12:38:48Z

Another fix is required, still due to the lack of support of null values in an unique constraint, see #1900.

This time, I have also locally applied this fix to the AnyWhere branch, and no tests are failing any-more for SQL Anywhere, with #1900 cherry-picked.

And add the Teamcity build

fredericDelaporte

Now, the SQL Anywhere build succeeds.

fredericDelaporte · 2018-11-15T13:50:54Z

About enabling the build on TeamCity, we should likely wait for having active PR updated first.

fredericDelaporte · 2018-12-07T15:59:50Z

@hazzik, do you agree for enabling the SAP SQL Anywhere build for all builds?

I think it is time to have all PR targeting master at least up-to-date with 5.2.

It will cause a failure for PR targeting older branches though. I do not see how to avoid that. (Excepted back-porting this PR here to the older branches, which I would rather avoid.)

fredericDelaporte added t: New Feature c: DataProviders / Dialects p: Minor labels Sep 21, 2018

fredericDelaporte reviewed Sep 21, 2018

View reviewed changes

This was referenced Sep 22, 2018

Fix NotNullUnique not taken into account for single column #1855

Merged

Fix Guid.ToString() #1856

Merged

fredericDelaporte reviewed Sep 25, 2018

View reviewed changes

fredericDelaporte force-pushed the master branch from 6b9b1c3 to fca0feb Compare October 4, 2018 18:13

hazzik previously approved these changes Oct 30, 2018

View reviewed changes

fredericDelaporte changed the title ~~Added SQL Anywhere 17 support~~ WIP - Added SQL Anywhere 17 support Oct 31, 2018

fredericDelaporte dismissed hazzik’s stale review via 683d974 November 7, 2018 13:12

fredericDelaporte force-pushed the master branch 2 times, most recently from 683d974 to 3eb4307 Compare November 7, 2018 13:18

fredericDelaporte changed the title ~~WIP - Added SQL Anywhere 17 support~~ Added SQL Anywhere 17 support Nov 7, 2018

fredericDelaporte changed the title ~~Added SQL Anywhere 17 support~~ Add SQL Anywhere 17 support Nov 7, 2018

fredericDelaporte mentioned this pull request Nov 8, 2018

Do not generate FK on non-generated unique constraint #1900

Merged

fredericDelaporte changed the title ~~Add SQL Anywhere 17 support~~ WIP - Add SQL Anywhere 17 support Nov 11, 2018

Petter and others added 2 commits November 14, 2018 13:23

Added SQL Anywhere 17 support

4110d2e

Fix failing tests for SQL Anywhere

06da1c9

And add the Teamcity build

fredericDelaporte force-pushed the master branch from 3eb4307 to 06da1c9 Compare November 14, 2018 12:23

fredericDelaporte changed the title ~~WIP - Add SQL Anywhere 17 support~~ Add SQL Anywhere 17 support Nov 14, 2018

fredericDelaporte added this to the 5.2 milestone Nov 14, 2018

fredericDelaporte approved these changes Nov 14, 2018

View reviewed changes

fredericDelaporte merged commit 018cf1c into nhibernate:master Nov 15, 2018

fredericDelaporte added the r: Fixed label Nov 15, 2018

Add SQL Anywhere 17 support #1854

Add SQL Anywhere 17 support #1854

Uh oh!

Conversation

pettersoft commented Sep 20, 2018

Uh oh!

fredericDelaporte left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte commented Sep 21, 2018

Uh oh!

fredericDelaporte left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hazzik commented Sep 26, 2018

Uh oh!

fredericDelaporte commented Sep 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fredericDelaporte commented Sep 27, 2018

Uh oh!

hazzik commented Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fredericDelaporte commented Sep 27, 2018

Uh oh!

fredericDelaporte commented Sep 27, 2018

Uh oh!

fredericDelaporte commented Sep 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fredericDelaporte commented Sep 30, 2018

Uh oh!

fredericDelaporte commented Oct 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hazzik left a comment

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte commented Oct 31, 2018

Uh oh!

fredericDelaporte commented Nov 7, 2018

Uh oh!

fredericDelaporte commented Nov 8, 2018

Uh oh!

fredericDelaporte left a comment

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte commented Nov 15, 2018

Uh oh!

fredericDelaporte commented Dec 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

fredericDelaporte left a comment •

edited

Loading

fredericDelaporte commented Sep 26, 2018 •

edited

Loading

hazzik commented Sep 27, 2018 •

edited

Loading

fredericDelaporte commented Sep 28, 2018 •

edited

Loading

fredericDelaporte commented Oct 4, 2018 •

edited

Loading

fredericDelaporte commented Dec 7, 2018 •

edited

Loading