Bugzilla Version 2.16.7
Bugzilla Bug 1752
  activate LIMIT for MS SQL Server
     Query page      Enter new bug
Bug#: 1752   Platform:   Reporter: martin-fuchs at gmx dot net (Martin Fuchs)
Product:   OS:   Add CC:
Component:   Version:   CC:
Remove selected CCs
Status: CLOSED   Priority:  
Resolution: FIXED   Severity:  
Assigned To: werner dot guttmann at gmx dot net (Werner Guttmann)   Target Milestone:  
URL:
Summary:

Attachment Type Modified Status Actions
Patch for SQLServerQueryExpression patch 2004-09-28 06:03 none Edit
Complete patch (for review) patch 2004-10-01 01:47 none Edit
Create a New Attachment (proposed patch, testcase, etc.) View All

Bug 1752 depends on: Show dependency tree
Show dependency graph
Bug 1752 blocks:
Votes: 0    Show votes for this bug    Vote for this bug

Additional Comments:


Leave as CLOSED FIXED
Reopen bug

View Bug Activity   |   Format For Printing

Description: Opened: 2004-09-28 06:03

> 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?

------- Additional Comment #1 From Martin Fuchs 2004-09-28 06:03 -------
Created an attachment (id=700)
Patch for SQLServerQueryExpression

------- Additional Comment #2 From Werner Guttmann 2004-09-28 12:31 -------
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.

------- Additional Comment #3 From Werner Guttmann 2004-09-28 12:35 -------
Please ignore my last comment, figured things out myself the hard way ... ;-(.

------- Additional Comment #4 From Martin Fuchs 2004-09-29 02:14 -------
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().

------- Additional Comment #5 From Martin Fuchs 2004-09-29 02:17 -------
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...  ;-)

------- Additional Comment #6 From Werner Guttmann 2004-09-29 02:29 -------
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 ...

------- Additional Comment #7 From Patrick van Kann 2004-09-29 02:49 -------
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.

------- Additional Comment #8 From Werner Guttmann 2004-09-29 03:16 -------
Thanks, Patrick.

------- Additional Comment #9 From Martin Fuchs 2004-09-29 04:49 -------
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.

------- Additional Comment #10 From Werner Guttmann 2004-09-29 05:10 -------
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.

------- Additional Comment #11 From Martin Fuchs 2004-10-01 00:24 -------
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>

------- Additional Comment #12 From Werner Guttmann 2004-10-01 01:47 -------
Created an attachment (id=706)
Complete patch (for review)

------- Additional Comment #13 From Werner Guttmann 2004-10-01 01:49 -------
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.

------- Additional Comment #14 From Martin Fuchs 2004-10-01 02:50 -------
The patch looks good. Just waiting for Patrick's test results.  :)

------- Additional Comment #15 From Patrick van Kann 2004-10-01 03:55 -------
ooops... am on it. Got swamped in client work!

------- Additional Comment #16 From Patrick van Kann 2004-10-01 06:14 -------
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... 



------- Additional Comment #17 From Werner Guttmann 2004-10-01 06:31 -------
> 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 .. :-(.

------- Additional Comment #18 From Werner Guttmann 2004-10-01 06:31 -------
> 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.

------- Additional Comment #19 From Patrick van Kann 2004-10-01 06:34 -------
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.

------- Additional Comment #20 From Werner Guttmann 2004-10-01 06:44 -------
> 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 ?

------- Additional Comment #21 From Martin Fuchs 2004-10-01 06:48 -------
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.

------- Additional Comment #22 From Patrick van Kann 2004-10-01 07:13 -------
Werner - WinCVS freezes on load after I installed TortoiseCVS, that's all... not
a patch file compat issue.
Martin - Cheers, will download your tools. 

------- Additional Comment #23 From Werner Guttmann 2004-10-06 02:27 -------
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 ?

------- Additional Comment #24 From Martin Fuchs 2004-10-06 02:37 -------
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.

------- Additional Comment #25 From Werner Guttmann 2004-10-06 02:44 -------
> 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.

------- Additional Comment #26 From Martin Fuchs 2004-10-06 02:56 -------
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.

------- Additional Comment #27 From Patrick van Kann 2004-10-06 04:05 -------
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?

------- Additional Comment #28 From Martin Fuchs 2004-10-06 04:13 -------
Ooops - yes, this DLL is missing in the archive. I will send it to you in 
private email and maybe update the archive.

------- Additional Comment #29 From Gregory Block 2004-10-06 14:32 -------
(sidenote - "I HATE YOU", in my case, is coming from an odd bug in eclipse that still seems to be 
around in M2; all I had to do was re-enter the password into Eclipse, and she's happy.

------- Additional Comment #30 From Martin Fuchs 2004-10-06 14:47 -------
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.

------- Additional Comment #31 From Werner Guttmann 2004-10-08 02:03 -------
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).

------- Additional Comment #32 From Martin Fuchs 2004-10-08 07:27 -------
Closed.

     Query page      Enter new bug
This is Bugzilla: the Mozilla bug system. For more information about what Bugzilla is and what it can do, see bugzilla.org.
Actions: New | Query | bug # | Reports   New Account | Log In