History | Log In     View a printable version of the current page.  
Issue Details (XML | Word | Printable)

Key: CASTOR-891
Type: Bug Bug
Status: Closed Closed
Resolution: Fixed
Priority: Major Major
Assignee: Werner Guttmann
Reporter: Martin Fuchs
Votes: 0
Watchers: 0
Operations

If you were logged in you would be able to see more operations.
castor

activate LIMIT for MS SQL Server

Created: 28/Sep/04 06:03 AM   Updated: 19/Apr/05 10:52 AM
Component/s: JDO
Affects Version/s: 0.9.5.3
Fix Version/s: None

Time Tracking:
Not Specified

File Attachments: 1. Text File mssql-limit.patch (0.7 kb)
2. Text File patch.txt (11 kb)

Environment:
Operating System: All
Platform: PC

Bugzilla Id: 1752


 Description  « Hide
> Agreed. And let's move the HSQL and MSSQL parts of this to separate (new)
bugs.
> Let's not reopen bug 1221, please.

Too late. I decided to reopen it before your answer came in. And I think its
correct to place it there since it seems it never worked with the initial patch.

Well, here is the (trivial) MS SQL Server patch again:

I just saw there is missing a isLimitClauseSupported() function in
SQLServerQueryExpression.java, as it claims to be able to process LIMIT queries.
I can't test it, may be some one else has a MS SQL server running and can look
into this?



 All   Comments   Work Log   Change History      Sort Order: Ascending order - Click to sort in descending order
Martin Fuchs - 28/Sep/04 06:03 AM
Created an attachment (id=700)
Patch for SQLServerQueryExpression

Werner Guttmann - 28/Sep/04 12:31 PM
Martin, your patch above does not overload getStatement (boolean) as in all the
other QueryExpression instances .. . I'll have a look at your comments in bug
1732, and add the code myself.

Werner Guttmann - 28/Sep/04 12:35 PM
Please ignore my last comment, figured things out myself the hard way ... ;-(.

Martin Fuchs - 29/Sep/04 02:14 AM
Sorry, I thought you did know, the LIMIT code was already present in
SQLServerQueryExpression. It was just no more active since the introduction of
isLimitClauseSupported().

Martin Fuchs - 29/Sep/04 02:17 AM
Well, this problem had been catched far sooner. What's missing in this project
is running the test suite for ALL databases on a regular base. This could be
achieved by setting up a test server running an instance of any DB and
launching the tests using a batch job. But there's of course some work to be
done for this...

Werner Guttmann - 29/Sep/04 02:29 AM
No problem, Martin. As long as I can keep the overhead cycles to a minimum, I
am fine .. .

Let's see what Bruce has got to say to the 'test server' issue, though he's not
working for Intalio anymore, as far as I remember. But maybe he is still able
to activate some of his old contacts ...


Patrick van Kann - 29/Sep/04 02:49 AM
I'm happy to test against SQL Server. I'll check out a fresh copy of Castor,
apply the patch and run the test suite to see if it passes. I have all of this
set up in SQL Server already.

Werner Guttmann - 29/Sep/04 03:16 AM
Thanks, Patrick.

Martin Fuchs - 29/Sep/04 04:49 AM
Hi Patrick,

can you please also try to run text case TC62 for MS SQL using the following
patch, since no other test case uses the LIMIT keyword:

Index: src/tests/tests.xml
===================================================================
RCS file: /cvs/castor/castor/src/tests/tests.xml,v
retrieving revision 1.10
diff -u -p -d -r1.10 tests.xml
— a/src/tests/tests.xml 28 Sep 2004 10:17:29 -0000 1.10
+++ b/src/tests/tests.xml 29 Sep 2004 11:47:30 -0000
at at -292,6 +292,7 at at
<!-- jdo.ListTypes is a helper class for
writing castor jdo driver -->
<!-- <case class="jdo.ListTypes" /> -->
+ <case class="jdo.TestLimitClause" id="TC62" />
</category>

</harness>

Index: src/tests/jdo/TestLimitClause.java
===================================================================
RCS file: /cvs/castor/castor/src/tests/jdo/TestLimitClause.java,v
retrieving revision 1.3
diff -u -p -d -r1.3 TestLimitClause.java
— a/src/tests/jdo/TestLimitClause.java 27 Sep 2004 20:41:50 -0000
1.3
+++ b/src/tests/jdo/TestLimitClause.java 29 Sep 2004 11:49:25 -0000
at at -123,7 +123,9 at at public class TestLimitClause extends Cas

public void runTest() throws PersistenceException { testLimit(); +/* no support for OFFSET with MS SQL Server testLimitWithOffset(); +*/ testOffsetWithoutLimit(); }

Thanks.


Werner Guttmann - 29/Sep/04 05:10 AM
Actually, looking at this, I believe that TestLimit should be split into two
classes, TestLimit and TestLimitOffset in suich a way that

a) testLimit() should remain in TestLimit
b) testLimitWithOffset() and testOffsetWithoutLimit should be moved to
TestLimitOffset.

This way, we could selectively include one or the other CTF test case in
src/tests/tests.xml.


Martin Fuchs - 01/Oct/04 12:24 AM
So I agree. Splitting the test case would allow to test LIMIT for MS SQL
without getting the "OFFSET not supported" exception.

<comment>
Another way would be to call isOffsetClauseSupported() to see if this test
should be run, but this is not the way test cases should be written.
Configuring it in test.xml is cleaner.
</comment>


Werner Guttmann - 01/Oct/04 01:47 AM
Created an attachment (id=706)
Complete patch (for review)

Werner Guttmann - 01/Oct/04 01:49 AM
Above patch includes the changes to SQLServerQueryExpression as discussed, plus
the change to src/tests/jdo/TestLimitClause. This change introduces a new class
src/tests/jdo/TestLimitWithOffsetClause that extends TestLimitClause, and moves
all test methods related to testing LIMIT with OFFSET clauses to the former. In
addition, I've patched src/tests/tests.xml to use one or the other, wherever
appropriate.

Martin Fuchs - 01/Oct/04 02:50 AM
The patch looks good. Just waiting for Patrick's test results.

Patrick van Kann - 01/Oct/04 03:55 AM
ooops... am on it. Got swamped in client work!

Patrick van Kann - 01/Oct/04 06:14 AM
I can confirm that if you modify SQLServerQueryExpression and TestLimitClause to
remove the offset (i.e. only run the testLimit() method) this runs successfully
against SQL Server 2000 with the InetSoft Opta2000 driver. Unfortunately as a
consequence of the fact that I haven’t run the suite against MSSQL for ages
there are 2 other failures not related to this functionality. One of them
appears to be schema change related but another needs to be examined further.

Also, does anyone know of a Windows version of patch? I had to make all the
changes to the test cases and SQLServerQueryExpression manually Is this
something that Eclipse does? I probably need to switch...


Werner Guttmann - 01/Oct/04 06:31 AM
> Unfortunately as a consequence of the fact that I haven’t run the
> suite against MSSQL for ages there are 2 other failures not related
> to this functionality.
What version numbers. TC02 fails with any RDBMS, and we are not going to get ths
fixed any time soon .. .

Werner Guttmann - 01/Oct/04 06:31 AM
> Also, does anyone know of a Windows version of patch?
Not really, but there's a choice. A fully-fledged IDE (such as Eclipse is),
WinCVS, etc.

Patrick van Kann - 01/Oct/04 06:34 AM
Yes, TCO2 is one. Phew. I will double check the other schema-related problem and
file a patch in bugzilla if necessary. Looks like I need to finally take the
Eclipse plunge as I use TortoiseCVS and it doesn't seem to play nice with WinCVS.

Werner Guttmann - 01/Oct/04 06:44 AM
> Looks like I need to finally take the Eclipse plunge as I use TortoiseCVS
> and it doesn't seem to play nice with WinCVS.
Which is quite a surprise, as CVS is CVS, and diffs are diffs. Once Martin et
alias start supplying unified diffs only, things should not be painful at all.
What problems are you experiencing, iima ?

Martin Fuchs - 01/Oct/04 06:48 AM
Patrick, you may have a look at my WIN32 version of Linux tools I compiled some
time ago:
http://private.addcom.de/m.fuchs/ftp/xtools.rar
It includes a patch.exe you can use on Windows.

Patrick van Kann - 01/Oct/04 07:13 AM
Werner - WinCVS freezes on load after I installed TortoiseCVS, that's all... not
a patch file compat issue.
Martin - Cheers, will download your tools.

Werner Guttmann - 06/Oct/04 02:27 AM
I think that the CVS support within Eclipse is one of the best I've seen so far.
And they are still adding new features as we speak (see the release notes for
the 3.1M2 build, where they are introducing CVS commit sets for your
convenience). Patrick, I know that switching to a new IDE is always work, but
imho it pays off. And finally, did you eventually find some time to run the
tests on MS SQL server ?

Martin Fuchs - 06/Oct/04 02:37 AM
I'm always getting the error message "I HATE YOU" from the exolab CVS server
when trying to connect using Eclipse. May be this is because I am using the
anonymous CVS account?

Well, the CVS integration in Eclipse is really quite good. basic commands
(update, commit) work without problems. But there are still some issues. For
example last time I tried to create branches using the Eclipse plugin I wasn't
very successfull. Hope this will be fixed soon.


Werner Guttmann - 06/Oct/04 02:44 AM
> I'm always getting the error message "I HATE YOU" from the exolab CVS
> server when trying to connect using Eclipse.
Eclipse 3.0 did have some problems with this before the M9 build. Bear in mind
that Eclipse by default does not save your CVS password .. if you happen to have
one. But as you are using anonymous access, I wonder what else it could be.

Martin Fuchs - 06/Oct/04 02:56 AM
It's just the same with Eclipse versions 3.0.0 and 3.1.0. May be Bruce can give
me a password to verify if the anoncvs is really causing the problem.
Using cvs from the command line or using WinCVS just works without any problems.

Patrick van Kann - 06/Oct/04 04:05 AM
I made the changes indicated in the test manually and the new tests ran fine
against SQL Server, with the exception of TC02 as previously discussed. My main
resistance to Eclipse is the performance of my laptop

Martin, I tried xtools but seem to have a missing DLL (win32gnu.dll) - this
doesn't appear to be in the RAR. Am I being thick or do I need something else?


Martin Fuchs - 06/Oct/04 04:13 AM
Ooops - yes, this DLL is missing in the archive. I will send it to you in
private email and maybe update the archive.


Martin Fuchs - 06/Oct/04 02:47 PM
Gregory, thanks for the hint. It now works.
... and sorry for the off topic thread.

I think we can now commit the patch, and close this bug, since the test was
successfull.


Werner Guttmann - 08/Oct/04 02:03 AM
Committed as supplied above. In addition, I have moved all test methods
regarding the use of the OFFSET clause to a new TestLimitWithOffsetClause
(TC62a), and modified to use this new class for MS SQL server (as it does not
provide support for OFFSET clauses).

Martin Fuchs - 08/Oct/04 07:27 AM
Closed.