Derby
  1. Derby
  2. DERBY-4208

Parameters ? with OFFSET and/or FETCH

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.5.1.1
    • Fix Version/s: 10.6.1.0
    • Component/s: SQL
    • Labels:
      None
    • Issue & fix info:
      High Value Fix
    • Bug behavior facts:
      Performance, Security

      Description

      The new OFFSET/FETCH syntax does not support Parameters.

      From ij against the toursdb I would like to do the following:

      ij> prepare foo as 'select * from cities offset ? rows fetch first 10 rows only';

      but results in the following syntax error:

      ERROR 42X01: Syntax error: Encountered "?" at line 1, column 29.

      Parameterized OFFSET/FETCH is important for performance (can be prepared) and security (SQL strings not created on the fly).

      1. derby4208-docs-b.diff
        2 kB
        Dag H. Wanvik
      2. derby4208-docs-a.diff
        2 kB
        Dag H. Wanvik
      3. derby4208c.stat
        0.7 kB
        Dag H. Wanvik
      4. derby4208c.diff
        25 kB
        Dag H. Wanvik
      5. derby4208b.stat
        0.7 kB
        Dag H. Wanvik
      6. derby4208b.diff
        26 kB
        Dag H. Wanvik
      7. derby4208a.stat
        0.7 kB
        Dag H. Wanvik
      8. derby4208a.diff
        24 kB
        Dag H. Wanvik

        Issue Links

          Activity

          Hide
          Dag H. Wanvik added a comment -

          Right, since this feature is not included by the SQL standard (offset
          is a <simple value specification>, not a <general value specification>
          in the syntax), we didn't implement it in the first version, but I
          definitely see the utility of this extension. Feel free to vote for
          this issue is you want it, folks.

          Show
          Dag H. Wanvik added a comment - Right, since this feature is not included by the SQL standard (offset is a <simple value specification>, not a <general value specification> in the syntax), we didn't implement it in the first version, but I definitely see the utility of this extension. Feel free to vote for this issue is you want it, folks.
          Hide
          Dag H. Wanvik added a comment -

          Here is a first cut at a patch for this functionality. What do people think,
          should let Derby have this functionality in spite of the standard not catering for it?

          The code is currently a little clumsy since it more or less lets the current code
          in peace, and just extends with treatment for dynamic parameters. Not for commit yet,
          I'd just like some feedback on the desirability and general approach for now.

          I added some tests for it as well.

          Show
          Dag H. Wanvik added a comment - Here is a first cut at a patch for this functionality. What do people think, should let Derby have this functionality in spite of the standard not catering for it? The code is currently a little clumsy since it more or less lets the current code in peace, and just extends with treatment for dynamic parameters. Not for commit yet, I'd just like some feedback on the desirability and general approach for now. I added some tests for it as well.
          Hide
          Dag H. Wanvik added a comment -

          Uploading a fairly complete version of this patch, I did some simplifications, removed cruft
          and added NULL handling (supplying a NULL argument will give an error now),
          and added more tests.

          A note: ParameterNode.setType always make the type nullable, no matter if the passed in DVD is not nullable, not sure why. This makes the parameter metadata for ? always nullable, which doesn't seem right here.. Since dynamic arguments in OFFSET/FETCH is outside the standard, we don't have any guidance here...

          Show
          Dag H. Wanvik added a comment - Uploading a fairly complete version of this patch, I did some simplifications, removed cruft and added NULL handling (supplying a NULL argument will give an error now), and added more tests. A note: ParameterNode.setType always make the type nullable, no matter if the passed in DVD is not nullable, not sure why. This makes the parameter metadata for ? always nullable, which doesn't seem right here.. Since dynamic arguments in OFFSET/FETCH is outside the standard, we don't have any guidance here...
          Hide
          Rick Hillegas added a comment -

          Hi Dag,

          Thanks for tackling this important usability issue. I agree that the handling of dynamic parameters looks a little complicated. We use dynamic parameters all across the language, practically anywhere that a ValueNode can appear. It's hard to believe that special generated methods are added to the factory signatures of the ResultSets for all of these instances. I would recommend tracking down how we handle ? in a statement like "select * from t where a = ?". Thanks.

          Show
          Rick Hillegas added a comment - Hi Dag, Thanks for tackling this important usability issue. I agree that the handling of dynamic parameters looks a little complicated. We use dynamic parameters all across the language, practically anywhere that a ValueNode can appear. It's hard to believe that special generated methods are added to the factory signatures of the ResultSets for all of these instances. I would recommend tracking down how we handle ? in a statement like "select * from t where a = ?". Thanks.
          Hide
          Dag H. Wanvik added a comment -

          Thanks for looking at the patch, Rick, I am still learning how to best
          go about the code generation in non-trivial cases..

          > I would recommend tracking down how we handle ? in a statement like
          > "select * from t where a = ?".

          Actually, thats how I found the pattern to make to it work at all; I
          did look at the restrict generated code, cf. here:

          ProjectRestrictNode.generateMinion

          which does pass in specially generated methods to the result set, and
          also see the usage of these generated methods:

          ProjectRestrictResultSet.openCore: constantRestriction.invoke and
          ProjectRestrictResultSet.getNextRowCore: restriction.invoke

          so I don't yet understand how this particular usage of ? would lead me to a
          simplification.

          I could probaby mimic the constantRestriction handling for the
          constant, but I'm can't yet see how that would simplify things, if
          anything that's an overkill, because in our case, we only allow
          literals for the constant case.

          I'll read some more code..

          Show
          Dag H. Wanvik added a comment - Thanks for looking at the patch, Rick, I am still learning how to best go about the code generation in non-trivial cases.. > I would recommend tracking down how we handle ? in a statement like > "select * from t where a = ?". Actually, thats how I found the pattern to make to it work at all; I did look at the restrict generated code, cf. here: ProjectRestrictNode.generateMinion which does pass in specially generated methods to the result set, and also see the usage of these generated methods: ProjectRestrictResultSet.openCore: constantRestriction.invoke and ProjectRestrictResultSet.getNextRowCore: restriction.invoke so I don't yet understand how this particular usage of ? would lead me to a simplification. I could probaby mimic the constantRestriction handling for the constant, but I'm can't yet see how that would simplify things, if anything that's an overkill, because in our case, we only allow literals for the constant case. I'll read some more code..
          Hide
          Dag H. Wanvik added a comment -

          Uploading a slightly simplified version, which removes the special casing of
          literals (code is generated uniformly when a row count is provided). This makes for less clutter
          (e.g. two fewer args to the result set class/factory methods, only one range check instance).

          Show
          Dag H. Wanvik added a comment - Uploading a slightly simplified version, which removes the special casing of literals (code is generated uniformly when a row count is provided). This makes for less clutter (e.g. two fewer args to the result set class/factory methods, only one range check instance).
          Hide
          Dag H. Wanvik added a comment -

          I will commit this shortly, if there are no further review comments.

          Show
          Dag H. Wanvik added a comment - I will commit this shortly, if there are no further review comments.
          Hide
          Dag H. Wanvik added a comment -

          Uploading a documentation patch for this change; derby-4208-docs-a.
          Looked OK in the HTML pages view and in the .pdf.

          Show
          Dag H. Wanvik added a comment - Uploading a documentation patch for this change; derby-4208-docs-a. Looked OK in the HTML pages view and in the .pdf.
          Hide
          Dag H. Wanvik added a comment -

          Committed patch derby4208c as svn 807337 on trunk.
          I will let it remain unresolved until the docs patch has been committed.

          Show
          Dag H. Wanvik added a comment - Committed patch derby4208c as svn 807337 on trunk. I will let it remain unresolved until the docs patch has been committed.
          Hide
          Kim Haase added a comment -

          The doc looks fine, Dag, thanks! One line of code is long and runs over a line in the PDF, making a line break that wouldn't work in real code:

          PreparedStatement p = con.prepareStatement("SELECT * FROM T ORDER BY I OFFSET ? ROWS");

          You might fix it like this:

          PreparedStatement p =
          con.prepareStatement("SELECT * FROM T ORDER BY I OFFSET ? ROWS");

          This is optional if you don't want to bother – people can probably figure it out.

          Show
          Kim Haase added a comment - The doc looks fine, Dag, thanks! One line of code is long and runs over a line in the PDF, making a line break that wouldn't work in real code: PreparedStatement p = con.prepareStatement("SELECT * FROM T ORDER BY I OFFSET ? ROWS"); You might fix it like this: PreparedStatement p = con.prepareStatement("SELECT * FROM T ORDER BY I OFFSET ? ROWS"); This is optional if you don't want to bother – people can probably figure it out.
          Hide
          Dag H. Wanvik added a comment -

          Thanks for looking at the doc patch, Kim!
          Uploading version "b" to fix the long time, committed as svn 807458.

          Show
          Dag H. Wanvik added a comment - Thanks for looking at the doc patch, Kim! Uploading version "b" to fix the long time, committed as svn 807458.
          Hide
          Dag H. Wanvik added a comment -

          Resolving.

          Show
          Dag H. Wanvik added a comment - Resolving.
          Hide
          Dag H. Wanvik added a comment -

          Removing release note needed flag: this is a new feature which has been documented and it should not break existing applications.

          Show
          Dag H. Wanvik added a comment - Removing release note needed flag: this is a new feature which has been documented and it should not break existing applications.

            People

            • Assignee:
              Dag H. Wanvik
              Reporter:
              Steve Radman
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development