Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 10.4.1.3
    • Component/s: SQL
    • Labels:
      None

      Description

      As part of implementing the overall OLAP Operations features of SQL (DERBY-581), implement the ROW_NUMBER() window function.

      More information about this feature is available at http://wiki.apache.org/db-derby/OLAPRowNumber

      DERBY-2998 implements

      • framework code for windows and window function result columns.
      • the ROW_NUMBER() window function according to SQL2003
        but the implementation is limited by
      • only supporting an empty, unnamed, inline window definition (i.e the full, unordered resultset)
      • not being fully optimized
      1. d2998-followup-removeunused.diff
        0.9 kB
        Thomas Nielsen
      2. d2998-followup-distinct.diff
        5 kB
        Thomas Nielsen
      3. rreffuncrownumber.html
        4 kB
        Thomas Nielsen
      4. d2998-followup-doc3.stat
        0.0 kB
        Thomas Nielsen
      5. d2998-followup-doc3.diff
        1 kB
        Thomas Nielsen
      6. d2998-followup-distinct.stat
        0.2 kB
        Thomas Nielsen
      7. d2998-followup-distinct.diff
        5 kB
        Thomas Nielsen
      8. d2998-followup-issue4.stat
        0.1 kB
        Thomas Nielsen
      9. d2998-followup-issue4.diff
        3 kB
        Thomas Nielsen
      10. d2998-doc-2.diff
        4 kB
        Thomas Nielsen
      11. d2998-doc-2.stat
        0.1 kB
        Thomas Nielsen
      12. d2998-followup-testsuite.stat
        0.1 kB
        Thomas Nielsen
      13. d2998-followup-testsuite.diff
        0.7 kB
        Thomas Nielsen
      14. d2998-followup-2.stat
        0.1 kB
        Thomas Nielsen
      15. d2998-followup-2.diff
        0.6 kB
        Thomas Nielsen
      16. d2998-followup-issue1.stat
        0.1 kB
        Thomas Nielsen
      17. d2998-followup-issue1.diff
        0.6 kB
        Thomas Nielsen
      18. d2998-test9.diff
        12 kB
        Thomas Nielsen
      19. d2998-19.stat
        2 kB
        Thomas Nielsen
      20. d2998-19.diff
        57 kB
        Thomas Nielsen
      21. d2998-test8.diff
        11 kB
        Thomas Nielsen
      22. d2998-18.stat
        2 kB
        Thomas Nielsen
      23. d2998-18.diff
        58 kB
        Thomas Nielsen
      24. d2998-17.stat
        1 kB
        Thomas Nielsen
      25. d2998-17.diff
        59 kB
        Thomas Nielsen
      26. d2998-16.stat
        2 kB
        Thomas Nielsen
      27. d2998-16.diff
        57 kB
        Thomas Nielsen
      28. d2998-test7.diff
        7 kB
        Thomas Nielsen
      29. d2998-15.stat
        1 kB
        Thomas Nielsen
      30. d2998-15.diff
        55 kB
        Thomas Nielsen
      31. d2998-14.stat
        1 kB
        Thomas Nielsen
      32. d2998-14.diff
        52 kB
        Thomas Nielsen
      33. d2998-test6.diff
        8 kB
        Thomas Nielsen
      34. d2998-13.stat
        1 kB
        Thomas Nielsen
      35. d2998-13.diff
        53 kB
        Thomas Nielsen
      36. d2998-12.stat
        1 kB
        Thomas Nielsen
      37. d2998-12.diff
        52 kB
        Thomas Nielsen
      38. d2998-test4.stat
        0.1 kB
        Thomas Nielsen
      39. d2998-test4.diff
        6 kB
        Thomas Nielsen
      40. d2998-11.diff
        50 kB
        Thomas Nielsen
      41. d2998-10.stat
        1 kB
        Thomas Nielsen
      42. d2998-10.diff
        52 kB
        Thomas Nielsen
      43. d2998-9-derby.log
        60 kB
        Thomas Nielsen
      44. d2998-9.stat
        1 kB
        Thomas Nielsen
      45. d2998-9.diff
        52 kB
        Thomas Nielsen
      46. d2998-8.diff
        46 kB
        Thomas Nielsen
      47. d2998-8.stat
        1 kB
        Thomas Nielsen
      48. d2998-doc-1.stat
        0.1 kB
        Thomas Nielsen
      49. d2998-doc-1.diff
        4 kB
        Thomas Nielsen
      50. d2998-test3.stat
        0.1 kB
        Thomas Nielsen
      51. d2998-test3.diff
        3 kB
        Thomas Nielsen
      52. d2998-7.stat
        1 kB
        Thomas Nielsen
      53. d2998-7.diff
        45 kB
        Thomas Nielsen
      54. d2998-6.stat
        1 kB
        Thomas Nielsen
      55. d2998-6.diff
        45 kB
        Thomas Nielsen
      56. d2998-test2.stat
        0.1 kB
        Thomas Nielsen
      57. d2998-test2.diff
        2 kB
        Thomas Nielsen
      58. d2998-test.stat
        0.1 kB
        Thomas Nielsen
      59. d2998-test.diff
        5 kB
        Thomas Nielsen
      60. d2998-5.diff
        39 kB
        Thomas Nielsen
      61. d2998-5.stat
        0.9 kB
        Thomas Nielsen
      62. d2998-4.stat
        1 kB
        Thomas Nielsen
      63. d2998-4.diff
        44 kB
        Thomas Nielsen

        Issue Links

          Activity

          Hide
          Thomas Nielsen added a comment -

          First off, we will need support for unnamed windows for this jira to be completed

          I'm off on the deep end of the pool, and could use some help and/or input on completing the prototype patch...

          I have extended the grammar, handling the row_number() function as an option in selectSubList(). Might not be the ideal place for all olap functions, but let's leave it there for the prototype patch.

          The basic idea is that bytecode is generated for the extended QueryTreeNode classes in the nodetree, and the bytecode will be run when the statement is actually executed after optimization. First I tried using the count aggregate as a "template" for row_number(). I learned a lot about how aggregates work in derby, but it weered off in the wrong direction.

          Second attempt introduce a new RowNumberColumNode that extends ConstantNode, hoping to assign an incremental number when building the ResultSet. ResultSetList.generateCore() seems to be the place where this should be done? Is there some existing machinery that I should/can hook into for doing the incrementation, like the autoincrement functionality for insert? Would it be better to extend from VirtualColumnNode instead of ConstantNode since the column isn't actually stored?

          My knowledge of the inner workings of derby are still fairly limited, and there isn't really a lot on the wiki on this except the header, so your help is greatly appreciated.

          Show
          Thomas Nielsen added a comment - First off, we will need support for unnamed windows for this jira to be completed I'm off on the deep end of the pool, and could use some help and/or input on completing the prototype patch... I have extended the grammar, handling the row_number() function as an option in selectSubList(). Might not be the ideal place for all olap functions, but let's leave it there for the prototype patch. The basic idea is that bytecode is generated for the extended QueryTreeNode classes in the nodetree, and the bytecode will be run when the statement is actually executed after optimization. First I tried using the count aggregate as a "template" for row_number(). I learned a lot about how aggregates work in derby, but it weered off in the wrong direction. Second attempt introduce a new RowNumberColumNode that extends ConstantNode, hoping to assign an incremental number when building the ResultSet. ResultSetList.generateCore() seems to be the place where this should be done? Is there some existing machinery that I should/can hook into for doing the incrementation, like the autoincrement functionality for insert? Would it be better to extend from VirtualColumnNode instead of ConstantNode since the column isn't actually stored? My knowledge of the inner workings of derby are still fairly limited, and there isn't really a lot on the wiki on this except the header, so your help is greatly appreciated.
          Hide
          Bryan Pendleton added a comment -

          Thomas, thanks for posting your findings so far. Knowing what doesn't work can
          be very useful, and is certainly worth recording. We will all be doing a lot of learning
          about Derby; that's the fun of open source!

          Here's a suggestion: perhaps you could investigate how "expressions" are computed,
          by tracing through the compilation and execution of statements such as:

          select substring(last_name from 1 for 10) from employee

          select case when age < 21 then 'minor' else 'adult' end from dependents

          select cost + cost * 0.0825 as cost_including_taxes from sales_history

          Can
          select row_number(), last_name, emp_id from employee
          be modeled in the same fashion?

          I agree that eventually we will have to consider the "window" concept to have a true
          implementation of this feature, but perhaps to start we can just build a simple
          scalar function that has reasonable behavior for its particular result set, and see
          what we can learn from that prototype implementation.

          Show
          Bryan Pendleton added a comment - Thomas, thanks for posting your findings so far. Knowing what doesn't work can be very useful, and is certainly worth recording. We will all be doing a lot of learning about Derby; that's the fun of open source! Here's a suggestion: perhaps you could investigate how "expressions" are computed, by tracing through the compilation and execution of statements such as: select substring(last_name from 1 for 10) from employee select case when age < 21 then 'minor' else 'adult' end from dependents select cost + cost * 0.0825 as cost_including_taxes from sales_history Can select row_number(), last_name, emp_id from employee be modeled in the same fashion? I agree that eventually we will have to consider the "window" concept to have a true implementation of this feature, but perhaps to start we can just build a simple scalar function that has reasonable behavior for its particular result set, and see what we can learn from that prototype implementation.
          Hide
          Thomas Nielsen added a comment -

          Thanks for chiming in Bryan!

          My plan for the prototype patch was, like you propose, to disregard the windowing entirely and enable simple queries like
          select row_number(), ... from t
          and treat row_number() is a simple scalar function.

          After having another look at how 'select a+b from t' works, I'm finally getting somewhere

          Currently at the point where I get one generated value per row returned in the ResultSet, although they are all the default value, they do come from my RowNumberColumnNode class. Now I need to find a way to do the incrementation.

          Show
          Thomas Nielsen added a comment - Thanks for chiming in Bryan! My plan for the prototype patch was, like you propose, to disregard the windowing entirely and enable simple queries like select row_number(), ... from t and treat row_number() is a simple scalar function. After having another look at how 'select a+b from t' works, I'm finally getting somewhere Currently at the point where I get one generated value per row returned in the ResultSet, although they are all the default value, they do come from my RowNumberColumnNode class. Now I need to find a way to do the incrementation.
          Hide
          Thomas Nielsen added a comment -

          In ResultSetList.generateCore() - the method that actually does the work - I now handle the case of a RownumberColumnNode instance, and attempt to invoke the incrementor function in RowNumberAggregator over in exec. RowNumberAggregator extends SystemAggregator as of now.

          When execution gets to GenericPreparedStatement.getActivation() my "select row_number() from t;" fails with:

          ERROR XBCM2: Cannot create an instance of generated class org.apache.derby.exe.ac601a400fx0114xd591xad7axffffe275b2700.
          ERROR XJ001: Java exception: '(class: org/apache/derby/exe/ac601a400fx0114xd591xad7axffffe275b2700, method: e1 signature: ()Ljava/lang/Object Incompatible object argument for function call: java.lang.VerifyError'.

          From what I can tell this means I'm either missing a generated class, or I'm passing the wrong type of argument to my RowNumberAggregator incrementor method?

          It would probably be good idea move the handling from ResultSetList.generateCore() to RowNumberColumnNode.generateExpression()?
          And the incrementor would perhaps be better placed in BaseAcitvation instead?

          Right now I feel like pounding nails with a shovel - you just know there's a better tool available for the job, you just have to find it...

          Show
          Thomas Nielsen added a comment - In ResultSetList.generateCore() - the method that actually does the work - I now handle the case of a RownumberColumnNode instance, and attempt to invoke the incrementor function in RowNumberAggregator over in exec. RowNumberAggregator extends SystemAggregator as of now. When execution gets to GenericPreparedStatement.getActivation() my "select row_number() from t;" fails with: ERROR XBCM2: Cannot create an instance of generated class org.apache.derby.exe.ac601a400fx0114xd591xad7axffffe275b2700. ERROR XJ001: Java exception: '(class: org/apache/derby/exe/ac601a400fx0114xd591xad7axffffe275b2700, method: e1 signature: ()Ljava/lang/Object Incompatible object argument for function call: java.lang.VerifyError'. From what I can tell this means I'm either missing a generated class, or I'm passing the wrong type of argument to my RowNumberAggregator incrementor method? It would probably be good idea move the handling from ResultSetList.generateCore() to RowNumberColumnNode.generateExpression()? And the incrementor would perhaps be better placed in BaseAcitvation instead? Right now I feel like pounding nails with a shovel - you just know there's a better tool available for the job, you just have to find it...
          Hide
          Knut Anders Hatlen added a comment -

          Hi Thomas,

          It would probably be easier for people to tell what's wrong and give advice if you posted a patch which shows what you have tried so far. I would guess the VerifyError is thrown because you're passing the wrong argument type. I think the error message would look different if you were missing a class.

          By the way, don't aggregators normally return one value for all rows within a group? Does that match what you're trying to achieve with row numbers?

          Show
          Knut Anders Hatlen added a comment - Hi Thomas, It would probably be easier for people to tell what's wrong and give advice if you posted a patch which shows what you have tried so far. I would guess the VerifyError is thrown because you're passing the wrong argument type. I think the error message would look different if you were missing a class. By the way, don't aggregators normally return one value for all rows within a group? Does that match what you're trying to achieve with row numbers?
          Hide
          Thomas Nielsen added a comment -

          Knut,

          You are right - sorry for rambling on like that. Anyway, After taking two steps back and having anohter look I now have the row number prototype working, and I'll post the prototype patch once I can get around to it.
          It works for the very basic stuff, but there's some things I know don't work.

          Show
          Thomas Nielsen added a comment - Knut, You are right - sorry for rambling on like that. Anyway, After taking two steps back and having anohter look I now have the row number prototype working, and I'll post the prototype patch once I can get around to it. It works for the very basic stuff, but there's some things I know don't work.
          Hide
          Thomas Nielsen added a comment - - edited

          I have attached my prototype for the row_number() implementation. It's not intended for commit.

          With a simple testtable t you can now do:

          ij> select row_number(),a,b from t;
          row_number() |A |B
          --------------------------------------------
          1 |1 |9
          2 |2 |8
          3 |3 |7

          AS clause is also functioning for this query.

          However,

          ij> select row_number() as r from t where r >= 2;
          ERROR 42X04: Column 'R' is either not in any table in the FROM list or appears within a join specification and is outside the scope of the join specification or appears in a HAVING clause and is not in the GROUP BY list. If this is a CREATE or ALTER TABLE statement then 'R' is not a column in the target table.

          This seems to be due to a limitation in derby. I get the same error with 'select a+b as r ...'. This can be rewritten using a nested select statement

          ij> select * from (select row_number() as r from t) as t(r) where r >= 2;
          R
          --------------------
          2
          3

          The patch works along these lines:
          I added a new class RowNumberColumnNode that is added to the querytree at compiletime. On invokation of ResultSetList.generateCore() we generate code to call new method BaseActivation.getSetRowNumber() that does the actual incrementing during execution.
          Diff and .stat files are attached.

          There is one issue I shortcut - the setup of the rnCache array in BaseActivation. To get it working I just created a 10 element array to hold the row_number() values from invocation to invocation. This should be either number of actual row_number() columns, or total number of columns in the resultset we are building.

          Your comments are greatly appreciated

          Show
          Thomas Nielsen added a comment - - edited I have attached my prototype for the row_number() implementation. It's not intended for commit. With a simple testtable t you can now do: — ij> select row_number(),a,b from t; row_number() |A |B -------------------------------------------- 1 |1 |9 2 |2 |8 3 |3 |7 AS clause is also functioning for this query. However, — ij> select row_number() as r from t where r >= 2; ERROR 42X04: Column 'R' is either not in any table in the FROM list or appears within a join specification and is outside the scope of the join specification or appears in a HAVING clause and is not in the GROUP BY list. If this is a CREATE or ALTER TABLE statement then 'R' is not a column in the target table. — This seems to be due to a limitation in derby. I get the same error with 'select a+b as r ...'. This can be rewritten using a nested select statement — ij> select * from (select row_number() as r from t) as t(r) where r >= 2; R -------------------- 2 3 — The patch works along these lines: I added a new class RowNumberColumnNode that is added to the querytree at compiletime. On invokation of ResultSetList.generateCore() we generate code to call new method BaseActivation.getSetRowNumber() that does the actual incrementing during execution. Diff and .stat files are attached. There is one issue I shortcut - the setup of the rnCache array in BaseActivation. To get it working I just created a 10 element array to hold the row_number() values from invocation to invocation. This should be either number of actual row_number() columns, or total number of columns in the resultset we are building. Your comments are greatly appreciated
          Hide
          Bryan Pendleton added a comment -

          Hi Thomas, thanks for posting the patch! I'll try to have a look at the code soon.

          Perhaps we could treat the inability to reference the "AS R" renamed column in the WHERE clause as a separate issue since, as you observe, it is a problem with all column expressions, not just row_number() expressions. The most commonly-requested use of row_number() is probably going to be for limiting the number of rows retried, as in DERBY-581.

          If you use the nested select form, to limit the results:

          select * from (select row_number() as rownum, * from t) where runum <= 10

          then does the execution logic actually short-circuit the evaluation properly after the first 10 rows have been read? Or does the execution of this query run through the entire table and then later end up discarding all the rows beyond the first 10?

          Since one of the primary goals of DERBY-581 is to efficiently limit the processing to only a small subset of the rows, we want to make sure that we achieve that result if possible.

          Show
          Bryan Pendleton added a comment - Hi Thomas, thanks for posting the patch! I'll try to have a look at the code soon. Perhaps we could treat the inability to reference the "AS R" renamed column in the WHERE clause as a separate issue since, as you observe, it is a problem with all column expressions, not just row_number() expressions. The most commonly-requested use of row_number() is probably going to be for limiting the number of rows retried, as in DERBY-581 . If you use the nested select form, to limit the results: select * from (select row_number() as rownum, * from t) where runum <= 10 then does the execution logic actually short-circuit the evaluation properly after the first 10 rows have been read? Or does the execution of this query run through the entire table and then later end up discarding all the rows beyond the first 10? Since one of the primary goals of DERBY-581 is to efficiently limit the processing to only a small subset of the rows, we want to make sure that we achieve that result if possible.
          Hide
          Thomas Nielsen added a comment - - edited

          Bryan,

          I agree, lets treat the "AS R ... WHERE" problem in a separate JIRA. There are some similar constructs that doesn't work either. Looks like it has to do with the WHERE clause working with the data before projection, and row_number(), for one, is added at projection. I haven't verified this though.

          If you do the nested select, I think you actually need to project the inner select into a temporary table to enable the outer SELECT ... WHERE to complete without a problem:
          ij> select * from (select row_number() as r, a, b from t) as t(r,a,b) where r > 2;
          R |A |B
          --------------------------------------------
          3 |3 |7

          This means there is no short-cut in the execution logic to only project the N first/next rows (as of now).

          The exact query you posted, does not actually seem to work though:
          ij> select * from (select row_number() as r, * from t) as t(r,*) where r>2;
          ERROR 42X01: Syntax error: Encountered "*" at line 1, column 42.

          Wildcard column specifier is not allowed in a comma separated list with column specifiers or expressions.

          However using 't.*' (kinda) works:
          ij> select * from (select row_number() as r, t.* from t) as t(r,*) where r>2;
          ERROR 42X01: Syntax error: Encountered "where" at line 1, column 55.

          ij> select * from (select row_number() as r, a, b from t) where r >=2;
          ERROR 42X01: Syntax error: Encountered "where" at line 1, column 55.

          The two last ones are valid queries AFAIK, and accepting WHERE here is possibly worthy of its own JIRA?

          Show
          Thomas Nielsen added a comment - - edited Bryan, I agree, lets treat the "AS R ... WHERE" problem in a separate JIRA. There are some similar constructs that doesn't work either. Looks like it has to do with the WHERE clause working with the data before projection, and row_number(), for one, is added at projection. I haven't verified this though. If you do the nested select, I think you actually need to project the inner select into a temporary table to enable the outer SELECT ... WHERE to complete without a problem: ij> select * from (select row_number() as r, a, b from t) as t(r,a,b) where r > 2; R |A |B -------------------------------------------- 3 |3 |7 This means there is no short-cut in the execution logic to only project the N first/next rows (as of now). The exact query you posted, does not actually seem to work though: ij> select * from (select row_number() as r, * from t) as t(r,*) where r>2; ERROR 42X01: Syntax error: Encountered "*" at line 1, column 42. Wildcard column specifier is not allowed in a comma separated list with column specifiers or expressions. However using 't.*' (kinda) works: ij> select * from (select row_number() as r, t.* from t) as t(r,*) where r>2; ERROR 42X01: Syntax error: Encountered "where" at line 1, column 55. ij> select * from (select row_number() as r, a, b from t) where r >=2; ERROR 42X01: Syntax error: Encountered "where" at line 1, column 55. The two last ones are valid queries AFAIK, and accepting WHERE here is possibly worthy of its own JIRA?
          Hide
          Thomas Nielsen added a comment -

          The "AS R ... WHERE" problem filed as DERBY-3066

          Show
          Thomas Nielsen added a comment - The "AS R ... WHERE" problem filed as DERBY-3066
          Hide
          Bryan Pendleton added a comment -

          Hi Thomas, regarding the syntax for the nested select, do these formulations work?

          select * from (select row_number() as r, t.* from t) as t_with_rownum where r < 10

          select * from (select row_number() as r, a, b from t) as t_with_rownum where r < 10

          That is, if we specify the "as <aliasname>" clause for the nested select, is the where clause recognized properly?

          Show
          Bryan Pendleton added a comment - Hi Thomas, regarding the syntax for the nested select, do these formulations work? select * from (select row_number() as r, t.* from t) as t_with_rownum where r < 10 select * from (select row_number() as r, a, b from t) as t_with_rownum where r < 10 That is, if we specify the "as <aliasname>" clause for the nested select, is the where clause recognized properly?
          Hide
          Thomas Nielsen added a comment -

          Yes Bryan, aliasing the nested select results in the where clause being properly recognized:

          ij> select * from (select row_number() as r, t.* from t) as t_with_rownum where r < 2;
          R |A |B
          --------------------------------------------
          1 |1 |9

          Show
          Thomas Nielsen added a comment - Yes Bryan, aliasing the nested select results in the where clause being properly recognized: ij> select * from (select row_number() as r, t.* from t) as t_with_rownum where r < 2; R |A |B -------------------------------------------- 1 |1 |9
          Hide
          Thomas Nielsen added a comment -

          Updated the prototype patch to use number of actual columns in resultset to size the rownumber cache array.
          Patch is still not intended for commit.

          Show
          Thomas Nielsen added a comment - Updated the prototype patch to use number of actual columns in resultset to size the rownumber cache array. Patch is still not intended for commit.
          Hide
          Thomas Nielsen added a comment -

          When I step through the code executing the aliased nested selects above there is (currently) no between the inner SELECT and the outer WHERE. This makes the inner ROW_NUMBER() to be computed for all rows in the table, and then the first N rows are selected in the WHERE clause. As previously stated there is no short-cut in the execution. Not a very effective idea if you have a million-and-one rows in the table and only want the first few.

          It would be strange if all subqueries are executed like this in derby - so on with the investigation...

          Show
          Thomas Nielsen added a comment - When I step through the code executing the aliased nested selects above there is (currently) no between the inner SELECT and the outer WHERE. This makes the inner ROW_NUMBER() to be computed for all rows in the table, and then the first N rows are selected in the WHERE clause. As previously stated there is no short-cut in the execution. Not a very effective idea if you have a million-and-one rows in the table and only want the first few. It would be strange if all subqueries are executed like this in derby - so on with the investigation...
          Hide
          Knut Anders Hatlen added a comment -

          Perhaps you need to update ResultSetNode.isOrderedOn() or some of its overrides?

          Show
          Knut Anders Hatlen added a comment - Perhaps you need to update ResultSetNode.isOrderedOn() or some of its overrides?
          Hide
          Thomas Nielsen added a comment -

          You have a valid point Knut.

          Further investigation shows that the inner row_number() is executed once for every outer execution - which is the way one would like to do this. The following output from a test table with 3 rows illustrate this. Never mind the bug not distinguishing between the two row_number()s for now, but it clearly shows the order in which this is executed:

          ij> select row_number(),r,a,b from (select row_number() as r, t.* from t) as tr where r <=3;
          row_number() |R |A |B
          -----------------------------------------------------------------
          2 |1 |1 |9
          4 |3 |2 |8

          2 rows selected

          If the inner resultset was flagged as ordered on R the outer might use that info to determine when to stop.

          Show
          Thomas Nielsen added a comment - You have a valid point Knut. Further investigation shows that the inner row_number() is executed once for every outer execution - which is the way one would like to do this. The following output from a test table with 3 rows illustrate this. Never mind the bug not distinguishing between the two row_number()s for now, but it clearly shows the order in which this is executed: ij> select row_number(),r,a,b from (select row_number() as r, t.* from t) as tr where r <=3; row_number() |R |A |B ----------------------------------------------------------------- 2 |1 |1 |9 4 |3 |2 |8 2 rows selected If the inner resultset was flagged as ordered on R the outer might use that info to determine when to stop.
          Hide
          Thomas Nielsen added a comment -

          Updated the patch to distinguish differnet row_number() instances based on which ResultSet they are in.

          ij> select row_number(), r, b from (select row_number() as r, t.* from t) as tr where r >= 2;
          row_number() |R |B
          -----------------------------------------------------
          1 |2 |8
          2 |3 |7

          2 rows selected

          Patch still not intended for commit.

          Show
          Thomas Nielsen added a comment - Updated the patch to distinguish differnet row_number() instances based on which ResultSet they are in. ij> select row_number(), r, b from (select row_number() as r, t.* from t) as tr where r >= 2; row_number() |R |B ----------------------------------------------------- 1 |2 |8 2 |3 |7 2 rows selected Patch still not intended for commit.
          Hide
          Thomas Nielsen added a comment -

          Marking the row_number() result columns using ResultColumn.markAsGroupingColumn() while in ResultColumnList.generateCore() is not enough to stop the execution of the inner row_number() with a WHILE clause at least... Investigation still ongoing. Any other hints or tips appreaciated.

          Show
          Thomas Nielsen added a comment - Marking the row_number() result columns using ResultColumn.markAsGroupingColumn() while in ResultColumnList.generateCore() is not enough to stop the execution of the inner row_number() with a WHILE clause at least... Investigation still ongoing. Any other hints or tips appreaciated.
          Hide
          Thomas Nielsen added a comment -

          Just to rule it out I applied the patch for DERBY-1905 (optimizer subquery cost way too high), and it didn't make a difference.
          So it's something I'm not doing that's not causing the optimizer to push the WHERE clause into the subquery to make it stop...

          Show
          Thomas Nielsen added a comment - Just to rule it out I applied the patch for DERBY-1905 (optimizer subquery cost way too high), and it didn't make a difference. So it's something I'm not doing that's not causing the optimizer to push the WHERE clause into the subquery to make it stop...
          Hide
          Thomas Nielsen added a comment -

          Working my way through the optimization of the simple query:
          select * from (select row_number() as r, t.* from t) as tr where r < 2;
          I see that during preprocessing the WHERE clause is converted to a PredicateList. The predicate BinaryOperatorNode (my '<' lessThan) is successfully categorize()'d as pushable, which is a good thing!
          However, ProjectRestrictNode.pushExpressionsIntoSelect() in PRN.pushExpressions() does not do anything. I would have thought this was where the predicate would be pushed into the select subquery.
          As a side note DERBY-649 seems to have fixed a similar issue by pushing predicates into UnionNodes. Investigation ongoing.

          Show
          Thomas Nielsen added a comment - Working my way through the optimization of the simple query: select * from (select row_number() as r, t.* from t) as tr where r < 2; I see that during preprocessing the WHERE clause is converted to a PredicateList. The predicate BinaryOperatorNode (my '<' lessThan) is successfully categorize()'d as pushable, which is a good thing! However, ProjectRestrictNode.pushExpressionsIntoSelect() in PRN.pushExpressions() does not do anything. I would have thought this was where the predicate would be pushed into the select subquery. As a side note DERBY-649 seems to have fixed a similar issue by pushing predicates into UnionNodes. Investigation ongoing.
          Hide
          Thomas Nielsen added a comment -

          My problem at this stage is very basic - whenever the restriction is fulfilled, stop the execution - but actually doing it in code is a little worse it seems.
          I've been looking at the optimizer and generation of ProjectRestictNodes, but got nowhere.

          After an offline chat with Knut, I started looking at how a table scan is stopped for indexes. Luckily it seems like IndexToBaseRowNode does something similar to what I want to accomplish in it's generate() method.

          Show
          Thomas Nielsen added a comment - My problem at this stage is very basic - whenever the restriction is fulfilled, stop the execution - but actually doing it in code is a little worse it seems. I've been looking at the optimizer and generation of ProjectRestictNodes, but got nowhere. After an offline chat with Knut, I started looking at how a table scan is stopped for indexes. Luckily it seems like IndexToBaseRowNode does something similar to what I want to accomplish in it's generate() method.
          Hide
          Thomas Nielsen added a comment -

          After Dags work on ROLEs, I needed a slight tweak to this patch. Nothing else new.

          Show
          Thomas Nielsen added a comment - After Dags work on ROLEs, I needed a slight tweak to this patch. Nothing else new.
          Hide
          Thomas Nielsen added a comment - - edited

          Here's how it seems to work for indexes for the following simple query on table t where column a is the primary key.

          select * from t where a <= 2;

          During parsing we add a CursorNode and an IndexToBaseRowNode to the QueryTree since we have an index on 'a'.
          Based on the IndexToBaseRowNode we create the accompanying IndexRowToBaseRowResultSet and feed any restrictions to its constructor.
          In my case the restriction is generated code for '<='.

          On opening the IndexRowToBaseRowResultSet we also call its "source" .openCore() method BulkTableScanResultSet.openCore().
          BulkTableScanResultSet does just that, it reads a base table or an index in bulk (chunks) from Store.

          The execution then does a do-while loop that calls .getNextRowCore(). For each pass it invokes the restriction code it got in its constructor by restriction.invoke(). In my case this ends up invoking SQLInteger.lessOrEqual(). The do-while loop exits once the restriction is met.

          The key seems to be the three calls to openCore(), getNextRowCore() and closeCore(), and overrides of these in the ResultSet subclasses.

          Show
          Thomas Nielsen added a comment - - edited Here's how it seems to work for indexes for the following simple query on table t where column a is the primary key. select * from t where a <= 2; During parsing we add a CursorNode and an IndexToBaseRowNode to the QueryTree since we have an index on 'a'. Based on the IndexToBaseRowNode we create the accompanying IndexRowToBaseRowResultSet and feed any restrictions to its constructor. In my case the restriction is generated code for '<='. On opening the IndexRowToBaseRowResultSet we also call its "source" .openCore() method BulkTableScanResultSet.openCore(). BulkTableScanResultSet does just that, it reads a base table or an index in bulk (chunks) from Store. The execution then does a do-while loop that calls .getNextRowCore(). For each pass it invokes the restriction code it got in its constructor by restriction.invoke(). In my case this ends up invoking SQLInteger.lessOrEqual(). The do-while loop exits once the restriction is met. The key seems to be the three calls to openCore(), getNextRowCore() and closeCore(), and overrides of these in the ResultSet subclasses.
          Hide
          Thomas Nielsen added a comment -

          When I execute the "normal" query on a table without an index

          ij> select * from (select row_number() as r, t.* from t) as tr where r < 2;

          With this query I get a BasicNoPutResultSet, that in turn uses the same BulkTableScanResultSet for fetching the rows as the index query above. However, BasicNoPutResultSet does not take any restrictions into consideration at all. The execution is not stopped but continues on with the full table scan. The restriction for the resultset is enforced at a higher level.

          Two options stand out for moving this ahead:
          1)
          extend the current BasicNoPutResultSet to take the restrictions (if any) into consideration.
          2)
          A better approach might be to create a new NoPutResultSet subclass ("OLAPResultSet"?) that will use the getNextRowCore() approach of IndexRowFromBaseRowResultSet. I have a hunch there will be other scenarios where one would need an "OLAPResultSet" when we proceed with other subtasks of the OLAP functionallity (DERBY-581), and this would probably give the lesser amount of clutter to the existing ResultSet classes?

          Show
          Thomas Nielsen added a comment - When I execute the "normal" query on a table without an index ij> select * from (select row_number() as r, t.* from t) as tr where r < 2; With this query I get a BasicNoPutResultSet, that in turn uses the same BulkTableScanResultSet for fetching the rows as the index query above. However, BasicNoPutResultSet does not take any restrictions into consideration at all. The execution is not stopped but continues on with the full table scan. The restriction for the resultset is enforced at a higher level. Two options stand out for moving this ahead: 1) extend the current BasicNoPutResultSet to take the restrictions (if any) into consideration. 2) A better approach might be to create a new NoPutResultSet subclass ("OLAPResultSet"?) that will use the getNextRowCore() approach of IndexRowFromBaseRowResultSet. I have a hunch there will be other scenarios where one would need an "OLAPResultSet" when we proceed with other subtasks of the OLAP functionallity ( DERBY-581 ), and this would probably give the lesser amount of clutter to the existing ResultSet classes?
          Hide
          Thomas Nielsen added a comment -

          Where would the best place be to insert/use/introduce the OLAPResultSet?

          • in sqlgrammar.jj, when we add the RowNumberColumnNode we could also add an "umbrella" OLAPResultSetNode to the resultset?
          • in ProjectRestrictNode.modifyAccessPath(), once the optimizer selects the best access path to the data?
          • any other place I've missed?

          Index based queries run the second option and will insert an IndexToBaseRowNode in the querytree during optimization if an index is present and will be used for accessing the rows. This ends up generating the IndexRowToBaseRowResultSet bytecode.
          Ordering queries use the first option (if I'm not mistaken), and insert a OrderByNode during querytree generation, ending up in the proper flagging of the existing resultset as ordered.

          I know upfront that I will need a OLAPResultSet, so it might be appropriate to just add one in sqlgrammar.jj? But then again it might be irrelvant so I should add it at optimizing?

          Any input appreciated!

          Show
          Thomas Nielsen added a comment - Where would the best place be to insert/use/introduce the OLAPResultSet? in sqlgrammar.jj, when we add the RowNumberColumnNode we could also add an "umbrella" OLAPResultSetNode to the resultset? in ProjectRestrictNode.modifyAccessPath(), once the optimizer selects the best access path to the data? any other place I've missed? Index based queries run the second option and will insert an IndexToBaseRowNode in the querytree during optimization if an index is present and will be used for accessing the rows. This ends up generating the IndexRowToBaseRowResultSet bytecode. Ordering queries use the first option (if I'm not mistaken), and insert a OrderByNode during querytree generation, ending up in the proper flagging of the existing resultset as ordered. I know upfront that I will need a OLAPResultSet, so it might be appropriate to just add one in sqlgrammar.jj? But then again it might be irrelvant so I should add it at optimizing? Any input appreciated!
          Hide
          Thomas Nielsen added a comment -

          Attaching a new, working, but still not properly optimized, patch for the ROW_NUMBER() implementation.

          This patch inserts a OLAPNode in the QueryTree, and generates an OLAPResultSet fetching rows from any given source ResultSet. The implementation of OLAPNode and OLAPResultSet is based on IndexToBaseRowNode and IndexRowToBaseRowResultSet respectively.

          The missing optimization is due to the restriction (whereClause) not being properly recognized or passed to the inner select in the ROW_NUMBER() construct:
          select * from (select row_number() as r, t.* from t) as tr where r <= N;

          The code in OLAPResultSet will check the restriction for all rows it sees, if defined.

          As of now, the whereClause is changed to wherePredicates before passing them to the inner Select. But for the inner select each Predicate in the wherePredicates have (Prediacte.)scoped=false.

          Patch still not intended for commit...

          Show
          Thomas Nielsen added a comment - Attaching a new, working, but still not properly optimized, patch for the ROW_NUMBER() implementation. This patch inserts a OLAPNode in the QueryTree, and generates an OLAPResultSet fetching rows from any given source ResultSet. The implementation of OLAPNode and OLAPResultSet is based on IndexToBaseRowNode and IndexRowToBaseRowResultSet respectively. The missing optimization is due to the restriction (whereClause) not being properly recognized or passed to the inner select in the ROW_NUMBER() construct: select * from (select row_number() as r, t.* from t) as tr where r <= N; The code in OLAPResultSet will check the restriction for all rows it sees, if defined. As of now, the whereClause is changed to wherePredicates before passing them to the inner Select. But for the inner select each Predicate in the wherePredicates have (Prediacte.)scoped=false. Patch still not intended for commit...
          Hide
          Thomas Nielsen added a comment -

          My last patch works, it's just not optimized. If someone wants to have a look and comment on it I'm all ears

          Still investigating why the restriction given in the outer select isn't passed on to the inner select.
          At the moment it seems like the restriction is skipped in ProjectRestrictNode.pushExpressions(). Here, the restriction has become a member in a PredicateList, on which we call predicateList.pushExpressionsIntoSelect() which checks ColumnRefereces and decides wether to push the predicate or not. It would seem this check somehow fails for the RowNumberColumn.

          Show
          Thomas Nielsen added a comment - My last patch works, it's just not optimized. If someone wants to have a look and comment on it I'm all ears Still investigating why the restriction given in the outer select isn't passed on to the inner select. At the moment it seems like the restriction is skipped in ProjectRestrictNode.pushExpressions(). Here, the restriction has become a member in a PredicateList, on which we call predicateList.pushExpressionsIntoSelect() which checks ColumnRefereces and decides wether to push the predicate or not. It would seem this check somehow fails for the RowNumberColumn.
          Hide
          Thomas Nielsen added a comment -

          After changing predicateList.pushExpressionsIntoSelect to not ditch any RowNumberColumnNode expressions, the query seems to be properly optimized and stopped as expected after the restriction is fulfilled.

          I'll post an updated patch very very soon

          Show
          Thomas Nielsen added a comment - After changing predicateList.pushExpressionsIntoSelect to not ditch any RowNumberColumnNode expressions, the query seems to be properly optimized and stopped as expected after the restriction is fulfilled. I'll post an updated patch very very soon
          Hide
          Thomas Nielsen added a comment -

          Attaching 'd2998-4.diff' and 'd2998-4.stat' which includes optimization as well. It's still a little rough around the edges, but seems to work for this query:
          select * from (select row_number() as r, t.* from t) as tr where r <= 2;

          Comments are very welcome.

          Tip: Uncomment the (crude) System.out.println() in BaseActivation see the computation.

          Show
          Thomas Nielsen added a comment - Attaching 'd2998-4.diff' and 'd2998-4.stat' which includes optimization as well. It's still a little rough around the edges, but seems to work for this query: select * from (select row_number() as r, t.* from t) as tr where r <= 2; Comments are very welcome. Tip: Uncomment the (crude) System.out.println() in BaseActivation see the computation.
          Hide
          Thomas Nielsen added a comment -

          After pushing the restriction down (PredicateList.java @ 1468), it seems queries with < and <= restrictions are executed nicely. But > and >= are not. Materialization of the row_number() column happens too late, so even if you get the correct base table rows, they are still numbered starting from 1 it seems.

          Show
          Thomas Nielsen added a comment - After pushing the restriction down (PredicateList.java @ 1468), it seems queries with < and <= restrictions are executed nicely. But > and >= are not. Materialization of the row_number() column happens too late, so even if you get the correct base table rows, they are still numbered starting from 1 it seems.
          Hide
          Knut Anders Hatlen added a comment -

          Hi Thomas,

          It seems like the predicates are pushed too far down in some cases for < as well. Try for instance a join where the first rows of one the tables are not part of the result:

          ij> create table t1 (x int);
          0 rows inserted/updated/deleted
          ij> create table t2 (y int);
          0 rows inserted/updated/deleted
          ij> insert into t1 values 1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20;
          20 rows inserted/updated/deleted
          ij> insert into t2 values 19,20,21,22,23,24,25,26,27,28,29,30;
          12 rows inserted/updated/deleted
          ij> select row_number(),x from t1,t2 where x=y;
          row_number() |X
          --------------------------------
          1 |19
          2 |20

          2 rows selected
          ij> select * from (select row_number(),x from t1,t2 where x=y) s(r,x) where r < 3;
          R |X
          --------------------------------

          0 rows selected

          Show
          Knut Anders Hatlen added a comment - Hi Thomas, It seems like the predicates are pushed too far down in some cases for < as well. Try for instance a join where the first rows of one the tables are not part of the result: ij> create table t1 (x int); 0 rows inserted/updated/deleted ij> create table t2 (y int); 0 rows inserted/updated/deleted ij> insert into t1 values 1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20; 20 rows inserted/updated/deleted ij> insert into t2 values 19,20,21,22,23,24,25,26,27,28,29,30; 12 rows inserted/updated/deleted ij> select row_number(),x from t1,t2 where x=y; row_number() |X -------------------------------- 1 |19 2 |20 2 rows selected ij> select * from (select row_number(),x from t1,t2 where x=y) s(r,x) where r < 3; R |X -------------------------------- 0 rows selected
          Hide
          Thomas Nielsen added a comment -

          '2998-5.diff' removes the faulty predicate push, and is a tidy, working, but not optimized implementation of
          .. ROW_NUMBER() OVER () ...

          Show
          Thomas Nielsen added a comment - '2998-5.diff' removes the faulty predicate push, and is a tidy, working, but not optimized implementation of .. ROW_NUMBER() OVER () ...
          Hide
          Thomas Nielsen added a comment -

          I started comparing
          select * from (select row_number() as r, t.* from t) as tr where r < 4;
          to
          select * from (select a*b as r, t.* from t) as tr where r < 4;

          i.e change row_number() for a*b as the expression in the inner select.

          In both these cases the top of the QueryTree looks like

          SELECT

          PRN

          SELECT

          PRN
          ...

          where the SelectNode fromList point to the below ProjectRestrictNode (PRN), and the PRNs childResult point to the second SelectNode and so on.

          Even in the a*b scenario, the wherePredicate in the top SelectNode is not pushed below the first PRN, since it is referencing an expression. This means as of now the inner 'select a*b as r ...' results in a full table scan, returning all rows to the outer select, and the restriction is checked at the outer select which in the end return only a few rows.

          It would seem like a reasonable/good/necessary optimization to push the where predicate down into the second select?
          I can't remember seeing any jiras relating to this off the top of my head. In the general case one could possibly argue that it does not really matter where it is restricted as long as it's done in the engine, but in the row_number() case and possibly with indexes and sorted inner results, you will take a (potentially severe) performance hit?

          Show
          Thomas Nielsen added a comment - I started comparing select * from (select row_number() as r, t.* from t) as tr where r < 4; to select * from (select a*b as r, t.* from t) as tr where r < 4; i.e change row_number() for a*b as the expression in the inner select. In both these cases the top of the QueryTree looks like SELECT PRN SELECT PRN ... where the SelectNode fromList point to the below ProjectRestrictNode (PRN), and the PRNs childResult point to the second SelectNode and so on. Even in the a*b scenario, the wherePredicate in the top SelectNode is not pushed below the first PRN, since it is referencing an expression. This means as of now the inner 'select a*b as r ...' results in a full table scan, returning all rows to the outer select, and the restriction is checked at the outer select which in the end return only a few rows. It would seem like a reasonable/good/necessary optimization to push the where predicate down into the second select? I can't remember seeing any jiras relating to this off the top of my head. In the general case one could possibly argue that it does not really matter where it is restricted as long as it's done in the engine, but in the row_number() case and possibly with indexes and sorted inner results, you will take a (potentially severe) performance hit?
          Hide
          Thomas Nielsen added a comment -

          Attaching a minimal junit test for row_number()

          Show
          Thomas Nielsen added a comment - Attaching a minimal junit test for row_number()
          Hide
          Dyre Tjeldvoll added a comment -

          I committed the minimal junit test for row_number(). I expect that this is "work in progress" as the test was not added to a suite (reasonable since the functionality being tested isn't available yet .

          One comment: I see the JUnit gurus advocating the use of the harness utility function createStatement() because Statement objects then will be closed automatically. You may want to consider this for the final version.

          Show
          Dyre Tjeldvoll added a comment - I committed the minimal junit test for row_number(). I expect that this is "work in progress" as the test was not added to a suite (reasonable since the functionality being tested isn't available yet . One comment: I see the JUnit gurus advocating the use of the harness utility function createStatement() because Statement objects then will be closed automatically. You may want to consider this for the final version.
          Hide
          Knut Anders Hatlen added a comment -

          It would also be good to document why the lock timeouts need to be lowered. It's not obvious to me, as a single connection shouldn't cause any lock conflicts.

          Show
          Knut Anders Hatlen added a comment - It would also be good to document why the lock timeouts need to be lowered. It's not obvious to me, as a single connection shouldn't cause any lock conflicts.
          Hide
          Thomas Nielsen added a comment -

          Thanks Dyre.
          Yes, it's still work in progress. I'll look into the createStatement() harness utility function.

          Knut:
          That's a copy-paste error on my part. I based the new test on SimpleTest. There should be no need to lower the lock timeouts.

          Show
          Thomas Nielsen added a comment - Thanks Dyre. Yes, it's still work in progress. I'll look into the createStatement() harness utility function. Knut: That's a copy-paste error on my part. I based the new test on SimpleTest. There should be no need to lower the lock timeouts.
          Hide
          Thomas Nielsen added a comment -

          'd2998-test2.diff' is a patch for the committed test fixing the comments from Dyre and Knut on createStatement() and lock settings.

          Show
          Thomas Nielsen added a comment - 'd2998-test2.diff' is a patch for the committed test fixing the comments from Dyre and Knut on createStatement() and lock settings.
          Hide
          Knut Anders Hatlen added a comment -

          Thanks Thomas. Committed revision 601309. I made a small change before committing: Since TestConfiguration.defaultSuite() wraps the suite in a CleanDatabaseTestSetup internally, I removed the explicit wrapping in the suite() method.

          Show
          Knut Anders Hatlen added a comment - Thanks Thomas. Committed revision 601309. I made a small change before committing: Since TestConfiguration.defaultSuite() wraps the suite in a CleanDatabaseTestSetup internally, I removed the explicit wrapping in the suite() method.
          Hide
          Thomas Nielsen added a comment -

          Thanks Knut - also for spotting that one...

          Show
          Thomas Nielsen added a comment - Thanks Knut - also for spotting that one...
          Hide
          Thomas Nielsen added a comment -

          'd2998-6.diff' has the following updates:

          • adds statistics for queryplan dumps (-Dderby.language.logQueryPlan=true)
          • refactors newly introduced class OLAPResultSetImpl to OLAPResultSet to align with similar classes.
          • fixes a few issues with the patch due to other recent changes
          Show
          Thomas Nielsen added a comment - 'd2998-6.diff' has the following updates: adds statistics for queryplan dumps (-Dderby.language.logQueryPlan=true) refactors newly introduced class OLAPResultSetImpl to OLAPResultSet to align with similar classes. fixes a few issues with the patch due to other recent changes
          Hide
          Thomas Nielsen added a comment -

          An email discussion with A B on derby-dev indicates that the conditional pushing I originally did in PredicateList was correct after all
          So I need to sort out how I can get the failing join Knut reported to work, and also why the less than works, but greater than doesn't.

          I started looking at triggerig materializing the OLAPResultSet by returning true from OLAPNode.performMaterialization() as one option.

          I might need to move the generation of calls to increment code from ResultColumnList.generateCore() and down into RowNumberColumnNode.generateCore(). I'm also wondering wether the call shouldn't be made into BaseActivation like in d2998-6.diff.

          I'll keep digging...

          Show
          Thomas Nielsen added a comment - An email discussion with A B on derby-dev indicates that the conditional pushing I originally did in PredicateList was correct after all So I need to sort out how I can get the failing join Knut reported to work, and also why the less than works, but greater than doesn't. I started looking at triggerig materializing the OLAPResultSet by returning true from OLAPNode.performMaterialization() as one option. I might need to move the generation of calls to increment code from ResultColumnList.generateCore() and down into RowNumberColumnNode.generateCore(). I'm also wondering wether the call shouldn't be made into BaseActivation like in d2998-6.diff. I'll keep digging...
          Hide
          Daniel John Debrunner added a comment -

          Is there a write-up on what this issue is planning to produce?
          The linked to wiki page clearly shows the syntax but most of the current examples in the test (and in most comments in this issue) do not follow that syntax.

          E.g. select row_number(), t1.* from t1

          I understand that it's a work in progress, but is the final aim to support more than

          ROW_NUMBER() OVER <window name or specification>

          and hence have non-standard expressions? Or are the non-standard examples just a interim development step that will be removed before any release?

          http://wiki.apache.org/db-derby/OLAPRowNumber

          Show
          Daniel John Debrunner added a comment - Is there a write-up on what this issue is planning to produce? The linked to wiki page clearly shows the syntax but most of the current examples in the test (and in most comments in this issue) do not follow that syntax. E.g. select row_number(), t1.* from t1 I understand that it's a work in progress, but is the final aim to support more than ROW_NUMBER() OVER <window name or specification> and hence have non-standard expressions? Or are the non-standard examples just a interim development step that will be removed before any release? http://wiki.apache.org/db-derby/OLAPRowNumber
          Hide
          Thomas Nielsen added a comment -

          Dan,

          Yes, this is still a work in progress and a lot of the comments reflect this. I dove off the deep end to get my feet wet with derby, and I'm still trying to learn how to swim

          That linked wikipage is, or at least was intended to be, the writeup of what this issue should end as.

          The future goal is for derby to support window functions over both named and unnamed windows, but that needs to be done in multiple jiras IMHO as it's a large task. This jira will only implement the ROW_NUMBER() function, and not the window specification (OVER <window name or specification>). As long as the window specification remains unsupported, the syntax will be deviating from the standard. It's not about introducing non-standard expressions, but a step towards having standard window functions in derby.

          In attached patch 'd2998-6', I accept 'OVER ()' silently in sqlgrammar.jj. That should change to accepting 'OVER <window name or spec>'. At the same time it may be a good idea to throw an unsupported type exception if the user attempts using a window name or specification?

          Not having support for <window name or specification> is the reason for the test not using the window name or specification as of now.

          Show
          Thomas Nielsen added a comment - Dan, Yes, this is still a work in progress and a lot of the comments reflect this. I dove off the deep end to get my feet wet with derby, and I'm still trying to learn how to swim That linked wikipage is, or at least was intended to be, the writeup of what this issue should end as. The future goal is for derby to support window functions over both named and unnamed windows, but that needs to be done in multiple jiras IMHO as it's a large task. This jira will only implement the ROW_NUMBER() function, and not the window specification (OVER <window name or specification>). As long as the window specification remains unsupported, the syntax will be deviating from the standard. It's not about introducing non-standard expressions, but a step towards having standard window functions in derby. In attached patch 'd2998-6', I accept 'OVER ()' silently in sqlgrammar.jj. That should change to accepting 'OVER <window name or spec>'. At the same time it may be a good idea to throw an unsupported type exception if the user attempts using a window name or specification? Not having support for <window name or specification> is the reason for the test not using the window name or specification as of now.
          Hide
          Bryan Pendleton added a comment -

          Perhaps, for this phase of the implementation, we should require OVER (),
          and the test cases should all use this format.

          Show
          Bryan Pendleton added a comment - Perhaps, for this phase of the implementation, we should require OVER (), and the test cases should all use this format.
          Hide
          Daniel John Debrunner added a comment -

          Thanks Thomas, I agree that the implementation path you are on is good approach, but at some point someone will want to produce a release that is branched off the trunk, we just need to ensure that non-standard use of ROW_NUMBER is not supported. That would either mean disabling ROW_NUMBER in the branch (e.g. 10.4) or not supporting non-standard use in the trunk at any time.
          Probably any decision can be deferred until closer to the next release, which is due in Feb.
          Note that the 10.4 wiki page does indicate that ROW_NUMBER() is to be supported in 10.4, from the comments in this issue it seems unlikely a conformant ROW_NUMBER will be finished by then??

          Show
          Daniel John Debrunner added a comment - Thanks Thomas, I agree that the implementation path you are on is good approach, but at some point someone will want to produce a release that is branched off the trunk, we just need to ensure that non-standard use of ROW_NUMBER is not supported. That would either mean disabling ROW_NUMBER in the branch (e.g. 10.4) or not supporting non-standard use in the trunk at any time. Probably any decision can be deferred until closer to the next release, which is due in Feb. Note that the 10.4 wiki page does indicate that ROW_NUMBER() is to be supported in 10.4, from the comments in this issue it seems unlikely a conformant ROW_NUMBER will be finished by then??
          Hide
          Bryan Pendleton added a comment -

          Dan, do you believe that ROW_NUMBER OVER () is a non-standard use of ROW_NUMBER?

          Show
          Bryan Pendleton added a comment - Dan, do you believe that ROW_NUMBER OVER () is a non-standard use of ROW_NUMBER?
          Hide
          Mamta A. Satoor added a comment -

          I thought I would chime in here and say that ROW_NUMBER OVER () is SQL standards compliant.

          SQL spec has following (Section 7.11 <window clause>)
          <window specification> ::=
          <left paren> <window specification details> <right paren>
          <window specification details> ::=
          [ <existing window name> ]
          [ <window partition clause> ]
          [ <window order clause> ]
          [ <window frame clause> ]

          This indicates that window specificatoin details are optional.

          Show
          Mamta A. Satoor added a comment - I thought I would chime in here and say that ROW_NUMBER OVER () is SQL standards compliant. SQL spec has following (Section 7.11 <window clause>) <window specification> ::= <left paren> <window specification details> <right paren> <window specification details> ::= [ <existing window name> ] [ <window partition clause> ] [ <window order clause> ] [ <window frame clause> ] This indicates that window specificatoin details are optional.
          Hide
          Daniel John Debrunner added a comment -

          > do you believe that ROW_NUMBER OVER () is a non-standard use of ROW_NUMBER?

          No idea, Mamta points out that the syntax grammar allows it. I haven't gone through all the syntax and general rules in 6.10 & 7.11 to see if there are additional conditions that would lead an empty window specification to be invalid.

          (e.g. the syntax grammar for routine creation allows multiple LANGUAGE clauses, but the rules specify "at most one" language clause, so the syntax grammar is not the final say on if a statement is valid or not)

          Show
          Daniel John Debrunner added a comment - > do you believe that ROW_NUMBER OVER () is a non-standard use of ROW_NUMBER? No idea, Mamta points out that the syntax grammar allows it. I haven't gone through all the syntax and general rules in 6.10 & 7.11 to see if there are additional conditions that would lead an empty window specification to be invalid. (e.g. the syntax grammar for routine creation allows multiple LANGUAGE clauses, but the rules specify "at most one" language clause, so the syntax grammar is not the final say on if a statement is valid or not)
          Hide
          Thomas Nielsen added a comment -

          Thanks for taking the time to all of you. Confirmation that I'm on the right path is also very good to get.

          I was under the impression that
          ROW_NUMBER() OVER ()
          was compliant and meaning over the unsorted resultset. This construct was indeed what I was hoping to have ready for 10.4. I'll have another look at the spec, but would appreciate if someone else could have a look as well. It's not an easy read.

          Based on your comments I've attached 'd2998.7.diff' which requires the OVER () window specfication in sqlgrammar.jj, and a patch for the test with the necessary changes.

          Show
          Thomas Nielsen added a comment - Thanks for taking the time to all of you. Confirmation that I'm on the right path is also very good to get. I was under the impression that ROW_NUMBER() OVER () was compliant and meaning over the unsorted resultset. This construct was indeed what I was hoping to have ready for 10.4. I'll have another look at the spec, but would appreciate if someone else could have a look as well. It's not an easy read. Based on your comments I've attached 'd2998.7.diff' which requires the OVER () window specfication in sqlgrammar.jj, and a patch for the test with the necessary changes.
          Hide
          Thomas Nielsen added a comment -

          Test patch for d2998-7.diff which requires OVER ()

          Show
          Thomas Nielsen added a comment - Test patch for d2998-7.diff which requires OVER ()
          Hide
          Daniel John Debrunner added a comment -

          Looking at the spec I think ROW_NUMBER() OVER () is compliant.

          Section 6.10 SR 6) covers ROW_NUMBER and rank function and there is explicit text (SR 6a) that says:
          >>>> If RANK or DENSE_RANK is specified, then the window ordering clause WOC of WDX shall be present.

          I can't see any such restrictions for ROW_NUMBER in SR 6.

          Also from googling I saw that IBM's RedBrick supports ROW_NUMBER() OVER ().

          Show
          Daniel John Debrunner added a comment - Looking at the spec I think ROW_NUMBER() OVER () is compliant. Section 6.10 SR 6) covers ROW_NUMBER and rank function and there is explicit text (SR 6a) that says: >>>> If RANK or DENSE_RANK is specified, then the window ordering clause WOC of WDX shall be present. I can't see any such restrictions for ROW_NUMBER in SR 6. Also from googling I saw that IBM's RedBrick supports ROW_NUMBER() OVER ().
          Hide
          Daniel John Debrunner added a comment -

          > ROW_NUMBER() OVER ()
          > This construct was indeed what I was hoping to have ready for 10.4.

          Sounds great, and I think a sub-set of standard behaviour is fine for any release. Since this is open source no one can say you must implement all of a standard feature (e.g. T611), only be thankful for any progress towards its completion.

          Show
          Daniel John Debrunner added a comment - > ROW_NUMBER() OVER () > This construct was indeed what I was hoping to have ready for 10.4. Sounds great, and I think a sub-set of standard behaviour is fine for any release. Since this is open source no one can say you must implement all of a standard feature (e.g. T611), only be thankful for any progress towards its completion.
          Hide
          xiangzhouwang added a comment -

          I really need it!

          Show
          xiangzhouwang added a comment - I really need it!
          Hide
          Thomas Nielsen added a comment -

          It seems we all agree on ROW_NUMBER() OVER () being compliant - good

          The materialization path puts me in somewhat of a corner when I try to do connect-the-dots. I get a two column ExecRow from the materialized resultset, that is put into a three column ExecRow returned from OLAPResultSet.getNextRowCore(). That results in an NPE at a later stage. I had a look at HashJoins and ProjectRestrictNode which ought to do more or less the same, as well as the javadocs for various descriptors available in OLAPResultSet.getNextRowCore(), but it's not obvious (yet) how I'd go about adding a column to the materialized ExecRow.

          I hope to attach a doc patch later today.

          Show
          Thomas Nielsen added a comment - It seems we all agree on ROW_NUMBER() OVER () being compliant - good The materialization path puts me in somewhat of a corner when I try to do connect-the-dots. I get a two column ExecRow from the materialized resultset, that is put into a three column ExecRow returned from OLAPResultSet.getNextRowCore(). That results in an NPE at a later stage. I had a look at HashJoins and ProjectRestrictNode which ought to do more or less the same, as well as the javadocs for various descriptors available in OLAPResultSet.getNextRowCore(), but it's not obvious (yet) how I'd go about adding a column to the materialized ExecRow. I hope to attach a doc patch later today.
          Hide
          Thomas Nielsen added a comment -

          Attached is a Reference manual doc update for ROW_NUMBER.

          I included info on the LIMIT syntax used in other databases, and added LIMIT as a dita keyword so those searching will find ROW_NUMBER() in the ref manual.

          Please do not commit until code patch is ready

          Show
          Thomas Nielsen added a comment - Attached is a Reference manual doc update for ROW_NUMBER. I included info on the LIMIT syntax used in other databases, and added LIMIT as a dita keyword so those searching will find ROW_NUMBER() in the ref manual. Please do not commit until code patch is ready
          Hide
          Thomas Nielsen added a comment -

          Note to self: RowUtil seems to have the kind of functions I need to extend the ExecRow. Need to figure out the column order for the extention, though.

          Show
          Thomas Nielsen added a comment - Note to self: RowUtil seems to have the kind of functions I need to extend the ExecRow. Need to figure out the column order for the extention, though.
          Hide
          Thomas Nielsen added a comment -

          I have reworked my patch.

          Attaching 'd2998-8.diff' and 'd2998-8.stat'.

          During parsing (sqlgrammar.jj) we introduce the new RowNumberColumnNode and a WindowNode holding the specification of the window in the AST. The WindowNode does not do much as of today, but is there for future window functions and/or window specification to use.

          Once the best plan has been found during optimization, the last step in the current trunk code is to swap SelectNodes with ProjectRestrictNodes, and top the PRNs off with GroupByNode, DistinctNode, and a OrderByNode if required.

          At the generate PRN stage, and with the current 'd2998-8' patch, we pull the WindowNode up from under the window function node, and place it on top of the existing (sub)tree, ending up with the following tree for code generation:

          PRN

          WindowNode

          OrderByNode

          DistictNode

          GroupByNode

          <PRN on top of anything below us, restricted on any where, and projected>

          This ensures the WindowNodeResultSet is generated to pull rows and evaluate the window function over the correct lower restricted, projected, grouped and ordered ResultSets.

          The attached patch 'd2998-8.diff' work for selects and nested selects, and Knuts join posted further up.

          The attached patch does not, however, work with group by, ditinct and/or order by.

          The reason is that the ResultColumn object for the window function column is added at execution time by WindowResultSet. The PRN topping off the SelectNode from list (typically a PRN on top of a FromBaseTable for a simple select) currently has a 'null' (no object) for the window function ResultColumn. This in turn cause the sorters to object and thorw exceptions, as they find a null ResultColumn.

          Any suggestions as to how to solve this are very welcome!

          At the moment I see three solutions:

          • introduce an non-null but "empty" or "phantom" ResultColumn object in the bottom PRN, and replace it with the correct value once we get to the WindowResultSet?
          • adjust the bottom PRN, and the orderby, groupby and having ResultColumns to only have the ResultColumns of the underlying ResultSet? Today they contain a reference to, and thereby sort on, all ResultColumns, including any window function ResultColumns.
          • adjust the sorters to allow null values?

          Changing the sorters sounds dangerous . I don't feel truly comfortable with introducing phantom values either, even for a very short while. So maybe the second option is the best?

          Show
          Thomas Nielsen added a comment - I have reworked my patch. Attaching 'd2998-8.diff' and 'd2998-8.stat'. During parsing (sqlgrammar.jj) we introduce the new RowNumberColumnNode and a WindowNode holding the specification of the window in the AST. The WindowNode does not do much as of today, but is there for future window functions and/or window specification to use. Once the best plan has been found during optimization, the last step in the current trunk code is to swap SelectNodes with ProjectRestrictNodes, and top the PRNs off with GroupByNode, DistinctNode, and a OrderByNode if required. At the generate PRN stage, and with the current 'd2998-8' patch, we pull the WindowNode up from under the window function node, and place it on top of the existing (sub)tree, ending up with the following tree for code generation: PRN WindowNode OrderByNode DistictNode GroupByNode <PRN on top of anything below us, restricted on any where, and projected> This ensures the WindowNodeResultSet is generated to pull rows and evaluate the window function over the correct lower restricted, projected, grouped and ordered ResultSets. The attached patch 'd2998-8.diff' work for selects and nested selects, and Knuts join posted further up. The attached patch does not, however, work with group by, ditinct and/or order by. The reason is that the ResultColumn object for the window function column is added at execution time by WindowResultSet. The PRN topping off the SelectNode from list (typically a PRN on top of a FromBaseTable for a simple select) currently has a 'null' (no object) for the window function ResultColumn. This in turn cause the sorters to object and thorw exceptions, as they find a null ResultColumn. Any suggestions as to how to solve this are very welcome! At the moment I see three solutions: introduce an non-null but "empty" or "phantom" ResultColumn object in the bottom PRN, and replace it with the correct value once we get to the WindowResultSet? adjust the bottom PRN, and the orderby, groupby and having ResultColumns to only have the ResultColumns of the underlying ResultSet? Today they contain a reference to, and thereby sort on, all ResultColumns, including any window function ResultColumns. adjust the sorters to allow null values? Changing the sorters sounds dangerous . I don't feel truly comfortable with introducing phantom values either, even for a very short while. So maybe the second option is the best?
          Hide
          Thomas Nielsen added a comment -

          Reattached 'd2998-8.diff'

          Show
          Thomas Nielsen added a comment - Reattached 'd2998-8.diff'
          Hide
          Rick Hillegas added a comment -

          Thanks for the explanation of the problem, Thomas. I am still a bit puzzled, however. You have sketched the shape of the BestPlan produced by the optimizer, as seen before generate() is run on the plan. Could you show us the ExecutablePlan as well?

          Off the top of my head, at execution time, the sort ought to be happening beneath the WindowResultSet in the ExecutablePlan. It, however, sounds as though the row_number column has been pushed down into the ResultSet that the sorter is operating on. That sounds wrong to me. The row_number column should only appear in the top ResultSet generated from the top level PRN in the plan you sketched.

          Thanks,
          -Rick

          Show
          Rick Hillegas added a comment - Thanks for the explanation of the problem, Thomas. I am still a bit puzzled, however. You have sketched the shape of the BestPlan produced by the optimizer, as seen before generate() is run on the plan. Could you show us the ExecutablePlan as well? Off the top of my head, at execution time, the sort ought to be happening beneath the WindowResultSet in the ExecutablePlan. It, however, sounds as though the row_number column has been pushed down into the ResultSet that the sorter is operating on. That sounds wrong to me. The row_number column should only appear in the top ResultSet generated from the top level PRN in the plan you sketched. Thanks, -Rick
          Hide
          Thomas Nielsen added a comment -

          Attaching minor updates according to Ricks comment in d2998-9.diff.

          The query/executable plan is attached in d2998-9-derby.log, with d2998-9.diff applied, executing:
          select * from (select row_number() over () as r, t.* from t) as tr;

          The 9 patch attached does not evaluate WHERE clauses on expression columns in the materialized subquery resultset correctly.

          Show
          Thomas Nielsen added a comment - Attaching minor updates according to Ricks comment in d2998-9.diff. The query/executable plan is attached in d2998-9-derby.log, with d2998-9.diff applied, executing: select * from (select row_number() over () as r, t.* from t) as tr; The 9 patch attached does not evaluate WHERE clauses on expression columns in the materialized subquery resultset correctly.
          Hide
          Thomas Nielsen added a comment -

          > The 9 patch attached does not evaluate WHERE clauses on expression columns in the materialized subquery resultset correctly.

          The predicate of the outer select is flagged as pushable, and ends up being pushed into the subquery for evaluation. This is wrong for window functions, and possibly also for any where clause over a materialized resultset.

          Show
          Thomas Nielsen added a comment - > The 9 patch attached does not evaluate WHERE clauses on expression columns in the materialized subquery resultset correctly. The predicate of the outer select is flagged as pushable, and ends up being pushed into the subquery for evaluation. This is wrong for window functions, and possibly also for any where clause over a materialized resultset.
          Hide
          Rick Hillegas added a comment -

          Hi Thomas,

          Thanks for the latest patch. I have applied it and can see that the following query runs correctly:

          select *
          from
          (
          select row_number() over () as r, t.* from t
          ) tout
          order by r;

          but the following query generates an error:

          select *
          from
          (
          select row_number() over () as r, t.* from t
          ) tout
          where r > 2;
          ERROR XJ001: Java exception: 'ASSERT FAILED sourceResultSetNumber expected to be >= 0 for null.R: org.apache.derby.shared.common.sanity.AssertFailure'.

          But, of course, you already know this. What kind of feedback do you want on this patch? Thanks.

          Show
          Rick Hillegas added a comment - Hi Thomas, Thanks for the latest patch. I have applied it and can see that the following query runs correctly: select * from ( select row_number() over () as r, t.* from t ) tout order by r; but the following query generates an error: select * from ( select row_number() over () as r, t.* from t ) tout where r > 2; ERROR XJ001: Java exception: 'ASSERT FAILED sourceResultSetNumber expected to be >= 0 for null.R: org.apache.derby.shared.common.sanity.AssertFailure'. But, of course, you already know this. What kind of feedback do you want on this patch? Thanks.
          Hide
          A B added a comment -

          Is there any kind of detailed write-up or functional spec for the approach that is being pursued in the patches thus far (esp. d2998-9.diff)? I took a look at:

          http://wiki.apache.org/db-derby/OLAPRowNumber

          but the "proposed changes" section on that page is rather abbreviated. In particular, a description of how the query tree will look at various points during compilation (esp. 1) when created, 2) after preprocessing, and 3) after "modification of access paths"-i.e. just before code generation) might be useful in trying to a) evaluate the approach, and b) offer feedback/suggestions on the various issues that are occurring. Also useful would be a simple example of the intended compilation and execution-time processing that should occur when ROW_NUMBER() is in play.

          Apologies if the write-up already exists and I just missed it...

          One quick comment from scanning d2998-9.diff: Can you explain the purpose of the following change in SelectNode.java?

          /* Intentionally hide this.resultColumns */
          ResultColumnList resultColumns = this.resultColumns.copyListAndObjects();

          It appears to cause any RCL expression referenced by an outer query to fail with the same error as what Rick reported for ROW_ORDER(), ex:

          ij> select * from (select i+j as x from hmm) a where x > 0;
          ERROR XJ001: Java exception: 'ASSERT FAILED sourceResultSetNumber
          expected to be >= 0 for null.X

          But that same statement succeeds if you remove the above lines. Of course, if you remove those lines then the simple ROW_ORDER() queries that work with d2998-9.diff applied stop working, so something is amiss somewhere. A writeup of what the intended behavior/query tree structure is meant to be here might help...

          Show
          A B added a comment - Is there any kind of detailed write-up or functional spec for the approach that is being pursued in the patches thus far (esp. d2998-9.diff)? I took a look at: http://wiki.apache.org/db-derby/OLAPRowNumber but the "proposed changes" section on that page is rather abbreviated. In particular, a description of how the query tree will look at various points during compilation (esp. 1) when created, 2) after preprocessing, and 3) after "modification of access paths"- i.e. just before code generation) might be useful in trying to a) evaluate the approach, and b) offer feedback/suggestions on the various issues that are occurring. Also useful would be a simple example of the intended compilation and execution-time processing that should occur when ROW_NUMBER() is in play. Apologies if the write-up already exists and I just missed it... One quick comment from scanning d2998-9.diff: Can you explain the purpose of the following change in SelectNode.java? /* Intentionally hide this.resultColumns */ ResultColumnList resultColumns = this.resultColumns.copyListAndObjects(); It appears to cause any RCL expression referenced by an outer query to fail with the same error as what Rick reported for ROW_ORDER(), ex: ij> select * from (select i+j as x from hmm) a where x > 0; ERROR XJ001: Java exception: 'ASSERT FAILED sourceResultSetNumber expected to be >= 0 for null.X But that same statement succeeds if you remove the above lines. Of course, if you remove those lines then the simple ROW_ORDER() queries that work with d2998-9.diff applied stop working, so something is amiss somewhere. A writeup of what the intended behavior/query tree structure is meant to be here might help...
          Hide
          Thomas Nielsen added a comment -

          No, you didn't miss the writeup Army. The wikipage is all there is, and I haven't kept it up to date :/ Mostly because I've been feeling my way forward with this while trying to understand the inner workings of Derby. I'll try to make up for it now and do a proper writeup for the proposed changes.

          Attached is an updated patch d2998-10.diff.
          Still have some ordering issues, but where clauses and multiple row_number() columns are evaluated properly:
          ij>
          select * from t;
          A |B
          -----------------------
          10 |100
          20 |200
          30 |300
          40 |400
          50 |500

          5 rows selected
          ij>
          select row_number() over (), tr.* from (select row_number() over () as r, t.* from t) as tr where r > 2;
          row_number() |R |A |B
          -----------------------------------------------------------------
          1 |3 |30 |300
          2 |4 |40 |400
          3 |5 |50 |500

          3 rows selected
          ij>

          Show
          Thomas Nielsen added a comment - No, you didn't miss the writeup Army. The wikipage is all there is, and I haven't kept it up to date :/ Mostly because I've been feeling my way forward with this while trying to understand the inner workings of Derby. I'll try to make up for it now and do a proper writeup for the proposed changes. Attached is an updated patch d2998-10.diff. Still have some ordering issues, but where clauses and multiple row_number() columns are evaluated properly: ij> select * from t; A |B ----------------------- 10 |100 20 |200 30 |300 40 |400 50 |500 5 rows selected ij> select row_number() over (), tr.* from (select row_number() over () as r, t.* from t) as tr where r > 2; row_number() |R |A |B ----------------------------------------------------------------- 1 |3 |30 |300 2 |4 |40 |400 3 |5 |50 |500 3 rows selected ij>
          Hide
          Thomas Nielsen added a comment -

          I've updated the wikipage with a more thorough writeup. It includes visual examples of the AST and queryplan, as well as some details on the path from SQL parsing to code execution.

          Show
          Thomas Nielsen added a comment - I've updated the wikipage with a more thorough writeup. It includes visual examples of the AST and queryplan, as well as some details on the path from SQL parsing to code execution.
          Hide
          Thomas Nielsen added a comment -

          Fixed the issue with the sorter and ORDER BY clauses in d2998-11.diff. The patch still needs some polishing, but it works for most basic queries now.

          Show
          Thomas Nielsen added a comment - Fixed the issue with the sorter and ORDER BY clauses in d2998-11.diff. The patch still needs some polishing, but it works for most basic queries now.
          Hide
          A B added a comment -

          Thank you for updating the wiki page with more details, Thomas. A couple of comments that come to mind from a quick read of that page (without having examined the code changes themselves in detail):

          1. Near the top of the wiki page there is a sentence saying:

          "In this example the ROW_NUMBER function is used to limit the
          query as soon as the first N rows have been determined".

          But in the details that you've added to the wiki, you note the following:

          "For the nested select query above we materialize the subquery
          select result, and have the outer SELECT pull rows from the
          materialized result".

          From a quick reading of your writeup, it seems like materialization
          of the subquery might be counter-productive? That is, if I have the
          following query:

          SELECT * FROM (
          SELECT row_number() over () as r, t.* FROM T
          ) AS tmp WHERE r <= 3;

          and table T has a thousand rows in it, the ideal would be to use the
          ROW_NUMBER() function to limit the scan on T so that we only fetch
          the first 3 rows from disk. But if we materialize the subquery, I
          think that means the nested SELECT will issue a full scan on table T,
          returning all 1000 rows, and we'll "materialize" those into memory.
          Then the ROW_NUMBER() function will simply extract out the first 3
          rows from the materialized result set. Is that an accurate description
          of what you mean by "materialized result", or am I misreading? If
          this is in fact what you are proposing, then can you explain a bit more
          about what the benefit of such materialization is? Am I right in
          thinking that this could negate potential performance gains that might
          otherwise come from ROW_NUMBER() as a "limiting" predicate?

          The wiki page does say:

          "window function results that span multiple rows, like a moving
          average, will benefit from materialization..."

          which may be true. But it seems like there may be better ways to
          deal with such functions than full materialization of the result set.
          In particular, the idea of a "sort observer", similar to what is used
          for GroupedAggregateResultSet, seems like it could potentially be
          useful for such a function? Which brings me to my next comment...

          2. Did you by chance look at GroupedAggregateResultSet and its surrounding
          code to see if the OLAP functions might extend that class? It's perhaps
          overkill for a ROW_NUMBER() function that only supports empty, unnamed
          windows. But the OLAP window specification in general seems to share some
          common concepts with GROUP BY processing, so I wonder if it'd be possible
          to put building blocks in place based on that similarity--ex. to create
          some kind of "WindowFunctionValue" class that extends GroupedAggregated-
          ResultSet, and then have the various OLAP functions in turn extend
          WindowFunctionValue as needed.

          I have no idea if/how that would actually work, but I thought I'd mention
          it to see if you've had an opportunity to look at that option? Maybe
          it's not worth it for the simple ROW_NUMBER() function that you're currently
          working on--feel free to say so

          3. Under "modification of access paths" there is the following:

          "We add one WindowNode for each window function column, and they are
          evaluated left to right in the result column list, with the right most column
          being the top WindowNode in the query plan."

          So if I have:

          SELECT
          row_number() over () as r1,
          row_number() over () as r2,
          t.col1
          FROM T

          Is it correct to say that my query tree will end up as:

          ProjectRestrictNode
          ==> WindowNode (r2)
          ==> WindowNode (r1)
          ==> ProjectRestrictNode
          ==> FromBaseTable

          and my execution tree will look like:

          ProjectRestrictResultSet
          ==> WindowResultSet (r2)
          ==> WindowResultSet (r1)
          ==> ProjectRestrictResultSet
          ==> TableScanResultSet

          I guess this isn't really a comment, just a quick check to make
          sure I'm understanding what is supposed to happen

          Show
          A B added a comment - Thank you for updating the wiki page with more details, Thomas. A couple of comments that come to mind from a quick read of that page (without having examined the code changes themselves in detail): 1. Near the top of the wiki page there is a sentence saying: "In this example the ROW_NUMBER function is used to limit the query as soon as the first N rows have been determined". But in the details that you've added to the wiki, you note the following: "For the nested select query above we materialize the subquery select result, and have the outer SELECT pull rows from the materialized result". From a quick reading of your writeup, it seems like materialization of the subquery might be counter-productive? That is, if I have the following query: SELECT * FROM ( SELECT row_number() over () as r, t.* FROM T ) AS tmp WHERE r <= 3; and table T has a thousand rows in it, the ideal would be to use the ROW_NUMBER() function to limit the scan on T so that we only fetch the first 3 rows from disk. But if we materialize the subquery, I think that means the nested SELECT will issue a full scan on table T, returning all 1000 rows, and we'll "materialize" those into memory. Then the ROW_NUMBER() function will simply extract out the first 3 rows from the materialized result set. Is that an accurate description of what you mean by "materialized result", or am I misreading? If this is in fact what you are proposing, then can you explain a bit more about what the benefit of such materialization is? Am I right in thinking that this could negate potential performance gains that might otherwise come from ROW_NUMBER() as a "limiting" predicate? The wiki page does say: "window function results that span multiple rows, like a moving average, will benefit from materialization..." which may be true. But it seems like there may be better ways to deal with such functions than full materialization of the result set. In particular, the idea of a "sort observer", similar to what is used for GroupedAggregateResultSet, seems like it could potentially be useful for such a function? Which brings me to my next comment... 2. Did you by chance look at GroupedAggregateResultSet and its surrounding code to see if the OLAP functions might extend that class? It's perhaps overkill for a ROW_NUMBER() function that only supports empty, unnamed windows. But the OLAP window specification in general seems to share some common concepts with GROUP BY processing, so I wonder if it'd be possible to put building blocks in place based on that similarity--ex. to create some kind of "WindowFunctionValue" class that extends GroupedAggregated- ResultSet, and then have the various OLAP functions in turn extend WindowFunctionValue as needed. I have no idea if/how that would actually work, but I thought I'd mention it to see if you've had an opportunity to look at that option? Maybe it's not worth it for the simple ROW_NUMBER() function that you're currently working on--feel free to say so 3. Under "modification of access paths" there is the following: "We add one WindowNode for each window function column, and they are evaluated left to right in the result column list, with the right most column being the top WindowNode in the query plan." So if I have: SELECT row_number() over () as r1, row_number() over () as r2, t.col1 FROM T Is it correct to say that my query tree will end up as: ProjectRestrictNode ==> WindowNode (r2) ==> WindowNode (r1) ==> ProjectRestrictNode ==> FromBaseTable and my execution tree will look like: ProjectRestrictResultSet ==> WindowResultSet (r2) ==> WindowResultSet (r1) ==> ProjectRestrictResultSet ==> TableScanResultSet I guess this isn't really a comment, just a quick check to make sure I'm understanding what is supposed to happen
          Hide
          Thomas Nielsen added a comment -

          Thanks for pitching in Army.

          Ad 1)
          Good question. This probably needs a little more explaining

          The MaterializedResultSet does not materialize the complete lower result before pulling rows from it to the upper result. A row is generated, stored (i.e materialized), and then passed up the chain. Then the next row is materialized, and so on. This means you have only materialized those rows you have actually visited, so the optimization we seek will still happen. The query, and with that the row materialization, is stopped once the condition is met. The more involved your subquery is, the better effect materialization (or caching) will give.

          With that said I don't really see the need for materializing subqueries for ROW_NUMBER(). I have turned off materialization in the latest patches. But for other functions it may be valid, so materialization or not should probably become dependant on what function is evaluated.

          Ad 2)
          I did have a look at the GroupedAggregateResultSet at a very early stage, but I should probably revisit that now.

          Ad 3)
          Yes, that is basically how the trees will look like.

          It might be worth looking into generating all window functions belonging to a given RCL in the same WindowNode and/or WindowResultSet, but I won't dive into that now. Doing that is still only valid if the functions operate on the exact same window definition - however (un)likely that is, and adding handling of multiple window definitions to WindowNode and/or WindowResultSet will probably introduce some unnecessary complexity. The extra cost of passing the row through an additional ResultSet in the chain is pretty small, so I'm not yet convinced it's worth doing.

          Show
          Thomas Nielsen added a comment - Thanks for pitching in Army. Ad 1) Good question. This probably needs a little more explaining The MaterializedResultSet does not materialize the complete lower result before pulling rows from it to the upper result. A row is generated, stored (i.e materialized), and then passed up the chain. Then the next row is materialized, and so on. This means you have only materialized those rows you have actually visited, so the optimization we seek will still happen. The query, and with that the row materialization, is stopped once the condition is met. The more involved your subquery is, the better effect materialization (or caching) will give. With that said I don't really see the need for materializing subqueries for ROW_NUMBER(). I have turned off materialization in the latest patches. But for other functions it may be valid, so materialization or not should probably become dependant on what function is evaluated. Ad 2) I did have a look at the GroupedAggregateResultSet at a very early stage, but I should probably revisit that now. Ad 3) Yes, that is basically how the trees will look like. It might be worth looking into generating all window functions belonging to a given RCL in the same WindowNode and/or WindowResultSet, but I won't dive into that now. Doing that is still only valid if the functions operate on the exact same window definition - however (un)likely that is, and adding handling of multiple window definitions to WindowNode and/or WindowResultSet will probably introduce some unnecessary complexity. The extra cost of passing the row through an additional ResultSet in the chain is pretty small, so I'm not yet convinced it's worth doing.
          Hide
          A B added a comment -

          Thank you for the quick replies, Thomas.

          > The MaterializedResultSet does not materialize the complete lower result before
          > pulling rows from it to the upper result.

          Okay, thanks for setting me straight on that. I scanned the code in MaterializedResultSet and confirmed what you said, i.e. that rows are fetched one at a time and stored into a temporary conglomerate. It is only when (if) the scan is RE-opened again later that the temporary conglomerate is then used for subsequent scanning.

          > It might be worth looking into generating all window functions belonging to a given RCL in the
          > same WindowNode and/or WindowResultSet

          For what it's worth, I tend to like that approach better, as it seems (to me) to be a more intuitive way to handle multiple window functions in the same RCL.

          > Doing that is still only valid if the functions operate on the exact same window definition

          Would this be a requirement or just a preference? If you define some kind of WindowFunctionResult that takes "source" rows and orders/groups/filters them according to its own window definition, then would it not be possible for each instance of WindowFunctionResult to take the same "source" row and deal with it as necessary? I.e. the WindowResultSet would read the source row and then pass it down to each of its WindowFunctionResult columns, letting the latter handle the row as needed (discard it, sort it, group it, etc.). Not sure exactly how that might work, but it seems feasible...

          > adding handling of multiple window definitions to WindowNode and/or WindowResultSet will
          > probably introduce some unnecessary complexity

          It would certainly require more work to get your current code to support that, but from a high-level view, would it result in a more complex overall design/implementation? I don't know one way or the other, I'm just asking the question. And "I don't know because I haven't tried" would be a perfectly valid answer

          Show
          A B added a comment - Thank you for the quick replies, Thomas. > The MaterializedResultSet does not materialize the complete lower result before > pulling rows from it to the upper result. Okay, thanks for setting me straight on that. I scanned the code in MaterializedResultSet and confirmed what you said, i.e. that rows are fetched one at a time and stored into a temporary conglomerate. It is only when (if) the scan is RE-opened again later that the temporary conglomerate is then used for subsequent scanning. > It might be worth looking into generating all window functions belonging to a given RCL in the > same WindowNode and/or WindowResultSet For what it's worth, I tend to like that approach better, as it seems (to me) to be a more intuitive way to handle multiple window functions in the same RCL. > Doing that is still only valid if the functions operate on the exact same window definition Would this be a requirement or just a preference? If you define some kind of WindowFunctionResult that takes "source" rows and orders/groups/filters them according to its own window definition, then would it not be possible for each instance of WindowFunctionResult to take the same "source" row and deal with it as necessary? I.e. the WindowResultSet would read the source row and then pass it down to each of its WindowFunctionResult columns, letting the latter handle the row as needed (discard it, sort it, group it, etc.). Not sure exactly how that might work, but it seems feasible... > adding handling of multiple window definitions to WindowNode and/or WindowResultSet will > probably introduce some unnecessary complexity It would certainly require more work to get your current code to support that, but from a high-level view, would it result in a more complex overall design/implementation? I don't know one way or the other, I'm just asking the question. And "I don't know because I haven't tried" would be a perfectly valid answer
          Hide
          A B added a comment -

          > d2998-11.diff. The patch still needs some polishing, but it works for most basic queries now.

          For what it's worth, I applied the patch and played around with some queries that include multiple ROW_NUMBER() functions in the same RCL. In doing so I noticed that the following queries all return incorrect results for the third column:

          create table t1 (i int, j int);
          insert into t1 values (1, 0), (1, -1), (2, -2), (3, -3), (4, -4);

          select row_number() over(), row_number() over(), i from t1;
          select row_number() over(), row_number() over(), 2*i from t1;
          select row_number() over(), row_number() over(), j from t1;
          select row_number() over(), row_number() over(), 'HMM' from t1;

          In all cases the third column returns the same valus as the ROW_NUMBER() columns--i.e. the results are:

          row_number() |row_number() |3
          ---------------------------------------------
          1 |1 |1
          2 |2 |2
          3 |3 |3
          4 |4 |4
          5 |5 |5

          5 rows selected

          So it looks like the third column is being ignored and/or replaced by a row_number() column.

          I also noticed that the following query fails with an ArrayIndexOutOfBoundsException:

          select row_number() over (), i from t1 order by j desc;

          while this one passes (order by "i" instead of "j"):

          select row_number() over (), i from t1 order by i desc;

          And the following fails with an ASSERT failure (this might just be DERBY-3310, but I thought I'd mention it):

          select * from
          (select row_number() over(), j from t1) x(a,b) where a > 2 order by a desc;

          Are these kind of issues ones about which you already know? If so, then I think I'll echo Rick's comment from a short while back: namely, what kind of feedback would you like on this patch?

          Show
          A B added a comment - > d2998-11.diff. The patch still needs some polishing, but it works for most basic queries now. For what it's worth, I applied the patch and played around with some queries that include multiple ROW_NUMBER() functions in the same RCL. In doing so I noticed that the following queries all return incorrect results for the third column: create table t1 (i int, j int); insert into t1 values (1, 0), (1, -1), (2, -2), (3, -3), (4, -4); select row_number() over(), row_number() over(), i from t1; select row_number() over(), row_number() over(), 2*i from t1; select row_number() over(), row_number() over(), j from t1; select row_number() over(), row_number() over(), 'HMM' from t1; In all cases the third column returns the same valus as the ROW_NUMBER() columns--i.e. the results are: row_number() |row_number() |3 --------------------------------------------- 1 |1 |1 2 |2 |2 3 |3 |3 4 |4 |4 5 |5 |5 5 rows selected So it looks like the third column is being ignored and/or replaced by a row_number() column. I also noticed that the following query fails with an ArrayIndexOutOfBoundsException: select row_number() over (), i from t1 order by j desc; while this one passes (order by "i" instead of "j"): select row_number() over (), i from t1 order by i desc; And the following fails with an ASSERT failure (this might just be DERBY-3310 , but I thought I'd mention it): select * from (select row_number() over(), j from t1) x(a,b) where a > 2 order by a desc; Are these kind of issues ones about which you already know? If so, then I think I'll echo Rick's comment from a short while back: namely, what kind of feedback would you like on this patch?
          Hide
          Thomas Nielsen added a comment -

          These are indeed the issues I'm looking into now.

          The incorrect results are caused by the source column mapping being messed up. I'm aware of the assert from the "where ... order by" query, but haven't had a chance to look closely at it yet. It's related to DERBY-3310, but rather (again) column mapping being off is causing us to map either an SQLInteger into a SQLLongint column or the other way around.

          I've uploaded the patches mainly so others can have a look if they're interested. I'm not really looking for any specific feedback - but any feedback is still appreciated. Thanks for taking the time

          Show
          Thomas Nielsen added a comment - These are indeed the issues I'm looking into now. The incorrect results are caused by the source column mapping being messed up. I'm aware of the assert from the "where ... order by" query, but haven't had a chance to look closely at it yet. It's related to DERBY-3310 , but rather (again) column mapping being off is causing us to map either an SQLInteger into a SQLLongint column or the other way around. I've uploaded the patches mainly so others can have a look if they're interested. I'm not really looking for any specific feedback - but any feedback is still appreciated. Thanks for taking the time
          Hide
          Thomas Nielsen added a comment -

          Attaching updates to the OLAPTest in 'd2998-test4.diff'. This is a collection of the different queries that have been reported to fail, as well as a couple of other verification queries.

          The test4 patch makes OLAPTest contain 13 queries expected to pass, and 3 expected to fail.

          Show
          Thomas Nielsen added a comment - Attaching updates to the OLAPTest in 'd2998-test4.diff'. This is a collection of the different queries that have been reported to fail, as well as a couple of other verification queries. The test4 patch makes OLAPTest contain 13 queries expected to pass, and 3 expected to fail.
          Hide
          Thomas Nielsen added a comment -

          Attaching 'd2998-12.diff' patch that fixes the problems also seen by Army.

          • queries ordered on a column left out of the projection
          • incorrect results due to incorrect column mapping
          • the assert due to incorrect column mapping

          The 16 queries in my previously attached test4 patch run successfully, and the lang/_Suite runs without failures.

          There's still a problem with the column mapping if you do multiple window functions in a single RCL.
          If you happen to play with this and see failing queries, please let me know.

          Show
          Thomas Nielsen added a comment - Attaching 'd2998-12.diff' patch that fixes the problems also seen by Army. queries ordered on a column left out of the projection incorrect results due to incorrect column mapping the assert due to incorrect column mapping The 16 queries in my previously attached test4 patch run successfully, and the lang/_Suite runs without failures. There's still a problem with the column mapping if you do multiple window functions in a single RCL. If you happen to play with this and see failing queries, please let me know.
          Hide
          Thomas Nielsen added a comment -

          Attaching patch d2998-13 that fixes multiple windowfunctions in a single RCL.
          Added 3 test queries with multiple window function columns to the test in d2998-test6.

          The extended test pass, and the lang/_Suite test is still running. I don't expect any problems there from the last increment to the patch.

          Before checking patch available I want to add an abstract superclass WindowFunctionColumnNode to RowNumberColumnNode.

          Show
          Thomas Nielsen added a comment - Attaching patch d2998-13 that fixes multiple windowfunctions in a single RCL. Added 3 test queries with multiple window function columns to the test in d2998-test6. The extended test pass, and the lang/_Suite test is still running. I don't expect any problems there from the last increment to the patch. Before checking patch available I want to add an abstract superclass WindowFunctionColumnNode to RowNumberColumnNode.
          Hide
          Thomas Nielsen added a comment -

          Tested more queries and it turns out the last patch I posted produces an assert from one of Armys queries above. Looking into this now.

          Show
          Thomas Nielsen added a comment - Tested more queries and it turns out the last patch I posted produces an assert from one of Armys queries above. Looking into this now.
          Hide
          Thomas Nielsen added a comment -

          The problem with the latest patch is due to the new code that changes the ResultColumnList column references into virtual column references.

          With this issue fixed I get incorrect results whenever there is a subquery involved. This again is because of how preprocessing is done. The outer SelectNodes FromList contains a FromSubquery with the inner SelectNode. The FromSubquery is rewritten during preprocessing to a ProjectRestictNode. This PRN gets the original ResultColumns (i.e before we optimize and modify the access paths and pull the WindowNodes into the tree and ResultColumList) In the original RCL the virtual column ids are not what the lower levels PRNs end up with after optimization:

          So, before preprocessing:
          SelectNode

          FromSubquery

          SelectNode

          ...

          During preprocessing this is rewritten to:
          SelectNode

          PRN <-- This PRN has original RCL

          SelectNode

          ...

          After optimization we have something like:
          PRN

          PRN <-- This PRN has original RCL, which now has wrong VirtualColumnID references

          PRN <-- This has correct RCL after optimization and pulling WindowNodes up

          WindowNode

          PRN

          ....

          Just need to figure out how to solve this. Might be possible to collapse the PRN as it seems to be a no-op, or pull the RCL up from the lower PRN after/during optimization if there are window columns involved?

          Show
          Thomas Nielsen added a comment - The problem with the latest patch is due to the new code that changes the ResultColumnList column references into virtual column references. With this issue fixed I get incorrect results whenever there is a subquery involved. This again is because of how preprocessing is done. The outer SelectNodes FromList contains a FromSubquery with the inner SelectNode. The FromSubquery is rewritten during preprocessing to a ProjectRestictNode. This PRN gets the original ResultColumns (i.e before we optimize and modify the access paths and pull the WindowNodes into the tree and ResultColumList) In the original RCL the virtual column ids are not what the lower levels PRNs end up with after optimization: So, before preprocessing: SelectNode FromSubquery SelectNode ... During preprocessing this is rewritten to: SelectNode PRN <-- This PRN has original RCL SelectNode ... After optimization we have something like: PRN PRN <-- This PRN has original RCL, which now has wrong VirtualColumnID references PRN <-- This has correct RCL after optimization and pulling WindowNodes up WindowNode PRN .... Just need to figure out how to solve this. Might be possible to collapse the PRN as it seems to be a no-op, or pull the RCL up from the lower PRN after/during optimization if there are window columns involved?
          Hide
          Thomas Nielsen added a comment -

          Attached patches (d2998-14 and test7) fixes the virtual column ids mentioned in my previous comment.

          Turned out the middle PRN (initially FromSubquery) and the lower SelectNode were actually using the same instance of the RCL, so when the lower SelectNode (its PRN to be precise) was updating its RCL the changes were also reflected in the upper PRN. This has been fixed in the 14 patch.

          The optimization now works as expected, but some of the resultset numbers are now missing in the middle PRNs RCL, and this cause code generation to complain for the last query in test7. Any input on how to solve this will be very welcome.

          Test7 reorders the queries in the test so that the two failing queries are run last. The other 19 or so runs fine.

          Show
          Thomas Nielsen added a comment - Attached patches (d2998-14 and test7) fixes the virtual column ids mentioned in my previous comment. Turned out the middle PRN (initially FromSubquery) and the lower SelectNode were actually using the same instance of the RCL, so when the lower SelectNode (its PRN to be precise) was updating its RCL the changes were also reflected in the upper PRN. This has been fixed in the 14 patch. The optimization now works as expected, but some of the resultset numbers are now missing in the middle PRNs RCL, and this cause code generation to complain for the last query in test7. Any input on how to solve this will be very welcome. Test7 reorders the queries in the test so that the two failing queries are run last. The other 19 or so runs fine.
          Hide
          Thomas Nielsen added a comment - - edited

          d2998-15 changes:

          • adds abstract class WindowFunctionColumnNode, which RowNumberColumnNode subclasses. This makes any instanceof-checks in other parts of code generic. Common code moved from RNCN to WFCN.
          • simplified pulling WindowNodes into the querytree in SelectNode.genProjectRestrictNode.

          The 15 patch still has the resultset numbering problem in the RCLof the PRN that was originally a FromSubquery.

          Show
          Thomas Nielsen added a comment - - edited d2998-15 changes: adds abstract class WindowFunctionColumnNode, which RowNumberColumnNode subclasses. This makes any instanceof-checks in other parts of code generic. Common code moved from RNCN to WFCN. simplified pulling WindowNodes into the querytree in SelectNode.genProjectRestrictNode. The 15 patch still has the resultset numbering problem in the RCLof the PRN that was originally a FromSubquery.
          Hide
          A B added a comment -

          > some of the resultset numbers are now missing in the middle PRNs RCL, and this
          > cause code generation to complain for the last query in test7. Any input on how to
          > solve this will be very welcome.

          I think the file "test7.diff" as attached is a "stat" file instead of a "diff" file...can you re-attach it?

          Show
          A B added a comment - > some of the resultset numbers are now missing in the middle PRNs RCL, and this > cause code generation to complain for the last query in test7. Any input on how to > solve this will be very welcome. I think the file "test7.diff" as attached is a "stat" file instead of a "diff" file...can you re-attach it?
          Hide
          Thomas Nielsen added a comment -

          Reattaching d2998-test7.diff. Sorry about that :|

          Show
          Thomas Nielsen added a comment - Reattaching d2998-test7.diff. Sorry about that :|
          Hide
          A B added a comment -

          Hi Thomas, thanks for your continued work on this.

          > Sorry about that :|

          Not a problem

          I haven't had a chance to look at the failing queries that you refer to in test7, but I did take a look at patch 15. Some comments/questions after my first run through are below.

          1) There are a lot of places with logic similar to the following:

          ResultColumn rc = (ResultColumn) resultColumns.elementAt(index);
          if (rc.getExpression() instanceof WindowFunctionColumnNode)

          { ... }

          Since WindowFunctionColumnNode extends ResultColumn, I wonder if it would be worth it to add a new "isWindowColumn()" method (or something like that) to ResultColumn.java and let that method contain all of the necessary logic for finding a WindowFunctionColumnNode. If that was done then the above type of "if" expression becomes simply:

          ResultColumn rc = (ResultColumn) resultColumns.elementAt(index);
          if (rc.isWindowColumn())

          { ... }

          which makes it easier to extend the check for "window columns" in the future if needed. For example, would it ever be necessary to walk further down the ResultColumn's expression chain to look for a RowNumber column? That is, could we ever have:

          ResultColumn
          -> VirtualColumnNode
          -> ResultColumn
          -> ...
          -> RowNumberColumnNode

          If so, then the "isWindowColumn()" as defined in ResultColumn.java could include the necessary logic to walk the chain, which would simplify the calling code. See, for example, comments #5 and #6 below.

          2)

          In WindowNode.bind() there is the following:

          /* At this stage there is hopefully a PRN below us as the fromList */
          return this;

          It might be good to add an ASSERT here to make sure that the 'hopefully' condition is satisified...

          3)

          Many of the fields in WindowNode (and other new classes added by this patch) are "public", but some of them are never referenced outside of WindowNode. Unless a "public" declaration is functionally necessary, it seems like it would be safer to make them private and then supply the necessary getter/setter methods where needed, to avoid accidental modification.

          4)

          In WindowNode.java there is a field "level" which is initialized to -1 and then is set from SelectNode.java. But as far as I can tell there are no comments explaining what that field does. I think I was able to figure it out from the code, but it'd be nice to have explanatory comments within WindowNode itself. And per comment #3 above, I think it'd be nice to use a setter method in SelectNode instead of direct assignment to the public field.

          5)

          In ColumnReference.java there are two different kinds of checks to see if the ColumnReference is pointing to a WindowColumnNode. The first is via the method "pointsToWindowFunction()", which does an instanceof check on the source ResultColumn's expression and returns true if it is a WindowFunctionColumnNode. The second is in the categorize() method, where the logic only evaluates to true if the source ResultColumn's expression is a VirtualColumnNode whose source ResultColumn's expression is a WindowFunctionColumnNode. So the checks do not appear to be equivalent. Is that intentional? If so, then explanatory comments might be good here. Also, I believe this is a good example of the kind of code that might benefit from an "isWindowColumn()" function on ResultColumn, per comment #1 above.

          6)

          In ResultColumnList.java, there is the following:

          /* Window function columns are added by the WindowResultSet */
          if (sourceExpr instanceof WindowFunctionColumnNode ||
          (sourceExpr instanceof VirtualColumnNode &&
          (sourceExpr.getSourceResultColumn().getExpression()
          instanceof WindowFunctionCumnNode)))

          { continue; }

          See comment #1 above for an alternate (cleaner?) approach...

          7)

          The result set statistics node for WindowResultSet does not currently print out the name of the result set...(I don't think). This is pretty minor, but I was briefly confused when I saw the RealWindowResultSetStatistics class and yet the query plans weren't showing anything "window" releated.

          8)

          If I'm understanding correctly, the execution-time logic for WindowResultSet operates in a fashion similar to ProjectRestrictResultSet. That is, it iterates through all of its source's rows and, for each one, it applies a restriction. If the row fails the restriction, then the iteration moves on to the next source row, and so on. This agrees with the following comments, pulled from the getNextRowCore() method of WindowResultSet:

          /*

          • Loop until we get a row from the source that qualifies, or there are
          • no more rows to qualify. For each iteration fetch a row from the
          • source, and evaluate against the restriction if any.
            */

          If this is correct, then I'm not entirely sure we'll get the performance boost that one might perhaps expect from a query like:

          SELECT * FROM (
          SELECT row_number() over () as r, t.* FROM T
          ) AS tmp WHERE r <= 3;

          I made a similar comment on Februrary 5th that was based on materialization of the source result set, and that comment ended up being wrong because I misunderstood materialization. But in looking at this query again, I think something might still be slightly sub-optimal here.

          If we take the approach of "loop until we get a row that qualifies", then we will loop through all of the rows in table T, even though only the first 3 satisfy the restriction. In the end we will correctly return the first 3 rows (and only the first 3 rows) but not before we've read all of the others from disk and applied the "<= 3" restriction to each one. This can be observed by either a) adding debug printouts to WindowResultSet.getNextRowCore() for every call to "source.getNextRowCore()", or b) looking at the query plan. For the above query, the plan shows the following for the top-most ProjectRestrictResultSet:

                      • Project-Restrict ResultSet (1):
                        Number of opens = 1
                        Rows seen = 1280
                        Rows filtered = 1277
                        restriction = true

          So we actually read all 1280 rows from disk, then filtered 1277 of them out so that, in the end, we only returned 3 rows. Thus from a performance perspective the ROW_NUMBER() function does not appear to improve things. Is that a known limitation of the current development (which, for the record, would be perfectly acceptable)?

          Having said that, there is absolutely NO need for you to boost performance before committing the changes Incremental development is good and it's great to have the functionality working. Performance improvements can always be addressed later, if you (or anyone else) so chooses. I'm just raising the issue to make sure that I understand how things are working.

          9)

          For the query mentioned in comment #8 above, the query plan shows:

          ProjectRestrictResultSet (sees ALL rows, restricts down to 3)
          -> ProjectRestrictResultSet (sees ALL rows)
          -> WindowResultSet (sees ALL rows)

          I intuitively expected the restriction to appear at the level of the WindowResultSet, as opposed to appearing at the level of the top-most PRN. The plan as shown above makes it look like the restriction is not being enforced by the Window ResultSet, but is instead being enforced by the ProjectRestrict. Is that correct? Or is this just a case where the query plan fails to capture what is really happening with the WindowResultSet? I'm guessing the latter, but am not sure...

          Show
          A B added a comment - Hi Thomas, thanks for your continued work on this. > Sorry about that :| Not a problem I haven't had a chance to look at the failing queries that you refer to in test7, but I did take a look at patch 15. Some comments/questions after my first run through are below. 1) There are a lot of places with logic similar to the following: ResultColumn rc = (ResultColumn) resultColumns.elementAt(index); if (rc.getExpression() instanceof WindowFunctionColumnNode) { ... } Since WindowFunctionColumnNode extends ResultColumn, I wonder if it would be worth it to add a new "isWindowColumn()" method (or something like that) to ResultColumn.java and let that method contain all of the necessary logic for finding a WindowFunctionColumnNode. If that was done then the above type of "if" expression becomes simply: ResultColumn rc = (ResultColumn) resultColumns.elementAt(index); if (rc.isWindowColumn()) { ... } which makes it easier to extend the check for "window columns" in the future if needed. For example, would it ever be necessary to walk further down the ResultColumn's expression chain to look for a RowNumber column? That is, could we ever have: ResultColumn -> VirtualColumnNode -> ResultColumn -> ... -> RowNumberColumnNode If so, then the "isWindowColumn()" as defined in ResultColumn.java could include the necessary logic to walk the chain, which would simplify the calling code. See, for example, comments #5 and #6 below. 2) In WindowNode.bind() there is the following: /* At this stage there is hopefully a PRN below us as the fromList */ return this; It might be good to add an ASSERT here to make sure that the 'hopefully' condition is satisified... 3) Many of the fields in WindowNode (and other new classes added by this patch) are "public", but some of them are never referenced outside of WindowNode. Unless a "public" declaration is functionally necessary, it seems like it would be safer to make them private and then supply the necessary getter/setter methods where needed, to avoid accidental modification. 4) In WindowNode.java there is a field "level" which is initialized to -1 and then is set from SelectNode.java. But as far as I can tell there are no comments explaining what that field does. I think I was able to figure it out from the code, but it'd be nice to have explanatory comments within WindowNode itself. And per comment #3 above, I think it'd be nice to use a setter method in SelectNode instead of direct assignment to the public field. 5) In ColumnReference.java there are two different kinds of checks to see if the ColumnReference is pointing to a WindowColumnNode. The first is via the method "pointsToWindowFunction()", which does an instanceof check on the source ResultColumn's expression and returns true if it is a WindowFunctionColumnNode. The second is in the categorize() method, where the logic only evaluates to true if the source ResultColumn's expression is a VirtualColumnNode whose source ResultColumn's expression is a WindowFunctionColumnNode. So the checks do not appear to be equivalent. Is that intentional? If so, then explanatory comments might be good here. Also, I believe this is a good example of the kind of code that might benefit from an "isWindowColumn()" function on ResultColumn, per comment #1 above. 6) In ResultColumnList.java, there is the following: /* Window function columns are added by the WindowResultSet */ if (sourceExpr instanceof WindowFunctionColumnNode || (sourceExpr instanceof VirtualColumnNode && (sourceExpr.getSourceResultColumn().getExpression() instanceof WindowFunctionCumnNode))) { continue; } See comment #1 above for an alternate (cleaner?) approach... 7) The result set statistics node for WindowResultSet does not currently print out the name of the result set...(I don't think). This is pretty minor, but I was briefly confused when I saw the RealWindowResultSetStatistics class and yet the query plans weren't showing anything "window" releated. 8) If I'm understanding correctly, the execution-time logic for WindowResultSet operates in a fashion similar to ProjectRestrictResultSet. That is, it iterates through all of its source's rows and, for each one, it applies a restriction. If the row fails the restriction, then the iteration moves on to the next source row, and so on. This agrees with the following comments, pulled from the getNextRowCore() method of WindowResultSet: /* Loop until we get a row from the source that qualifies, or there are no more rows to qualify. For each iteration fetch a row from the source, and evaluate against the restriction if any. */ If this is correct, then I'm not entirely sure we'll get the performance boost that one might perhaps expect from a query like: SELECT * FROM ( SELECT row_number() over () as r, t.* FROM T ) AS tmp WHERE r <= 3; I made a similar comment on Februrary 5th that was based on materialization of the source result set, and that comment ended up being wrong because I misunderstood materialization. But in looking at this query again, I think something might still be slightly sub-optimal here. If we take the approach of "loop until we get a row that qualifies", then we will loop through all of the rows in table T, even though only the first 3 satisfy the restriction. In the end we will correctly return the first 3 rows (and only the first 3 rows) but not before we've read all of the others from disk and applied the "<= 3" restriction to each one. This can be observed by either a) adding debug printouts to WindowResultSet.getNextRowCore() for every call to "source.getNextRowCore()", or b) looking at the query plan. For the above query, the plan shows the following for the top-most ProjectRestrictResultSet: Project-Restrict ResultSet (1): Number of opens = 1 Rows seen = 1280 Rows filtered = 1277 restriction = true So we actually read all 1280 rows from disk, then filtered 1277 of them out so that, in the end, we only returned 3 rows. Thus from a performance perspective the ROW_NUMBER() function does not appear to improve things. Is that a known limitation of the current development (which, for the record, would be perfectly acceptable)? Having said that, there is absolutely NO need for you to boost performance before committing the changes Incremental development is good and it's great to have the functionality working. Performance improvements can always be addressed later, if you (or anyone else) so chooses. I'm just raising the issue to make sure that I understand how things are working. 9) For the query mentioned in comment #8 above, the query plan shows: ProjectRestrictResultSet (sees ALL rows, restricts down to 3) -> ProjectRestrictResultSet (sees ALL rows) -> WindowResultSet (sees ALL rows) I intuitively expected the restriction to appear at the level of the WindowResultSet, as opposed to appearing at the level of the top-most PRN. The plan as shown above makes it look like the restriction is not being enforced by the Window ResultSet, but is instead being enforced by the ProjectRestrict. Is that correct? Or is this just a case where the query plan fails to capture what is really happening with the WindowResultSet? I'm guessing the latter, but am not sure...
          Hide
          Thomas Nielsen added a comment -

          Thanks again for having a look Army!

          Attaching patch d2998-16.diff that fixes most of Army comments, plus a few other very minor issues. Testwise I get the same result with patch 16.

          > 1) .... I wonder if it would be worth it to add a new "isWindowColumn()" method

          Agree - makes the code more easily readable. Fixed in patch 16.

          Also added RC.isVirtualWindowColumn() for checking for a VirtualColumnNode pointing to a WindowFunctionColumnNode.

          > 2)
          > It might be good to add an ASSERT here to make sure that the 'hopefully' condition is satisified...

          The comment is not correct and should be removed. Fixed in patch 16 by rephrasing to reflect actual conditions.

          > 3)
          > Many of the fields in WindowNode (and other new classes added by this patch) are
          > "public", ...

          Fixed in patch 16

          > 4)
          > In WindowNode.java there is a field "level" which is initialized to -1 and then is set from
          > SelectNode.java.

          Patch 16 adds getters and setters, as well as comments on use of 'level'.

          I'm not too satisfied with the variable name 'level' but it was the best
          I could think of...

          > 5)
          > In ColumnReference.java there are two different kinds of checks to see if the
          > ColumnReference is pointing to a WindowColumnNode. ...

          Yes, the two different approaches are intentional. See updated comment for CR.pointsToWindowFunction().
          Fixed to use isWindowFunction() and isVirtualWindowFunction() in patch 16.

          > 6)
          > See comment #1 above for an alternate (cleaner?) approach...

          Patch 16 now uses isWindowFunction() and new isVirtualWindowFunction() for
          this if().

          > 7)
          > The result set statistics node for WindowResultSet does not currently print out the
          > name of the result set...(I don't think).

          I'll need to look into this.

          > 8)
          > If I'm understanding correctly, the execution-time logic for WindowResultSet operates
          > in a fashion similar to ProjectRestrictResultSet.

          True, but more like the 'inverse' of PRRS. The PRRS gets many columns
          from its child resultset, and projects a few of these. The WRS projects
          a few columns into the many destination columns, along with its
          windowfunction result.

          > That is, it iterates through all of its source's rows and, for each one, it applies a
          > restriction. If the row fails the restriction, then the iteration moves on to the next source
          > row, and so on. This agrees with the following comments, pulled from the
          > getNextRowCore() method of WindowResultSet:
          >
          > /*
          > * Loop until we get a row from the source that qualifies, or there are
          > * no more rows to qualify. For each iteration fetch a row from the
          > * source, and evaluate against the restriction if any.
          > */
          >

          What should happen is that the outer selects(S1) PRN calls getNextRow()
          that again calls into the child/subquerys(S2) PRN getNextRowCore(). That
          again calls into the chain of WindowResultSets and PRNs, and eventually
          ends up in a a BulkTableScanResultSet.getNextRowCore() or similar to
          actually fetch one row. This single row is then pulled up the
          resultset chain, and the restriction is evaluated at the level it's been
          pushed down to.

          In your example S1 evaluates the restriction. It's not
          pushed from S1 into S2 for evaluation as it's a restriction on a window function
          column of S1s subquery/child resultset.
          S1 knows that column r is a row number column and that it
          is ascending, so we should stop pulling rows from below once we reach
          the condition.

          See reply to #9 too.

          > If this is correct, then I'm not entirely sure we'll get the performance boost that one might perhaps expect from a query like:
          >
          > SELECT * FROM (
          > SELECT row_number() over () as r, t.* FROM T
          > ) AS tmp WHERE r <= 3;
          >
          > .... For the above query, the plan shows the following for the top-most
          > ProjectRestrictResultSet:
          >
          > ******* Project-Restrict ResultSet (1):
          > Number of opens = 1
          > Rows seen = 1280
          > Rows filtered = 1277
          > restriction = true
          >

          This looks wrong given my above explaination.
          It seems we don't take the fact that we know we have an ascending column
          into consideration any more (we used to). I'll have to have another look at this.

          I haven't focused on the store layer at all. But based on how many rows
          there are in your table the store layer may load either all or parts of
          your data from disk into its cache. But they should not be pulled up the
          resultset chain to either S2 or S1.

          > 9)
          > For the query mentioned in comment #8 above, the query plan shows:
          >
          > ProjectRestrictResultSet (sees ALL rows, restricts down to 3)
          > -> ProjectRestrictResultSet (sees ALL rows)
          > -> WindowResultSet (sees ALL rows)
          >
          > I intuitively expected the restriction to appear at the level of the WindowResultSet, as
          > opposed to appearing at the level of the top-most PRN. The plan as shown above
          > makes it look like the restriction is not being enforced by the Window ResultSet, but is
          > instead being enforced by the ProjectRestrict. Is that correct? Or is this just a case
          > where the query plan fails to capture what is really happening with the
          > WindowResultSet? I'm guessing the latter, but am not sure...

          This is (more or less) correct.

          As explained above, the top PRN must do the restriction, but it should
          never fetch all the rows.

          I briefly tried pushing the restriction down to the WindowResultSet some time ago, but the expression generation/execution code does not
          currently want to work with the window function columns.
          I did not pursue this further, but agree it should be looked at for a
          future optimization.

          However, the code needed to do restriction at the WindowResultSet level
          have been kept in its getNextRowCore(). This is the reason for the loop
          and comment mentioned in reply to #8.

          Show
          Thomas Nielsen added a comment - Thanks again for having a look Army! Attaching patch d2998-16.diff that fixes most of Army comments, plus a few other very minor issues. Testwise I get the same result with patch 16. > 1) .... I wonder if it would be worth it to add a new "isWindowColumn()" method Agree - makes the code more easily readable. Fixed in patch 16. Also added RC.isVirtualWindowColumn() for checking for a VirtualColumnNode pointing to a WindowFunctionColumnNode. > 2) > It might be good to add an ASSERT here to make sure that the 'hopefully' condition is satisified... The comment is not correct and should be removed. Fixed in patch 16 by rephrasing to reflect actual conditions. > 3) > Many of the fields in WindowNode (and other new classes added by this patch) are > "public", ... Fixed in patch 16 > 4) > In WindowNode.java there is a field "level" which is initialized to -1 and then is set from > SelectNode.java. Patch 16 adds getters and setters, as well as comments on use of 'level'. I'm not too satisfied with the variable name 'level' but it was the best I could think of... > 5) > In ColumnReference.java there are two different kinds of checks to see if the > ColumnReference is pointing to a WindowColumnNode. ... Yes, the two different approaches are intentional. See updated comment for CR.pointsToWindowFunction(). Fixed to use isWindowFunction() and isVirtualWindowFunction() in patch 16. > 6) > See comment #1 above for an alternate (cleaner?) approach... Patch 16 now uses isWindowFunction() and new isVirtualWindowFunction() for this if(). > 7) > The result set statistics node for WindowResultSet does not currently print out the > name of the result set...(I don't think). I'll need to look into this. > 8) > If I'm understanding correctly, the execution-time logic for WindowResultSet operates > in a fashion similar to ProjectRestrictResultSet. True, but more like the 'inverse' of PRRS. The PRRS gets many columns from its child resultset, and projects a few of these. The WRS projects a few columns into the many destination columns, along with its windowfunction result. > That is, it iterates through all of its source's rows and, for each one, it applies a > restriction. If the row fails the restriction, then the iteration moves on to the next source > row, and so on. This agrees with the following comments, pulled from the > getNextRowCore() method of WindowResultSet: > > /* > * Loop until we get a row from the source that qualifies, or there are > * no more rows to qualify. For each iteration fetch a row from the > * source, and evaluate against the restriction if any. > */ > What should happen is that the outer selects(S1) PRN calls getNextRow() that again calls into the child/subquerys(S2) PRN getNextRowCore(). That again calls into the chain of WindowResultSets and PRNs, and eventually ends up in a a BulkTableScanResultSet.getNextRowCore() or similar to actually fetch one row. This single row is then pulled up the resultset chain, and the restriction is evaluated at the level it's been pushed down to. In your example S1 evaluates the restriction. It's not pushed from S1 into S2 for evaluation as it's a restriction on a window function column of S1s subquery/child resultset. S1 knows that column r is a row number column and that it is ascending, so we should stop pulling rows from below once we reach the condition. See reply to #9 too. > If this is correct, then I'm not entirely sure we'll get the performance boost that one might perhaps expect from a query like: > > SELECT * FROM ( > SELECT row_number() over () as r, t.* FROM T > ) AS tmp WHERE r <= 3; > > .... For the above query, the plan shows the following for the top-most > ProjectRestrictResultSet: > > ******* Project-Restrict ResultSet (1): > Number of opens = 1 > Rows seen = 1280 > Rows filtered = 1277 > restriction = true > This looks wrong given my above explaination. It seems we don't take the fact that we know we have an ascending column into consideration any more (we used to). I'll have to have another look at this. I haven't focused on the store layer at all. But based on how many rows there are in your table the store layer may load either all or parts of your data from disk into its cache. But they should not be pulled up the resultset chain to either S2 or S1. > 9) > For the query mentioned in comment #8 above, the query plan shows: > > ProjectRestrictResultSet (sees ALL rows, restricts down to 3) > -> ProjectRestrictResultSet (sees ALL rows) > -> WindowResultSet (sees ALL rows) > > I intuitively expected the restriction to appear at the level of the WindowResultSet, as > opposed to appearing at the level of the top-most PRN. The plan as shown above > makes it look like the restriction is not being enforced by the Window ResultSet, but is > instead being enforced by the ProjectRestrict. Is that correct? Or is this just a case > where the query plan fails to capture what is really happening with the > WindowResultSet? I'm guessing the latter, but am not sure... This is (more or less) correct. As explained above, the top PRN must do the restriction, but it should never fetch all the rows. I briefly tried pushing the restriction down to the WindowResultSet some time ago, but the expression generation/execution code does not currently want to work with the window function columns. I did not pursue this further, but agree it should be looked at for a future optimization. However, the code needed to do restriction at the WindowResultSet level have been kept in its getNextRowCore(). This is the reason for the loop and comment mentioned in reply to #8.
          Hide
          Thomas Nielsen added a comment -

          The resultset number problem is a little strange. The following query runs fine:
          select * from (select row_number() over (), t.a from t) as tr;

          but flip the column order, and it fails:
          select * from (select t.a, row_number() over () from t) as tr;

          With both queries we run fine until we eventually end up in ResultColumnList.generateCore() in a call from ProjectRestrictNode.generateMinion() for the PRN that once was the FromSubquery (mentioned earlier). At this stage the RCL for both queries look like:

          RC (resultset# = 3)

          .expression=
          VCN
          .sourceColumn =
          RC (resultset# = -1)

          CR

          ... (into base table)

          However

          • in the working query, the correlate flag is false for the base table columns
          • in the failing query, the correlate flag is true for the base table columns

          This was the only obvious difference I could see, and the correlate flag is what causes the logic in RCL.generateCore() to stray off on its "wild path".
          Looks like I need to investigate why the correlation flag differs between the two.

          Show
          Thomas Nielsen added a comment - The resultset number problem is a little strange. The following query runs fine: select * from (select row_number() over (), t.a from t) as tr; but flip the column order, and it fails: select * from (select t.a, row_number() over () from t) as tr; With both queries we run fine until we eventually end up in ResultColumnList.generateCore() in a call from ProjectRestrictNode.generateMinion() for the PRN that once was the FromSubquery (mentioned earlier). At this stage the RCL for both queries look like: RC (resultset# = 3) .expression= VCN .sourceColumn = RC (resultset# = -1) CR ... (into base table) However in the working query, the correlate flag is false for the base table columns in the failing query, the correlate flag is true for the base table columns This was the only obvious difference I could see, and the correlate flag is what causes the logic in RCL.generateCore() to stray off on its "wild path". Looks like I need to investigate why the correlation flag differs between the two.
          Hide
          Thomas Nielsen added a comment - - edited

          The culprit turned out to be ResultColumnList.allExpressionsAreColumns().

          After adding an explict check for any window functions here, we avoid massing up the correlation flag and every thing seems to be working as expected. All 20-ish queries in the test7 patch returns the expected rows.

          I should probably add that I want to clean up a couple of issues with patch 17 before I check 'patch available', but I wanted to post the working code so you can play with it, and maybe give some additional feedback.

          Show
          Thomas Nielsen added a comment - - edited The culprit turned out to be ResultColumnList.allExpressionsAreColumns(). After adding an explict check for any window functions here, we avoid massing up the correlation flag and every thing seems to be working as expected. All 20-ish queries in the test7 patch returns the expected rows. I should probably add that I want to clean up a couple of issues with patch 17 before I check 'patch available', but I wanted to post the working code so you can play with it, and maybe give some additional feedback.
          Hide
          A B added a comment -

          I have not had a chance to look at patch 17 yet, but I just had some follow-up feedback based on your recent comments:

          > I'm not too satisfied with the variable name 'level' but it was the best I could think of...

          I think even something naive like "windowFunctionLevel" would be fine for now. I'd rather avoid the generic name "level" since there is already a "level" field that exists in FromTable.java, of which WindowNode is an indirect subclass (so if you're debugging, two different "level" fields show up).

          > That again calls into the chain of WindowResultSets and PRNs, and eventually
          > ends up in a a BulkTableScanResultSet.getNextRowCore() or similar to
          > actually fetch one row.

          If the intent is to fetch exactly one row, then would it be beneficial to disable bulk fetching on the base table? Otherwise, WindowResultSet may only want to fetch a single a row but, due to bulk fetching, we could end up with 16 (or whatever the bulk size is). Not a huge problem by any means, but a potential point for improvement if you haven't already dealt with it (maybe you have and I just missed it).

          > It seems we don't take the fact that we know we have an ascending column
          > into consideration any more (we used to). I'll have to have another look at this.

          Is this something that has been fixed with patch 17? Or is it still work in progress? (I haven't tried it out yet).

          Show
          A B added a comment - I have not had a chance to look at patch 17 yet, but I just had some follow-up feedback based on your recent comments: > I'm not too satisfied with the variable name 'level' but it was the best I could think of... I think even something naive like "windowFunctionLevel" would be fine for now. I'd rather avoid the generic name "level" since there is already a "level" field that exists in FromTable.java, of which WindowNode is an indirect subclass (so if you're debugging, two different "level" fields show up). > That again calls into the chain of WindowResultSets and PRNs, and eventually > ends up in a a BulkTableScanResultSet.getNextRowCore() or similar to > actually fetch one row. If the intent is to fetch exactly one row, then would it be beneficial to disable bulk fetching on the base table? Otherwise, WindowResultSet may only want to fetch a single a row but, due to bulk fetching, we could end up with 16 (or whatever the bulk size is). Not a huge problem by any means, but a potential point for improvement if you haven't already dealt with it (maybe you have and I just missed it). > It seems we don't take the fact that we know we have an ascending column > into consideration any more (we used to). I'll have to have another look at this. Is this something that has been fixed with patch 17? Or is it still work in progress? (I haven't tried it out yet).
          Hide
          A B added a comment -

          > Is this something that has been fixed with patch 17? Or is it still work in progress?

          Okay, I ran the same queries with patch 17 and can see for myself that this particular change has not yet been made.

          On patch 17 itself I have a few more comments:

          1) The patch adds the following to ResultColumnList:

          /*

          • To avoid messing up the correlation flags we should determine if any
          • of this RCLs columns are either:
          • - A windowfunction column
          • - A virtual column node pointing to window function column
            */

          I think it would be nice to expand a bit on why this code is necessary. The comments indicate that without the code we could "mess up the correlation flags", but it doesn't say how. At a minimum it would be good to have a pointer to the correlation code that is affected by this.

          2) Code in WindowNode.generate(...) ends with a check and a comment about positioned updates and deletes. Are there test cases to ensure all is okay with such operations?

          3) On more a general level, and especially with respect to comment # 5 in my Feb 25th post, it seems like the changes for this issue rely heavily on the expected appearance of a WindowFunctionColumnNode in the query tree. Namely, the fact that

          ResultColumn -> WindowFunctionColumn

          is expected in some places and

          ResultColumn -> VirtualColumnNode -> WindowFunctionColumn

          is expected in others makes me nervous. My experience with optimization/compilation is that VirtualColumnNodes, ResultColumns, and ColumnReferences are often stacked on top of each other in rather gratuitous fashion. Code which accesses these types of nodes is usually (or at least, often) written with the expectation that it could be dealing with VCN, ResultColumn, and/or ColRef chains of arbitrary length, and so must walk the chain when retrieving certain information.

          But the changes for ROW_NUMBER() take a different approach, one in which the assumption is that we will always have either 1) a ResultColumn whose child is a WindowFunctionColumn, or 2) a ResultColumn whose child is a VirtualColumnNode whose child is a WindowFunctionColumn. But how confident are we that this will always be the case?

          With this thought in my head I can't help but feel that queries which involve ROW_NUMBER() and which are larger or more complex are at a high risk for column "mis-information", which could cause issues. I applied patch 17 and tried writing some more complicated queries that use ROW_NUMBER(), and it wasn't long before I hit the ASSERT failure that we've seen before. Esp:

          – OK
          select row_number() over () from t1 union all select row_number() over () from t1;
          select 2 * r from (select row_number() over () from t1) x(r);

          – Fail.
          select count from (select row_number() over() from t1) x;
          select count from (select row_number() over () as r from t1) as t(r) where r <=3;

          – OK
          select c3, c1, c2 from
          (select i, j, row_number() over() as r from t1) x1 (c1, c2, r1),
          (select row_number() over() as r, j, i from t1) x2 (r2, c3, c4);

          – OK
          select c3, c1, c2 from
          (select i, j, row_number() over() as r from t1) x1 (c1, c2, r1),
          (select row_number() over() as r, j, i from t1) x2 (r2, c3, c4),
          t1;

          – OK
          select c3, c1, c2 from
          (select i, j, row_number() over() as r from t1) x1 (c1, c2, r1),
          (select row_number() over() as r, j, i from t1) x2 (r2, c3, c4),
          t1
          where x1.r1 = 2 * x2.r2;

          – Fails
          select c3, c1, c2 from
          (select i, j, row_number() over() as r from t1) x1 (c1, c2, r1),
          (select row_number() over() as r, j, i from t1) x2 (r2, c3, c4),
          t1
          where x2.c4 = t1.i;

          – Fails
          select c3, c1, c2 from
          (select i, j, row_number() over() as r from t1) x1 (c1, c2, r1),
          (select row_number() over() as r, j, i from t1) x2 (r2, c3, c4),
          t1
          where x1.r1 = 2 * x2.r2 and x2.c4 = t1.i;

          Perhaps these failures have nothing to do with my "fear" outlined above--in which case I apologize of the misdirection. But in any event, there are still some failures that I see with patch 17. I'm not sure to what extent these need to be resolved before code can be committed, as they (appear to) only occur when ROW_NUMBER() is in use and thus shouldn't affect existing apps. But I'm not the one to say for sure one way or the other...

          Show
          A B added a comment - > Is this something that has been fixed with patch 17? Or is it still work in progress? Okay, I ran the same queries with patch 17 and can see for myself that this particular change has not yet been made. On patch 17 itself I have a few more comments: 1) The patch adds the following to ResultColumnList: /* To avoid messing up the correlation flags we should determine if any of this RCLs columns are either: - A windowfunction column - A virtual column node pointing to window function column */ I think it would be nice to expand a bit on why this code is necessary. The comments indicate that without the code we could "mess up the correlation flags", but it doesn't say how. At a minimum it would be good to have a pointer to the correlation code that is affected by this. 2) Code in WindowNode.generate(...) ends with a check and a comment about positioned updates and deletes. Are there test cases to ensure all is okay with such operations? 3) On more a general level, and especially with respect to comment # 5 in my Feb 25th post, it seems like the changes for this issue rely heavily on the expected appearance of a WindowFunctionColumnNode in the query tree. Namely, the fact that ResultColumn -> WindowFunctionColumn is expected in some places and ResultColumn -> VirtualColumnNode -> WindowFunctionColumn is expected in others makes me nervous. My experience with optimization/compilation is that VirtualColumnNodes, ResultColumns, and ColumnReferences are often stacked on top of each other in rather gratuitous fashion. Code which accesses these types of nodes is usually (or at least, often) written with the expectation that it could be dealing with VCN, ResultColumn, and/or ColRef chains of arbitrary length, and so must walk the chain when retrieving certain information. But the changes for ROW_NUMBER() take a different approach, one in which the assumption is that we will always have either 1) a ResultColumn whose child is a WindowFunctionColumn, or 2) a ResultColumn whose child is a VirtualColumnNode whose child is a WindowFunctionColumn. But how confident are we that this will always be the case? With this thought in my head I can't help but feel that queries which involve ROW_NUMBER() and which are larger or more complex are at a high risk for column "mis-information", which could cause issues. I applied patch 17 and tried writing some more complicated queries that use ROW_NUMBER(), and it wasn't long before I hit the ASSERT failure that we've seen before. Esp: – OK select row_number() over () from t1 union all select row_number() over () from t1; select 2 * r from (select row_number() over () from t1) x(r); – Fail. select count from (select row_number() over() from t1) x; select count from (select row_number() over () as r from t1) as t(r) where r <=3; – OK select c3, c1, c2 from (select i, j, row_number() over() as r from t1) x1 (c1, c2, r1), (select row_number() over() as r, j, i from t1) x2 (r2, c3, c4); – OK select c3, c1, c2 from (select i, j, row_number() over() as r from t1) x1 (c1, c2, r1), (select row_number() over() as r, j, i from t1) x2 (r2, c3, c4), t1; – OK select c3, c1, c2 from (select i, j, row_number() over() as r from t1) x1 (c1, c2, r1), (select row_number() over() as r, j, i from t1) x2 (r2, c3, c4), t1 where x1.r1 = 2 * x2.r2; – Fails select c3, c1, c2 from (select i, j, row_number() over() as r from t1) x1 (c1, c2, r1), (select row_number() over() as r, j, i from t1) x2 (r2, c3, c4), t1 where x2.c4 = t1.i; – Fails select c3, c1, c2 from (select i, j, row_number() over() as r from t1) x1 (c1, c2, r1), (select row_number() over() as r, j, i from t1) x2 (r2, c3, c4), t1 where x1.r1 = 2 * x2.r2 and x2.c4 = t1.i; Perhaps these failures have nothing to do with my "fear" outlined above--in which case I apologize of the misdirection. But in any event, there are still some failures that I see with patch 17. I'm not sure to what extent these need to be resolved before code can be committed, as they (appear to) only occur when ROW_NUMBER() is in use and thus shouldn't affect existing apps. But I'm not the one to say for sure one way or the other...
          Hide
          Daniel John Debrunner added a comment -

          AB> ... with patch 17. I'm not sure to what extent these need to be resolved before code can be committed, as they (appear to) only occur when ROW_NUMBER() is in use and thus shouldn't affect existing apps. But I'm not the one to say for sure one way or the other...

          On the contrary, you are one of the ones to say if the patch should be committed. There is no one authority, just a set of committers. If any committer has confidence in the patch then she can commit it (and any other committer can veto it with a valid technical reason if she thinks it should not be committed).

          With a patch reaching its 17th iteration I think one has to consider if it should be committed before too much of the effort is spent keeping the patch up to date with a moving codeline.

          Is it better to keep trying to get a perfect patch (noting that the Derby project has ~450 open bugs so the base code is not perfect)?

          Or is it better to get the code committed in some basic working state (has tests, doesn't cause existing tests to fail) so that others can try it out and ongoing changes don't cause regressions in the functionality.

          And of course it's all done through svn, so reverting any patch is always an option.

          Show
          Daniel John Debrunner added a comment - AB> ... with patch 17. I'm not sure to what extent these need to be resolved before code can be committed, as they (appear to) only occur when ROW_NUMBER() is in use and thus shouldn't affect existing apps. But I'm not the one to say for sure one way or the other... On the contrary, you are one of the ones to say if the patch should be committed. There is no one authority, just a set of committers. If any committer has confidence in the patch then she can commit it (and any other committer can veto it with a valid technical reason if she thinks it should not be committed). With a patch reaching its 17th iteration I think one has to consider if it should be committed before too much of the effort is spent keeping the patch up to date with a moving codeline. Is it better to keep trying to get a perfect patch (noting that the Derby project has ~450 open bugs so the base code is not perfect)? Or is it better to get the code committed in some basic working state (has tests, doesn't cause existing tests to fail) so that others can try it out and ongoing changes don't cause regressions in the functionality. And of course it's all done through svn, so reverting any patch is always an option.
          Hide
          Thomas Nielsen added a comment -

          Attaching patch 18 and test8.

          To Armys comments:
          #1 - Patch 18 fixes the comment in allExpressionsAreColumns()
          #2 - No, there are no tests that check this. This is a relic from cloning IndexToBaseRowNode. It should probably be removed?
          #3 - Patch 18 relaxes the expectancy of finding RCL->WindowFunction, as well as generalizing the conditions throughout. This fixes the failing count() queries.

          The test8 patch add all additional queries pointed to by army to the OLAPTest. All but the two last queries posted by Army pass, but I haven't had a chance to look at why yet. It is most likely connected to comment #3 though.

          Army> My experience with optimization/compilation is that VirtualColumnNodes, ResultColumns, and
          Army> ColumnReferences are often stacked on top of each other in rather gratuitous fashion.

          That's something I've obviously not taken well enough into consideration. Do you think that the approach I've taken is fundamentally flawed, or is it just being too restrictive?

          Show
          Thomas Nielsen added a comment - Attaching patch 18 and test8. To Armys comments: #1 - Patch 18 fixes the comment in allExpressionsAreColumns() #2 - No, there are no tests that check this. This is a relic from cloning IndexToBaseRowNode. It should probably be removed? #3 - Patch 18 relaxes the expectancy of finding RCL->WindowFunction, as well as generalizing the conditions throughout. This fixes the failing count() queries. The test8 patch add all additional queries pointed to by army to the OLAPTest. All but the two last queries posted by Army pass, but I haven't had a chance to look at why yet. It is most likely connected to comment #3 though. Army> My experience with optimization/compilation is that VirtualColumnNodes, ResultColumns, and Army> ColumnReferences are often stacked on top of each other in rather gratuitous fashion. That's something I've obviously not taken well enough into consideration. Do you think that the approach I've taken is fundamentally flawed, or is it just being too restrictive?
          Hide
          A B added a comment -

          > Do you think that the approach I've taken is fundamentally flawed, or is it just being too restrictive?

          No, I do not think it's flawed. And I can't say with any certainty that it's too restrictive, either, as I haven't actually found any cases which prove that. Maybe the failing queries I posted above fall into that category, maybe not--I haven't done any detailed tracing. I thought I'd mention it as something which made me uneasy, but it could very well be fine as it is.

          > #3 - Patch 18 relaxes the expectancy of finding RCL->WindowFunction, as well as generalizing
          > the conditions throughout. This fixes the failing count() queries.

          I like the additional methods to distinguish between how/where to look for the WindowFunctionColumnNode. Now the trick is to figure out which methods to call where My suggestion is that whenever you call any of these methods, try to determine if a comment indicating why you chose the method you did would be useful to the reader. And that's especially true if you find yourself switching from one method to another at any point...

          Side note: looks like the new "isReferenceToOrWindowFunction()" for patch 18 is never actually called anywhere. Was that intentional?

          Show
          A B added a comment - > Do you think that the approach I've taken is fundamentally flawed, or is it just being too restrictive? No, I do not think it's flawed. And I can't say with any certainty that it's too restrictive, either, as I haven't actually found any cases which prove that. Maybe the failing queries I posted above fall into that category, maybe not--I haven't done any detailed tracing. I thought I'd mention it as something which made me uneasy, but it could very well be fine as it is. > #3 - Patch 18 relaxes the expectancy of finding RCL->WindowFunction, as well as generalizing > the conditions throughout. This fixes the failing count() queries. I like the additional methods to distinguish between how/where to look for the WindowFunctionColumnNode. Now the trick is to figure out which methods to call where My suggestion is that whenever you call any of these methods, try to determine if a comment indicating why you chose the method you did would be useful to the reader. And that's especially true if you find yourself switching from one method to another at any point... Side note: looks like the new "isReferenceToOrWindowFunction()" for patch 18 is never actually called anywhere. Was that intentional?
          Hide
          Thomas Nielsen added a comment -

          Thanks Army. Good to hear it's not all wrong

          > Side note: looks like the new "isReferenceToOrWindowFunction()" for patch 18 is never actually called anywhere. Was that intentional?
          Yes

          Show
          Thomas Nielsen added a comment - Thanks Army. Good to hear it's not all wrong > Side note: looks like the new "isReferenceToOrWindowFunction()" for patch 18 is never actually called anywhere. Was that intentional? Yes
          Hide
          Thomas Nielsen added a comment -

          I've started looking at why the last two queries fail, and they both run down a code path I haven't been down before - involving a couple of JoinNodes and some "trickery" with the RCL. The predicate t1.a is pulled into the RCL, like we do for PRNs under certain conditions with a where predicate, and this somehow cause problems during code generation. I'll investigate more tomorrow, but as of now it doesn't look like its yet another RCL->WindowFunction assumption gone bad. It's probably related though - if I exchange the WindowFunction for a constant it works fine.

          Show
          Thomas Nielsen added a comment - I've started looking at why the last two queries fail, and they both run down a code path I haven't been down before - involving a couple of JoinNodes and some "trickery" with the RCL. The predicate t1.a is pulled into the RCL, like we do for PRNs under certain conditions with a where predicate, and this somehow cause problems during code generation. I'll investigate more tomorrow, but as of now it doesn't look like its yet another RCL->WindowFunction assumption gone bad. It's probably related though - if I exchange the WindowFunction for a constant it works fine.
          Hide
          Thomas Nielsen added a comment -

          It seems from a debugging session that the two last join queries Army posted both fail due to failure to classify some of the PRNs as noops.

          If a PRN is classified a noop, the RCL nodes are maked as redundant, and during code generation we walk further down the RC-tree to lay out the code at a lower level. If not marked as redundant we pull the value from the current level result set. If I understand this correctly the last two queries end up trying to get the values from the current level when it should really look even further down. Manually changing the isRedundant flag in the debugger actually produces what seems to be the correct results.

          Show
          Thomas Nielsen added a comment - It seems from a debugging session that the two last join queries Army posted both fail due to failure to classify some of the PRNs as noops. If a PRN is classified a noop, the RCL nodes are maked as redundant, and during code generation we walk further down the RC-tree to lay out the code at a lower level. If not marked as redundant we pull the value from the current level result set. If I understand this correctly the last two queries end up trying to get the values from the current level when it should really look even further down. Manually changing the isRedundant flag in the debugger actually produces what seems to be the correct results.
          Hide
          A B added a comment -

          > the two last join queries both fail due to failure to classify some of the PRNs as noops.

          The code to check for "no-op" ProjectRestrictNodes depends, among other things, on object equality between two ResultColumn nodes. See ResultColumnList.nopProjection(). I think the manipulation that is done in SelectNode.genProjectRestrict() re-arranges the tree and creates new nodes such that the object equality fails. And even if the object equality wasn't an issue, I think re-arragement of the tree would cause the method to return false anyway. Which corresponds with your findings on "redundant" result column lists.

          I did some tracing for the queries that are still failing with patch 18. The failure occurs during code generation, when we try to generate the predicate "x2.c4 = t1.i" as a qualifier on table T1. When it comes time to generate the left operand, which points to a table that is BENEATH the WindowNode that was generated for the ROW_NUMBER(), we can't find the target result set number for the operand "X2.C4". In looking at the predicate contents at the point of failure, I could see that the target result set number was buried down inside of a ResultColumn-to-VirtualColumn chain rooted at X2.C4. Again, this matches your findings regarding "redundant" result columns.

          I then replaced ROW_NUMBER() with a simple expression ("i+j") and stopped at the same point; in this case the target result set number is readily available (i.e. it's not buried) and the query passes.

          On a whim, I then removed the following code from SelectNode:

          Code fragment "A":

          /*

          • The this.resultColumns object is shared with any PRNs above this
          • SelectNode in case this is a subquery. The following RCL
          • modifications cause problems in the column mapping of the upper
          • PRN unless we start off by making a copy for ourselves.
            */
            setResultColumns(resultColumns.copyListAndObjects());

          When I took that out, the target result set number became readily available for the join queries and the queries executed without error, which was good. Of course, the queries returned the wrong results, and several other ROW_NUMBER() queries which used to work started failing again. So I tried to figure out why the code was added in the first place, as the comments didn't quite make it clear for me.

          As far as I can tell, the scenario that prompted the addition of the above code is something like:

          select * from
          (select row_number() over(), 'HMM' from t1) x(a,c)
          where a > 2

          which leads to a (simplified) tree that looks like:

          ... PRN0(a,c) -> SELECT(row_number(), 'HMM') -> ...

          In this tree, PRN0's RCL is a different object than SELECT's RCL, and the columns in PRN0's RCL have VirtualColumnNodes that do not appear in SELECT's RCL. But underneath it all, both RCLs ultimately point to the same underlying ResultColumn objects. Thus any changes we make to the underlying ResultColumns in SELECT's RCL will be "shared" by PRN0, meaning that the ResultColumns beneath PRN0's RCL will see the changes, as well.

          From what I can gather, the changes that we are talking about here deal with virtual column ids. During SelectNode.genProjectRestrict() we remove the WindowFunctionColumns from SELECT's ResultColumnList and then adjust the virtual column id's accordingly. So we get:

          ... PRN0(a,c) -> SELECT('HMM') -> ...

          In the absence of code fragment "A", the result column for "HMM" now has a virtual column id of "1" (instead of "2"). The fact that we changed the virtual column id from "2" to "1" means that the result column beneath "C" in PRN0's ResultColumnList also now has a virtual column id of "1". That indicates that "C" should pull its value from the 1st column in its child's RCL. This is where things start to go wrong...

          When we finish with SelectNode.genProjectRestrict(), our simplified tree looks like:

          PRN0(a,c)
          -> PRNDUMMY(row_number(), 'HMM')
          -> WindowNode(row_number(), 'HMM')
          -> PRN1('HMM') // this now has virtual column id of "1"
          -> ...

          Note that PRNDUMMY will be a "no-op" project restrict and thus will not actually be generated. So PRN0's child will end up being WindowNode, which has TWO columns. But PRN0.C still has a virtual column id of "1", meaning that it should pull its value from the first column of PRN0's child. PRN0's child is WindowNode, and the first column in WindowNode is row_number()--not 'HMM'. So "C" is now pointing to the wrong place.

          At code generation time, when we try to generate the projection columns for PRN0, we'll take a look at it's virtual column ids and generate the project "mapping" array from those. In ProjectRestrictNode we find the following within "generateMinion()":

          // Map the result columns to the source columns
          int[] mapArray = resultColumns.mapSourceColumns();

          The method "mapSourceColumns()" simply walks through the RCL and plugs in the virtual column ids for each of the columns. For PRN0, since "A" and "C" both have a virtual column id of "1", we'll get back an array whose contents are "[1, 1]".

          That array is then passed to the execution-time ProjectRestrictResultSet and is used for extracting columns from the underlying result set, which will be a WindowResultSet in this case. So we'll get rows from WindowResultSet that have the values (1, 'HMM'), (2, 'HMM'), etc. That part is fine. But then the ProjectRestrictResultSet for PRN0 will use the "mapping" array to pull columns out of each row from WindowResultSet. Since both elements in the array are "1", we extract out the 1st column and put it into both places. So the final rows will end up as (1, 1), (2, 2), etc. which is wrong.

          All of that said, code fragment "A" resolved this problem by cloning SELECT's result column list and making all modifications on the clone. So when the virtual column id for "HMM" changes from "2" to "1", PRN0 doesn't see the change and therefore code generation and execution run fine.

          But as I mentioned at the start of this comment, the cloning leads to other problems--such as the queries that are still failing with patch 18. To make a long story short, I tried various "tweaks" (or perhaps "hacks" is a better word) to get the virtual column ids for PRN0 to remain correct without having to clone SELECT's columns. In the end I couldn't find a clean way to do so.

          But I did find that if I made the following changes, things started working:

          1) REMOVE code fragment "A" from SelectNode, as mentioned above.
          2) REMOVE the following from SelectNode.java:

          for (int index = 0; index < size; index++) {
          ResultColumn rc = (ResultColumn) originalRCL.elementAt(index);
          /*

          • NOTE: We only care about window function columns that appear
          • here, and not about references into subquerys or similar.
            */
            if (!rc.expressionIsWindowFunction()) { windowFunctionRCL.setElementAt( prnRSN.getResultColumns().elementAt(srcindex), index); srcindex++; }

            }

          3) REMOVE the following from ResultColumnList:

          /*

          • All expressions are not columns if this RCL contains
          • - A windowfunction column
          • - A virtual column node pointing to window function column
          • This check is done as a separate loop to avoid incorrectly start
          • marking any/parts of the base table columns as correlated columns.
          • Marking a VCN pointing into a WindowResultSet as correlated cause
          • resultset reference errors during code generation.
            */
            for (int index = 0; index < size; index++)
            Unknown macro: { ResultColumn rc = (ResultColumn) elementAt(index); if (rc.isWindowFunction()){ return false; } }

          4) Change ProjectRestrictNode.java as follows:

          // Map the result columns to the source columns

          • int[] mapArray = resultColumns.mapSourceColumns();
            + int[] mapArray =
            resultColumns.containsWindowFunctionResultColumn()
            ? childResult.getResultColumns().mapSourceColumns()
            : resultColumns.mapSourceColumns();

          I think this change has the effect of saying "ignore PRN0's RCL if it points to a WindowNode, because PRN0's RCL may not be correct--esp. it may have incorrect virtual column ids". Thus instead of using "[1,1]" for the projection mapping, PRN0 will create its mapping from PRNDUMMY, which has correct virtual column ids of "[1,2]", so everything ends up okay. At the same time, we did not have to clone the SELECT's ResultColumnList objects, and that appears to make the join queries run correctly.

          With these changes, OLAP tests runs without error and the two join queries I posted previously also execute without error. And from the brief looking that I did it seems like the results are correct, though it wouldn't hurt to double-check that.

          All in all, though, I'd have to say this seems pretty hackish, so it may not be a proper solution. But it does cause all of the queries posted so far to pass, and it simplifies the changes by removing the chunks of code mentioned above. So I figured I'd post my findings anyways. I'll leave it up to you to determine if any of it is useful or worth pursing further...

          Show
          A B added a comment - > the two last join queries both fail due to failure to classify some of the PRNs as noops. The code to check for "no-op" ProjectRestrictNodes depends, among other things, on object equality between two ResultColumn nodes. See ResultColumnList.nopProjection(). I think the manipulation that is done in SelectNode.genProjectRestrict() re-arranges the tree and creates new nodes such that the object equality fails. And even if the object equality wasn't an issue, I think re-arragement of the tree would cause the method to return false anyway. Which corresponds with your findings on "redundant" result column lists. I did some tracing for the queries that are still failing with patch 18. The failure occurs during code generation, when we try to generate the predicate "x2.c4 = t1.i" as a qualifier on table T1. When it comes time to generate the left operand, which points to a table that is BENEATH the WindowNode that was generated for the ROW_NUMBER(), we can't find the target result set number for the operand "X2.C4". In looking at the predicate contents at the point of failure, I could see that the target result set number was buried down inside of a ResultColumn-to-VirtualColumn chain rooted at X2.C4. Again, this matches your findings regarding "redundant" result columns. I then replaced ROW_NUMBER() with a simple expression ("i+j") and stopped at the same point; in this case the target result set number is readily available (i.e. it's not buried) and the query passes. On a whim, I then removed the following code from SelectNode: Code fragment "A": /* The this.resultColumns object is shared with any PRNs above this SelectNode in case this is a subquery. The following RCL modifications cause problems in the column mapping of the upper PRN unless we start off by making a copy for ourselves. */ setResultColumns(resultColumns.copyListAndObjects()); When I took that out, the target result set number became readily available for the join queries and the queries executed without error, which was good. Of course, the queries returned the wrong results, and several other ROW_NUMBER() queries which used to work started failing again. So I tried to figure out why the code was added in the first place, as the comments didn't quite make it clear for me. As far as I can tell, the scenario that prompted the addition of the above code is something like: select * from (select row_number() over(), 'HMM' from t1) x(a,c) where a > 2 which leads to a (simplified) tree that looks like: ... PRN0(a,c) -> SELECT(row_number(), 'HMM') -> ... In this tree, PRN0's RCL is a different object than SELECT's RCL, and the columns in PRN0's RCL have VirtualColumnNodes that do not appear in SELECT's RCL. But underneath it all, both RCLs ultimately point to the same underlying ResultColumn objects. Thus any changes we make to the underlying ResultColumns in SELECT's RCL will be "shared" by PRN0, meaning that the ResultColumns beneath PRN0's RCL will see the changes, as well. From what I can gather, the changes that we are talking about here deal with virtual column ids. During SelectNode.genProjectRestrict() we remove the WindowFunctionColumns from SELECT's ResultColumnList and then adjust the virtual column id's accordingly. So we get: ... PRN0(a,c) -> SELECT('HMM') -> ... In the absence of code fragment "A", the result column for "HMM" now has a virtual column id of "1" (instead of "2"). The fact that we changed the virtual column id from "2" to "1" means that the result column beneath "C" in PRN0's ResultColumnList also now has a virtual column id of "1". That indicates that "C" should pull its value from the 1st column in its child's RCL. This is where things start to go wrong... When we finish with SelectNode.genProjectRestrict(), our simplified tree looks like: PRN0(a,c) -> PRNDUMMY(row_number(), 'HMM') -> WindowNode(row_number(), 'HMM') -> PRN1('HMM') // this now has virtual column id of "1" -> ... Note that PRNDUMMY will be a "no-op" project restrict and thus will not actually be generated. So PRN0's child will end up being WindowNode, which has TWO columns. But PRN0.C still has a virtual column id of "1", meaning that it should pull its value from the first column of PRN0's child. PRN0's child is WindowNode, and the first column in WindowNode is row_number()--not 'HMM'. So "C" is now pointing to the wrong place. At code generation time, when we try to generate the projection columns for PRN0, we'll take a look at it's virtual column ids and generate the project "mapping" array from those. In ProjectRestrictNode we find the following within "generateMinion()": // Map the result columns to the source columns int[] mapArray = resultColumns.mapSourceColumns(); The method "mapSourceColumns()" simply walks through the RCL and plugs in the virtual column ids for each of the columns. For PRN0, since "A" and "C" both have a virtual column id of "1", we'll get back an array whose contents are " [1, 1] ". That array is then passed to the execution-time ProjectRestrictResultSet and is used for extracting columns from the underlying result set, which will be a WindowResultSet in this case. So we'll get rows from WindowResultSet that have the values (1, 'HMM'), (2, 'HMM'), etc. That part is fine. But then the ProjectRestrictResultSet for PRN0 will use the "mapping" array to pull columns out of each row from WindowResultSet. Since both elements in the array are "1", we extract out the 1st column and put it into both places. So the final rows will end up as (1, 1), (2, 2), etc. which is wrong. All of that said, code fragment "A" resolved this problem by cloning SELECT's result column list and making all modifications on the clone. So when the virtual column id for "HMM" changes from "2" to "1", PRN0 doesn't see the change and therefore code generation and execution run fine. But as I mentioned at the start of this comment, the cloning leads to other problems--such as the queries that are still failing with patch 18. To make a long story short, I tried various "tweaks" (or perhaps "hacks" is a better word) to get the virtual column ids for PRN0 to remain correct without having to clone SELECT's columns. In the end I couldn't find a clean way to do so. But I did find that if I made the following changes, things started working: 1) REMOVE code fragment "A" from SelectNode, as mentioned above. 2) REMOVE the following from SelectNode.java: for (int index = 0; index < size; index++) { ResultColumn rc = (ResultColumn) originalRCL.elementAt(index); /* NOTE: We only care about window function columns that appear here, and not about references into subquerys or similar. */ if (!rc.expressionIsWindowFunction()) { windowFunctionRCL.setElementAt( prnRSN.getResultColumns().elementAt(srcindex), index); srcindex++; } } 3) REMOVE the following from ResultColumnList: /* All expressions are not columns if this RCL contains - A windowfunction column - A virtual column node pointing to window function column This check is done as a separate loop to avoid incorrectly start marking any/parts of the base table columns as correlated columns. Marking a VCN pointing into a WindowResultSet as correlated cause resultset reference errors during code generation. */ for (int index = 0; index < size; index++) Unknown macro: { ResultColumn rc = (ResultColumn) elementAt(index); if (rc.isWindowFunction()){ return false; } } 4) Change ProjectRestrictNode.java as follows: // Map the result columns to the source columns int[] mapArray = resultColumns.mapSourceColumns(); + int[] mapArray = resultColumns.containsWindowFunctionResultColumn() ? childResult.getResultColumns().mapSourceColumns() : resultColumns.mapSourceColumns(); I think this change has the effect of saying "ignore PRN0's RCL if it points to a WindowNode, because PRN0's RCL may not be correct--esp. it may have incorrect virtual column ids". Thus instead of using " [1,1] " for the projection mapping, PRN0 will create its mapping from PRNDUMMY, which has correct virtual column ids of " [1,2] ", so everything ends up okay. At the same time, we did not have to clone the SELECT's ResultColumnList objects, and that appears to make the join queries run correctly. With these changes, OLAP tests runs without error and the two join queries I posted previously also execute without error. And from the brief looking that I did it seems like the results are correct, though it wouldn't hurt to double-check that. All in all, though, I'd have to say this seems pretty hackish, so it may not be a proper solution. But it does cause all of the queries posted so far to pass, and it simplifies the changes by removing the chunks of code mentioned above. So I figured I'd post my findings anyways. I'll leave it up to you to determine if any of it is useful or worth pursing further...
          Hide
          Thomas Nielsen added a comment -

          I obviously have a lot to learn when it comes to writing my explainations in jira...

          Your initial analysis is identical to what I've found, and explained in an exceptional manner.

          > To make a long story short, I tried various "tweaks" (or perhaps "hacks" is a better word) to get the virtual column ids for PRN0 to remain correct without > having to clone SELECT's columns. In the end I couldn't find a clean way to do so.

          The cloning of the RCL in PRN.genProjectRestrictNode (fragment A) was the only way out I could find, and that again resulted in the changes 2) and 3).

          I really like the fact that your "proposal" doesn't need cloning the RCL (fragment A) - makes it all a lot cleaner.

          Even though your snippet 4) feels like a hack, it might be able to pull this logic into PRN.modifyAccessPath()?
          The simplified tree looks like:
          SELECT() - PRN () - SELECT()
          We then call into the PRNs modifyAccessPath(), that again calls into the lower select modifyAccessPath() where we pull the WindowNodes up and modify the RCL. Once this is done, we could check for the presence of a PRN->WindowNode as the top of the intial PRNs childResult, and pull the modified RCL up into the initial PRN? By doing so I imagine we may be able to eliminate any other problems caused by potential later/additional optimizer modifications to the querytree. I'll give it a try at least, and see what happens.

          Show
          Thomas Nielsen added a comment - I obviously have a lot to learn when it comes to writing my explainations in jira... Your initial analysis is identical to what I've found, and explained in an exceptional manner. > To make a long story short, I tried various "tweaks" (or perhaps "hacks" is a better word) to get the virtual column ids for PRN0 to remain correct without > having to clone SELECT's columns. In the end I couldn't find a clean way to do so. The cloning of the RCL in PRN.genProjectRestrictNode (fragment A) was the only way out I could find, and that again resulted in the changes 2) and 3). I really like the fact that your "proposal" doesn't need cloning the RCL (fragment A) - makes it all a lot cleaner. Even though your snippet 4) feels like a hack, it might be able to pull this logic into PRN.modifyAccessPath()? The simplified tree looks like: SELECT() - PRN () - SELECT() We then call into the PRNs modifyAccessPath(), that again calls into the lower select modifyAccessPath() where we pull the WindowNodes up and modify the RCL. Once this is done, we could check for the presence of a PRN->WindowNode as the top of the intial PRNs childResult, and pull the modified RCL up into the initial PRN? By doing so I imagine we may be able to eliminate any other problems caused by potential later/additional optimizer modifications to the querytree. I'll give it a try at least, and see what happens.
          Hide
          Thomas Nielsen added a comment -

          The hack that Army did really solves the fact that VCNs are wrong in the initial PRN when there are window function columns in the PRNs child. I kept Armys 1)3) changes, but removed the hack and fixed it by regenerating the VCNs in PRN.modifyAccessPath(). This is done after modifying the childResult if that modification ends with the PRNs child being PRN>WN (meaning there are window functions in the childs RCL). This works fine, and I expect it to actually solve other possible problems with later/further optimization too.

          Attaching patch 19 and test9:

          • fix as described above
          • some cleanup of unused methods and whitespace diffs
          • test9 includes the last two join queries.

          OLAPTest with test9 applied pass.
          I'll run suites.All and derbyAll to make sure nothing else is broken.

          Show
          Thomas Nielsen added a comment - The hack that Army did really solves the fact that VCNs are wrong in the initial PRN when there are window function columns in the PRNs child. I kept Armys 1) 3) changes, but removed the hack and fixed it by regenerating the VCNs in PRN.modifyAccessPath(). This is done after modifying the childResult if that modification ends with the PRNs child being PRN >WN (meaning there are window functions in the childs RCL). This works fine, and I expect it to actually solve other possible problems with later/further optimization too. Attaching patch 19 and test9: fix as described above some cleanup of unused methods and whitespace diffs test9 includes the last two join queries. OLAPTest with test9 applied pass. I'll run suites.All and derbyAll to make sure nothing else is broken.
          Hide
          Thomas Nielsen added a comment - - edited

          Both suites.All and derbyAll passed with patch 19

          Show
          Thomas Nielsen added a comment - - edited Both suites.All and derbyAll passed with patch 19
          Hide
          A B added a comment -

          Thanks for patch 19, Thomas. I think your approach to handling the virtual column nodes is a good one (certainly better than the alternatives seen thus far). In the interest of getting something into the codeline for others to play with-and to simplify future patches-per Dan's comment a few days back, I'll try to commit patch 19 sometime today.

          Once that is committed, perhaps you can post one or more follow-up patches to address the following un-related comments, which I noticed while looking into the VCN issue. Of these # 3 is probably the most important one...

          1) Instead of using "row_number()" for the default name of a ROW_NUMBER() column, I think it'd be better to use a normal generated column name, as is done for all other un-named expressions in Derby (I believe?). So something like:

          select i, j, i+j, row_number() over() from t1

          should return four columns, "I", "J", "3", and "4", where the last two are generic generated names used for unnamed expressions. With patch 19 such a query currently returns "I", "J", "3", "row_number()", which deviates from the existing pattern.

          2) Regarding the following logic in PredicateList.pushExpressionsIntoSelect():

          ColumnReference ref = (ColumnReference)e.nextElement();
          if (!ref.pointsToColumnReference() &&
          !ref.pointsToWindowFunction())

          { state = false; break; }

          I think it says "if the column reference points to something that is neither a) another column reference NOR b) a window function, then do not push it down." So if the column reference points to a window function, we we could still potentially push it down. Is that what we want? For the record, I commented the call to "pointsToWindowFunction()" out completely and all of the queries still ran the same. So is this check still necessary?

          3) The following query returns one row, regardless of how many rows there are in T1:

          select distinct row_number() over(), 'HMM' from t1

          It appears that we will evaluate the entire SELECT result set, then apply the DISTINCT, and finally, assign the row numbers. So if there are five rows in T1, we'll get five rows having a single "HMM" column, then we'll apply the distinct to get a single "HMM" row, and that row will have row number "1". But is that the correct behavior? Or is the row_number() supposed to be included in the "DISTINCT" qualification, in which case the query would return 5 rows because each row has a different row number and thus each is distinct from the others?

          Just to see what would happen I ran the above query on DB2 v8 and it returned one row for each row in T1. I took a look at the syntax rules for <window function> in SQL 2003 spec section 6.10 but could not, in my inexperience with reading specs, determine one way or the other. That said, though, I think functions in a ResultColumnList are typically evaluated once per row, prior to the DISTINCT being applied, so I would guess ROW_NUMBER() should do the same?

          4) Might be good to add some test cases for GROUP BY and HAVING clauses in the presence of ROW_NUMBER(). I tried a few quick ones and they seem to be working fine, but it wouldn't hurt to add some tests for posterity. These are the ones I tried, but use your imagination

          select r from (select i, row_number() over() as r, j from t1) x group by r;
          select * from (select i, row_number() over() as r, j from t1) x group by i, j, r;
          select * from (select i, row_number() over() as r, j from t1) x group by j, r, i;

          select * from
          (select i, row_number() over() as r, j from t1) x
          group by j, r, i
          having r > 2;

          select * from
          (select i, row_number() over() as r, j from t1) x
          group by j, r, i
          having r > 2 and i >=3
          order by i desc;

          select * from
          (select i, row_number() over() as r, j from t1) x
          group by j, r, i
          having r > 2 and i >=3
          order by r desc;

          select * from
          (select i, row_number() over() as r, j from t1) x
          group by j, r, i
          having r > 2 and i >=3
          order by i asc, r desc;

          And yes I realize that grouping by a ROW_NUMBER() is kind of silly since row numbers are unique across rows--but it's good make sure the behavior is correct

          5) SQL spec indicates that window functions like ROW_NUMBER() can be used in an ORDER BY clause, but that is not yet implemented (right?). Do you think it would be worth it to add a statement saying as much to the documentation?

          Show
          A B added a comment - Thanks for patch 19, Thomas. I think your approach to handling the virtual column nodes is a good one (certainly better than the alternatives seen thus far). In the interest of getting something into the codeline for others to play with- and to simplify future patches -per Dan's comment a few days back, I'll try to commit patch 19 sometime today. Once that is committed, perhaps you can post one or more follow-up patches to address the following un-related comments, which I noticed while looking into the VCN issue. Of these # 3 is probably the most important one... 1) Instead of using "row_number()" for the default name of a ROW_NUMBER() column, I think it'd be better to use a normal generated column name, as is done for all other un-named expressions in Derby (I believe?). So something like: select i, j, i+j, row_number() over() from t1 should return four columns, "I", "J", "3", and "4", where the last two are generic generated names used for unnamed expressions. With patch 19 such a query currently returns "I", "J", "3", "row_number()", which deviates from the existing pattern. 2) Regarding the following logic in PredicateList.pushExpressionsIntoSelect(): ColumnReference ref = (ColumnReference)e.nextElement(); if (!ref.pointsToColumnReference() && !ref.pointsToWindowFunction()) { state = false; break; } I think it says "if the column reference points to something that is neither a) another column reference NOR b) a window function, then do not push it down." So if the column reference points to a window function, we we could still potentially push it down. Is that what we want? For the record, I commented the call to "pointsToWindowFunction()" out completely and all of the queries still ran the same. So is this check still necessary? 3) The following query returns one row, regardless of how many rows there are in T1: select distinct row_number() over(), 'HMM' from t1 It appears that we will evaluate the entire SELECT result set, then apply the DISTINCT, and finally, assign the row numbers. So if there are five rows in T1, we'll get five rows having a single "HMM" column, then we'll apply the distinct to get a single "HMM" row, and that row will have row number "1". But is that the correct behavior? Or is the row_number() supposed to be included in the "DISTINCT" qualification, in which case the query would return 5 rows because each row has a different row number and thus each is distinct from the others? Just to see what would happen I ran the above query on DB2 v8 and it returned one row for each row in T1. I took a look at the syntax rules for <window function> in SQL 2003 spec section 6.10 but could not, in my inexperience with reading specs, determine one way or the other. That said, though, I think functions in a ResultColumnList are typically evaluated once per row, prior to the DISTINCT being applied, so I would guess ROW_NUMBER() should do the same? 4) Might be good to add some test cases for GROUP BY and HAVING clauses in the presence of ROW_NUMBER(). I tried a few quick ones and they seem to be working fine, but it wouldn't hurt to add some tests for posterity. These are the ones I tried, but use your imagination select r from (select i, row_number() over() as r, j from t1) x group by r; select * from (select i, row_number() over() as r, j from t1) x group by i, j, r; select * from (select i, row_number() over() as r, j from t1) x group by j, r, i; select * from (select i, row_number() over() as r, j from t1) x group by j, r, i having r > 2; select * from (select i, row_number() over() as r, j from t1) x group by j, r, i having r > 2 and i >=3 order by i desc; select * from (select i, row_number() over() as r, j from t1) x group by j, r, i having r > 2 and i >=3 order by r desc; select * from (select i, row_number() over() as r, j from t1) x group by j, r, i having r > 2 and i >=3 order by i asc, r desc; And yes I realize that grouping by a ROW_NUMBER() is kind of silly since row numbers are unique across rows--but it's good make sure the behavior is correct 5) SQL spec indicates that window functions like ROW_NUMBER() can be used in an ORDER BY clause, but that is not yet implemented (right?). Do you think it would be worth it to add a statement saying as much to the documentation?
          Hide
          Thomas Nielsen added a comment -

          Thanks for your continued help Army. I can indeed add some smaller additional smaller patches to fix these issues if patch 19 goes in the trunk.

          Ad 1) Agree. This should follow the existing pattern.

          Ad 2) ...obviously not needed anymore.

          Ad 3)
          In SQL2003 DISTINCT handling is defined in 10.9 <aggregate function>. Its section "General Rules", clause 4), b), i) refers on to subclause 8.2 <comparison predicate> for how to eliminate duplicates. 8.2 does not specify how to handle window functions explicitly, only general number/value handling.
          Since 10.9s "General Rules" 4) a) specifies that expressions should be evaluated for every row before DISTINCT, I agree that the current patch is not according to the spec in this respect. The DB2 behaviour you see is conformant. We should return 5, not 1, rows as you say.

          I haven't given this a very hard look, but it seems this will introduce some additional branching in SelectNode.genProjectRestrict(). As of now, DISTINCT is evaluated before ordering and grouping. To be conformant for window functions, it looks like we have to delay the DISTINCT evaluation, or even pull the DISTINCT out of the current PRN, and evaluate it in an additional PRN on top of the current one. Probably is sufficient to mark as !distinctScanPossible and delay insertion of the DISTINCT_NODE if there's a window function in the RCL.

          Ad 4)
          My imagination... ugh I'll add the one you listed at least, and see if I can find some extra ones too.

          Ad 5)
          No, window functions are not allowed in the ORDER BY clause as of now. Agree - it's a good idea to add this to the docs.

          Show
          Thomas Nielsen added a comment - Thanks for your continued help Army. I can indeed add some smaller additional smaller patches to fix these issues if patch 19 goes in the trunk. Ad 1) Agree. This should follow the existing pattern. Ad 2) ...obviously not needed anymore. Ad 3) In SQL2003 DISTINCT handling is defined in 10.9 <aggregate function>. Its section "General Rules", clause 4), b), i) refers on to subclause 8.2 <comparison predicate> for how to eliminate duplicates. 8.2 does not specify how to handle window functions explicitly, only general number/value handling. Since 10.9s "General Rules" 4) a) specifies that expressions should be evaluated for every row before DISTINCT, I agree that the current patch is not according to the spec in this respect. The DB2 behaviour you see is conformant. We should return 5, not 1, rows as you say. I haven't given this a very hard look, but it seems this will introduce some additional branching in SelectNode.genProjectRestrict(). As of now, DISTINCT is evaluated before ordering and grouping. To be conformant for window functions, it looks like we have to delay the DISTINCT evaluation, or even pull the DISTINCT out of the current PRN, and evaluate it in an additional PRN on top of the current one. Probably is sufficient to mark as !distinctScanPossible and delay insertion of the DISTINCT_NODE if there's a window function in the RCL. Ad 4) My imagination... ugh I'll add the one you listed at least, and see if I can find some extra ones too. Ad 5) No, window functions are not allowed in the ORDER BY clause as of now. Agree - it's a good idea to add this to the docs.
          Hide
          Thomas Nielsen added a comment -

          Ad 3)
          I quickly tried delaying DISTINCT evaluation when there's a window function in the RCL, and it seems to work as expected - behaves like Army reported from DB2, and does not break the existing test.

          Show
          Thomas Nielsen added a comment - Ad 3) I quickly tried delaying DISTINCT evaluation when there's a window function in the RCL, and it seems to work as expected - behaves like Army reported from DB2, and does not break the existing test.
          Hide
          A B added a comment - - edited

          Committed d2998-19.diff and d2998-test9.diff both with svn # 632494:

          URL: http://svn.apache.org/viewvc?rev=632494&view=rev

          In addition to the follow-up work mentioned in my previous comment, I also noticed one more thing that requires investigation. The following query fails with an ArrayIndexOutOfBounds exception while trying to print the query plan (i.e. after the query is executed, and only if "logQueryPlan" is set to true):

          select count from
          (select i, row_number() over () from t1
          union all
          select distinct row_number() over(), row_number() over() from t1) x(a, b);

          If I remove the "DISTINCT" or else replace one of the row_number() columns in the SELECT DISTINCT query with a simple column reference, the query plan is generated correctly. Pretty minor in the grand scheme of things, but I thought I'd mention it.

          Thanks for all of your work on this, Thomas!

          Show
          A B added a comment - - edited Committed d2998-19.diff and d2998-test9.diff both with svn # 632494: URL: http://svn.apache.org/viewvc?rev=632494&view=rev In addition to the follow-up work mentioned in my previous comment, I also noticed one more thing that requires investigation. The following query fails with an ArrayIndexOutOfBounds exception while trying to print the query plan (i.e. after the query is executed, and only if "logQueryPlan" is set to true): select count from (select i, row_number() over () from t1 union all select distinct row_number() over(), row_number() over() from t1) x(a, b); If I remove the "DISTINCT" or else replace one of the row_number() columns in the SELECT DISTINCT query with a simple column reference, the query plan is generated correctly. Pretty minor in the grand scheme of things, but I thought I'd mention it. Thanks for all of your work on this, Thomas!
          Hide
          Thomas Nielsen added a comment -

          Thanks for your help Army

          I'll start looking at the followup patches.

          Show
          Thomas Nielsen added a comment - Thanks for your help Army I'll start looking at the followup patches.
          Hide
          Thomas Nielsen added a comment -

          Attaching miniature patch for followup issue #1 - column name changed from "row_number()" to existing pattern (meaning not set when not explicitly aliased).

          Show
          Thomas Nielsen added a comment - Attaching miniature patch for followup issue #1 - column name changed from "row_number()" to existing pattern (meaning not set when not explicitly aliased).
          Hide
          Thomas Nielsen added a comment -

          Attaching miniature patch for followup issue #2 - unnecessary pointsToWindowNode() in PredicateList.java
          Like Army posted, OLAPTest still pass after this change.

          Show
          Thomas Nielsen added a comment - Attaching miniature patch for followup issue #2 - unnecessary pointsToWindowNode() in PredicateList.java Like Army posted, OLAPTest still pass after this change.
          Hide
          Dyre Tjeldvoll added a comment -

          Committed d2998-followup-issue1.diff with revision 632988, and d2998-followup-2.diff with revision 632992.

          Show
          Dyre Tjeldvoll added a comment - Committed d2998-followup-issue1.diff with revision 632988, and d2998-followup-2.diff with revision 632992.
          Hide
          Thomas Nielsen added a comment -

          Attaching a patch to include the OLAPTest in the lang testsuite, 'd2998-followup-testsuite.diff'
          Suite runs successfully with this patch applied.

          Show
          Thomas Nielsen added a comment - Attaching a patch to include the OLAPTest in the lang testsuite, 'd2998-followup-testsuite.diff' Suite runs successfully with this patch applied.
          Hide
          Thomas Nielsen added a comment -

          Attaching updated doc patch.
          Also addresses Armys followup issue #5.

          Docs build cleanly with this patch.

          Show
          Thomas Nielsen added a comment - Attaching updated doc patch. Also addresses Armys followup issue #5. Docs build cleanly with this patch.
          Hide
          Thomas Nielsen added a comment -

          Thanks for committing the followups, Dyre.

          Attaching patch with group by and having queries that Army listed - issue #4.
          OLAPTest pass with this patch applied.

          Show
          Thomas Nielsen added a comment - Thanks for committing the followups, Dyre. Attaching patch with group by and having queries that Army listed - issue #4. OLAPTest pass with this patch applied.
          Hide
          Dyre Tjeldvoll added a comment -

          Patch file: d2998-followup-testsuite.diff
          Committed revision 633058.

          Patch file: d2998-followup-issue4.diff
          Committed revision 633067.

          Show
          Dyre Tjeldvoll added a comment - Patch file: d2998-followup-testsuite.diff Committed revision 633058. Patch file: d2998-followup-issue4.diff Committed revision 633067.
          Hide
          Dyre Tjeldvoll added a comment -

          Patch file: d2998-doc-2.diff
          Committed revision 633083.

          Show
          Dyre Tjeldvoll added a comment - Patch file: d2998-doc-2.diff Committed revision 633083.
          Hide
          Thomas Nielsen added a comment -

          Thanks again Dyre.

          Attaching followup patch for the ArrayIndexOutOfBoundsException when logging queryplans.
          There was an unhandled case of an existing but empty list in RealDistinctScanStatistics.java.

          lang._Suite runs fine (doesn't print queryplans AFAIK), and derbyall is still running (but shouldn't show any problems).

          Show
          Thomas Nielsen added a comment - Thanks again Dyre. Attaching followup patch for the ArrayIndexOutOfBoundsException when logging queryplans. There was an unhandled case of an existing but empty list in RealDistinctScanStatistics.java. lang._Suite runs fine (doesn't print queryplans AFAIK), and derbyall is still running (but shouldn't show any problems).
          Hide
          A B added a comment -

          Thank you for all of the follow-up patches, Thomas. (And thank for the commits, Dyre!)

          Regarding d2998-followup-2.diff: with this change the method "pointsToWindowFunction()" in ColumnReference.java is no longer called. Would it make sense to remove the method altogether?

          Regarding d2998-followup-queryplanAOOB.diff: With this patch applied the query plan now shows:

          select count from
          (select i, row_number() over () from t1
          union all
          select distinct row_number() over(), row_number() over() from t1) x(a, b);

          ...
          Distinct Scan ResultSet for T1 at read committed isolation level using instantaneous share row locking:
          Number of opens = 1
          Hash table size = 1
          Distinct column is column number -1
          Rows seen = 1

          From reading the plan it wasn't immediately clear to me what "Distinct column is column number -1" meant. Esp. what does a column number of "-1" mean? I then ran a bunch of simple SELECT DISTINCT ... from T1 queries and determined that the distinct column number which is printed as part of the query plan is meant to be the 0-based position of the column as it appears in the conglomerate being scanned. Since ROW_NUMBER() is not a column in the conglomerate, I guess "-1" seems like an okay value.

          Having said that, though, I think it's interesting that if we replace ROW_NUMBER() with any other expression-such as "i+j" or a literal-a Distinct scan on the conglomerate doesn't happen: instead, we do a normal scan of the conglomerate and then a distinct sort on the results. I was expecting ROW_NUMBER() to behave the same, but apparently it doesn't. Is this because ROW_NUMBER() is (incorrectly) applied after the DISTINCT, instead of before, per earlier discussion? And if so, do you happen to know if this change to RealDistinctScanStatistics will still be needed after the DISTINCT issue is resolved?

          Show
          A B added a comment - Thank you for all of the follow-up patches, Thomas. (And thank for the commits, Dyre!) Regarding d2998-followup-2.diff: with this change the method "pointsToWindowFunction()" in ColumnReference.java is no longer called. Would it make sense to remove the method altogether? Regarding d2998-followup-queryplanAOOB.diff: With this patch applied the query plan now shows: select count from (select i, row_number() over () from t1 union all select distinct row_number() over(), row_number() over() from t1) x(a, b); ... Distinct Scan ResultSet for T1 at read committed isolation level using instantaneous share row locking: Number of opens = 1 Hash table size = 1 Distinct column is column number -1 Rows seen = 1 From reading the plan it wasn't immediately clear to me what "Distinct column is column number -1" meant. Esp. what does a column number of "-1" mean? I then ran a bunch of simple SELECT DISTINCT ... from T1 queries and determined that the distinct column number which is printed as part of the query plan is meant to be the 0-based position of the column as it appears in the conglomerate being scanned. Since ROW_NUMBER() is not a column in the conglomerate, I guess "-1" seems like an okay value. Having said that, though, I think it's interesting that if we replace ROW_NUMBER() with any other expression- such as "i+j" or a literal -a Distinct scan on the conglomerate doesn't happen: instead, we do a normal scan of the conglomerate and then a distinct sort on the results. I was expecting ROW_NUMBER() to behave the same, but apparently it doesn't. Is this because ROW_NUMBER() is (incorrectly) applied after the DISTINCT, instead of before, per earlier discussion? And if so, do you happen to know if this change to RealDistinctScanStatistics will still be needed after the DISTINCT issue is resolved?
          Hide
          Thomas Nielsen added a comment -

          Army> "pointsToWindowFunction()" in ColumnReference.java is no longer called. Would it make sense to remove the method altogether?

          Yes - it should be removed. Will fix tomorrow.

          Army> .... And if so, do you happen to know if this change to RealDistinctScanStatistics will still be needed after the DISTINCT issue is resolved?

          Fixing the distinct with a window function issue will result in placing the distinct node on top of the window node(s). Today it's the other way around. To do so involves explicitly doing a normal scan and a distinct sort - like you say. So, yes, we might not actually need it - please don't commit it yet.

          I'll start on the distinct fix tomorrow. The fix I tried in my sandbox worked for non-nested queries, but not when there's a subquery with a window function. This resulted in incorrect results being returned. Looked like a column mapping issue in the distinct node from the couple of alternative queries I tried without a window function. I know where to start looking at least

          Show
          Thomas Nielsen added a comment - Army> "pointsToWindowFunction()" in ColumnReference.java is no longer called. Would it make sense to remove the method altogether? Yes - it should be removed. Will fix tomorrow. Army> .... And if so, do you happen to know if this change to RealDistinctScanStatistics will still be needed after the DISTINCT issue is resolved? Fixing the distinct with a window function issue will result in placing the distinct node on top of the window node(s). Today it's the other way around. To do so involves explicitly doing a normal scan and a distinct sort - like you say. So, yes, we might not actually need it - please don't commit it yet. I'll start on the distinct fix tomorrow. The fix I tried in my sandbox worked for non-nested queries, but not when there's a subquery with a window function. This resulted in incorrect results being returned. Looked like a column mapping issue in the distinct node from the couple of alternative queries I tried without a window function. I know where to start looking at least
          Hide
          A B added a comment -

          Fixed some javadoc warnings introduced by d2998-19.diff with svn # 633251:

          URL: http://svn.apache.org/viewvc?rev=633251&view=rev

          Show
          A B added a comment - Fixed some javadoc warnings introduced by d2998-19.diff with svn # 633251: URL: http://svn.apache.org/viewvc?rev=633251&view=rev
          Hide
          Thomas Nielsen added a comment - - edited

          I was looking into why the distinct issue happened tonight and decided to fix it.
          Attaching 'd2998-followup-distinct.diff' that solves this for both queries and subqueries, as well as adding a couple of DISTINCT queries to OLAPTest.
          The updated OLAPTest pass.

          The proposed change in ProjectRestrictNode.java isn't perfect. I had to use a boolean variable to trigger VCN regeneration to avoid getting a nasty if-instanceof construct. It might just be too late in the day...

          When the distinct issue is fixed the ArrayIndexOutOfBoundsException Army reported is avoided since RealDistinctScanStatistics is not used. I'll remove the patch I uploaded earlier today.

          Show
          Thomas Nielsen added a comment - - edited I was looking into why the distinct issue happened tonight and decided to fix it. Attaching 'd2998-followup-distinct.diff' that solves this for both queries and subqueries, as well as adding a couple of DISTINCT queries to OLAPTest. The updated OLAPTest pass. The proposed change in ProjectRestrictNode.java isn't perfect. I had to use a boolean variable to trigger VCN regeneration to avoid getting a nasty if-instanceof construct. It might just be too late in the day... When the distinct issue is fixed the ArrayIndexOutOfBoundsException Army reported is avoided since RealDistinctScanStatistics is not used. I'll remove the patch I uploaded earlier today.
          Hide
          A B added a comment -

          I applied "d2998-followup-distinct.diff" and verified that both the DISTINCT issue and the ArrayIndexOutOfBounds issue are resolved.

          That said, in re-reading the comments in ProjectRestrictNode I noticed this:

          /*

          • We have a window function column in the RCL of our child
          • PRN, and need to regenerate the VCNs.
            */

          Out of curiosity I made a quick change so that, instead of checking for instances of WindowNode and DIstinctNode, the code just checked the condition described in the comment, i.e. "do we have a window function column in the RCL of our child PRN?". So I replaced:

          boolean regenVCN = false;
          ProjectRestrictNode prn = (ProjectRestrictNode)childResult;
          if (prn.childResult instanceof WindowNode)

          { regenVCN = true; }

          if (prn.childResult instanceof DistinctNode){
          DistinctNode dn = (DistinctNode)prn.childResult;
          if (dn.childResult instanceof WindowNode)

          { regenVCN = true; }

          }
          if (regenVCN){
          ...

          with

          ProjectRestrictNode prn = (ProjectRestrictNode)childResult;
          if (prn.childResult.getResultColumns()
          .containsWindowFunctionResultColumn())
          {
          ...

          From what I can tell this change still makes all of the queries pass (including OLAPTest). Would this be a viable alternative, or would such an approach be too broad? I admit I didn't do any tracing with this particular change, I just ran the tests and noted that they appear to run to correctly. The code seems cleaner and matches the comment, so I thought I'd throw it out there...

          On a completely unrelated note, I accidentally discovered that queries of the form:

          select * from (select row_number() over() as r, ... from t) x where r < ...

          can actually return different results depending on the presence of indexes. This is because the rows returned from the subquery have no guaranteed ordering (Derby doesn't allow ORDER BY in subqueries), and thus any predicate which restricts based on row_number() will restrict the rows based on an undefined order. Since the order of the rows from the subquery may depend on the presence of indexes, the set of rows which survives a row_order()based restriction may depend on the indexes, as well. In the end I do not think this is a bug-but it does strike me as a probable point of confusion for users. It seems that anyone who wants "the first x rows only" has to either accept the fact that "first" does not imply "ordered" (and thus results can vary depending on what conglomerate the optimizer chooses), or else s/he has to use optimizer ovverides to force the optimizer to use an index which is ordered on the desired columns. Is that an accurate assessment? I'm not saying anything needs to be done to address this, I'm just curious as to whether or not I've understood this correctly.

          But all of that aside, thanks for resolving the DISTINCT issue, Thomas!

          Show
          A B added a comment - I applied "d2998-followup-distinct.diff" and verified that both the DISTINCT issue and the ArrayIndexOutOfBounds issue are resolved. That said, in re-reading the comments in ProjectRestrictNode I noticed this: /* We have a window function column in the RCL of our child PRN, and need to regenerate the VCNs. */ Out of curiosity I made a quick change so that, instead of checking for instances of WindowNode and DIstinctNode, the code just checked the condition described in the comment, i.e. "do we have a window function column in the RCL of our child PRN?". So I replaced: boolean regenVCN = false; ProjectRestrictNode prn = (ProjectRestrictNode)childResult; if (prn.childResult instanceof WindowNode) { regenVCN = true; } if (prn.childResult instanceof DistinctNode){ DistinctNode dn = (DistinctNode)prn.childResult; if (dn.childResult instanceof WindowNode) { regenVCN = true; } } if (regenVCN){ ... with ProjectRestrictNode prn = (ProjectRestrictNode)childResult; if (prn.childResult.getResultColumns() .containsWindowFunctionResultColumn()) { ... From what I can tell this change still makes all of the queries pass (including OLAPTest). Would this be a viable alternative, or would such an approach be too broad? I admit I didn't do any tracing with this particular change, I just ran the tests and noted that they appear to run to correctly. The code seems cleaner and matches the comment, so I thought I'd throw it out there... On a completely unrelated note, I accidentally discovered that queries of the form: select * from (select row_number() over() as r, ... from t) x where r < ... can actually return different results depending on the presence of indexes. This is because the rows returned from the subquery have no guaranteed ordering (Derby doesn't allow ORDER BY in subqueries), and thus any predicate which restricts based on row_number() will restrict the rows based on an undefined order. Since the order of the rows from the subquery may depend on the presence of indexes, the set of rows which survives a row_order() based restriction may depend on the indexes, as well. In the end I do not think this is a bug -but it does strike me as a probable point of confusion for users. It seems that anyone who wants "the first x rows only" has to either accept the fact that "first" does not imply "ordered" (and thus results can vary depending on what conglomerate the optimizer chooses), or else s/he has to use optimizer ovverides to force the optimizer to use an index which is ordered on the desired columns. Is that an accurate assessment? I'm not saying anything needs to be done to address this, I'm just curious as to whether or not I've understood this correctly. But all of that aside, thanks for resolving the DISTINCT issue, Thomas!
          Hide
          Thomas Nielsen added a comment -

          I considered your proposed change, but turned it down as I thought it too general. That's why I ended up checking for the WindowNodes - we know it's at the top of the child if it's there. I'll do some tracing to see whether my conclusion was wrong or not.

          Wrt to window functions and ordering:
          Yes, the below ResultSets can return rows in an arbitrary order depending on indexes or not - and on other characteristics of the database engine. Very familiar with this from the distributed database I've worked on earlier. And, yes, my experience is it's something most users run into somehow The same goes for joins - think 2 rows joined with 3 rows. No guarantee that the join is performed a certain way and presence of an index may cause different row ordering that without. This really applies to all queries, as long as there is no ORDER BY clause, there is really no guarantee for the ordering. Since Derby does not support ordering in subqueries, we have no option but to take what the selected conglomorate supplies of row ordering.

          I'll add a paragraph about this behaviour to the row_number page in the reference manual.

          Note that once we get better/full window support, we should be able to specify ordering of the window in the window definition. This will hopefully help those that require the window to be ordered, and may work around the no-orderby-in-subquery limitation currently in Derby

          Show
          Thomas Nielsen added a comment - I considered your proposed change, but turned it down as I thought it too general. That's why I ended up checking for the WindowNodes - we know it's at the top of the child if it's there. I'll do some tracing to see whether my conclusion was wrong or not. Wrt to window functions and ordering: Yes, the below ResultSets can return rows in an arbitrary order depending on indexes or not - and on other characteristics of the database engine. Very familiar with this from the distributed database I've worked on earlier. And, yes, my experience is it's something most users run into somehow The same goes for joins - think 2 rows joined with 3 rows. No guarantee that the join is performed a certain way and presence of an index may cause different row ordering that without. This really applies to all queries, as long as there is no ORDER BY clause, there is really no guarantee for the ordering. Since Derby does not support ordering in subqueries, we have no option but to take what the selected conglomorate supplies of row ordering. I'll add a paragraph about this behaviour to the row_number page in the reference manual. Note that once we get better/full window support, we should be able to specify ordering of the window in the window definition. This will hopefully help those that require the window to be ordered, and may work around the no-orderby-in-subquery limitation currently in Derby
          Hide
          Thomas Nielsen added a comment -

          Attaching diff 'd2998-followup-doc3.diff' with extra note on the subquery ordering behaviour in the row_number page of ref man, as well as the resulting generated html.

          Show
          Thomas Nielsen added a comment - Attaching diff 'd2998-followup-doc3.diff' with extra note on the subquery ordering behaviour in the row_number page of ref man, as well as the resulting generated html.
          Hide
          Thomas Nielsen added a comment -

          Wrt allowing order by in a subquery select:
          I did the limited changes needed to allow order by in a subquery select, and have a working proto in my sandbox. Enables ordering and restriction on the window function column like we would like.

          ij> select * from (select row_number() over () as r, t.* from t order by b desc) tr where r<3;
          R |A |B
          --------------------------------------------
          1 |50 |500
          2 |40 |400

          Is this something we would like to support?
          Do you think it's worth making a new jira for this?

          The closest I could find was DERBY-4, but it's not identical as it centers around a INSERT INTO ... SELECT FROM ... query. And it is currently veto'ed. Looking at the patch for DERBY-4 now, I see it does more or less the same thing as my changes, only for insert statements.

          Show
          Thomas Nielsen added a comment - Wrt allowing order by in a subquery select: I did the limited changes needed to allow order by in a subquery select, and have a working proto in my sandbox. Enables ordering and restriction on the window function column like we would like. ij> select * from (select row_number() over () as r, t.* from t order by b desc) tr where r<3; R |A |B -------------------------------------------- 1 |50 |500 2 |40 |400 Is this something we would like to support? Do you think it's worth making a new jira for this? The closest I could find was DERBY-4 , but it's not identical as it centers around a INSERT INTO ... SELECT FROM ... query. And it is currently veto'ed. Looking at the patch for DERBY-4 now, I see it does more or less the same thing as my changes, only for insert statements.
          Hide
          A B added a comment -

          > I considered your proposed change, but turned it down as I thought it too general.

          Okay, good to know the idea was at least considered

          > I did the limited changes needed to allow order by in a subquery select
          > Is this something we would like to support?

          I think the first question is whether or not that is supported by the standard. If it's not in SQL standard but is "de-facto" standard, meaning (I think) that most database vendors support it and the behavior is consistent across vendors, then I think that would be acceptable, as well. But I don't know whether or not either of those is true for ORDER BY in a subquery?

          In any event, I definitely think it would warrant it's own Jira.

          Show
          A B added a comment - > I considered your proposed change, but turned it down as I thought it too general. Okay, good to know the idea was at least considered > I did the limited changes needed to allow order by in a subquery select > Is this something we would like to support? I think the first question is whether or not that is supported by the standard. If it's not in SQL standard but is "de-facto" standard, meaning (I think) that most database vendors support it and the behavior is consistent across vendors, then I think that would be acceptable, as well. But I don't know whether or not either of those is true for ORDER BY in a subquery? In any event, I definitely think it would warrant it's own Jira.
          Hide
          Thomas Nielsen added a comment -

          >> I considered your proposed change, but turned it down as I thought it too general.
          > Okay, good to know the idea was at least considered

          It seems using containsWindowFunctionResultColumn() is safe after all. I'll upload a new patch tomorrow morning, along with the removal of ColumnReference.pointsToWindowFunction().

          Army> I think the first question is whether or not that is supported by the standard.

          Actually it doesn't seem to be - and that's most likely the reason why it's not in there already The <order by clause> is part of <declare cursor>. <subquery> only includes <query expression>, which again includes <query specification>, which again does not include <declare cursor>.

          The specific problem we want to solve will be fixed when window ordering is included, so I won't pursue this now.

          Show
          Thomas Nielsen added a comment - >> I considered your proposed change, but turned it down as I thought it too general. > Okay, good to know the idea was at least considered It seems using containsWindowFunctionResultColumn() is safe after all. I'll upload a new patch tomorrow morning, along with the removal of ColumnReference.pointsToWindowFunction(). Army> I think the first question is whether or not that is supported by the standard. Actually it doesn't seem to be - and that's most likely the reason why it's not in there already The <order by clause> is part of <declare cursor>. <subquery> only includes <query expression>, which again includes <query specification>, which again does not include <declare cursor>. The specific problem we want to solve will be fixed when window ordering is included, so I won't pursue this now.
          Hide
          Thomas Nielsen added a comment -

          Attaching

          • updated followup patch for distinct with changes to PRN
          • followup patch to remove unused ColumnReference.pointsToWindowFunction()

          OLAPTest runs successfully with these patches.

          Show
          Thomas Nielsen added a comment - Attaching updated followup patch for distinct with changes to PRN followup patch to remove unused ColumnReference.pointsToWindowFunction() OLAPTest runs successfully with these patches.
          Hide
          A B added a comment -

          Committed d2998-followup-distinct.diff and d2998-followup-removeunused.diff with svn # 634057:

          URL: http://svn.apache.org/viewvc?rev=634057&view=rev

          Thanks Thomas! Can you perhaps give a quick update on what else you have planned for for this issue in the 10.4 timeframe?

          Show
          A B added a comment - Committed d2998-followup-distinct.diff and d2998-followup-removeunused.diff with svn # 634057: URL: http://svn.apache.org/viewvc?rev=634057&view=rev Thanks Thomas! Can you perhaps give a quick update on what else you have planned for for this issue in the 10.4 timeframe?
          Hide
          Thomas Nielsen added a comment -

          Thanks Army!

          I do not have any more work lined up on this for 10.4. Given that there's only two days until feature freeze, I don't think there's time to do much else

          I suggest we resolve and close this one as completed, and do any followup work in new jiras.

          Show
          Thomas Nielsen added a comment - Thanks Army! I do not have any more work lined up on this for 10.4. Given that there's only two days until feature freeze, I don't think there's time to do much else I suggest we resolve and close this one as completed, and do any followup work in new jiras.
          Hide
          A B added a comment -

          > I do not have any more work lined up on this for 10.4.

          Okay, thanks Thomas!

          The reason I asked is because on Feb 25 I posted a bunch of comments, of which #8 mentioned how predicates on a ROW_NUMBER() do not actually limit rows from store, and thus such a restriction will probably not meet the performance expectations of the user who specifies it. For reference, the example query was:

          SELECT * FROM (
          SELECT row_number() over () as r, t.* FROM T
          ) AS tmp WHERE r <= 3;

          for which the plan shows the following for the top-most ProjectRestrictResultSet:

                      • Project-Restrict ResultSet (1):
                        Number of opens = 1
                        Rows seen = 1280
                        Rows filtered = 1277
                        restriction = true

          So we actually read all 1280 rows from disk, then filtered 1277 of them out so that, in the end, we only returned 3 rows.

          In response to that you wrote (on Feb 26):

          Thomas> It seems we don't take the fact that we know we have an ascending column
          Thomas> into consideration any more (we used to). I'll have to have another look at this.

          So is it safe to say that this will not be addressed for 10.4? That's perfectly fine, I just want to make sure we're clear on that (esp. so that if users ask, we can give a clear answer Do you think it would be worth it to file a separate enhancement Jira for that particular task? (or is there one already?)

          Show
          A B added a comment - > I do not have any more work lined up on this for 10.4. Okay, thanks Thomas! The reason I asked is because on Feb 25 I posted a bunch of comments, of which #8 mentioned how predicates on a ROW_NUMBER() do not actually limit rows from store, and thus such a restriction will probably not meet the performance expectations of the user who specifies it. For reference, the example query was: SELECT * FROM ( SELECT row_number() over () as r, t.* FROM T ) AS tmp WHERE r <= 3; for which the plan shows the following for the top-most ProjectRestrictResultSet: Project-Restrict ResultSet (1): Number of opens = 1 Rows seen = 1280 Rows filtered = 1277 restriction = true So we actually read all 1280 rows from disk, then filtered 1277 of them out so that, in the end, we only returned 3 rows. In response to that you wrote (on Feb 26): Thomas> It seems we don't take the fact that we know we have an ascending column Thomas> into consideration any more (we used to). I'll have to have another look at this. So is it safe to say that this will not be addressed for 10.4? That's perfectly fine, I just want to make sure we're clear on that (esp. so that if users ask, we can give a clear answer Do you think it would be worth it to file a separate enhancement Jira for that particular task? (or is there one already?)
          Hide
          Thomas Nielsen added a comment -

          Updated the description field as suggested by Mike.

          Show
          Thomas Nielsen added a comment - Updated the description field as suggested by Mike.
          Hide
          Thomas Nielsen added a comment -

          Amry, I don't think there's time to address that in the 10.4 timeframe. But I'll give it a shot!

          I created DERBY-3505 for the "#8 comment" about ROW_NUMBER() performance. Whenever DERBY-3505 is fixed on trunk it can be ported to the 10.4 branch (no interface changes, only internal) and the improved performance shipped with the next update release off the 10.4 branch?

          Show
          Thomas Nielsen added a comment - Amry, I don't think there's time to address that in the 10.4 timeframe. But I'll give it a shot! I created DERBY-3505 for the "#8 comment" about ROW_NUMBER() performance. Whenever DERBY-3505 is fixed on trunk it can be ported to the 10.4 branch (no interface changes, only internal) and the improved performance shipped with the next update release off the 10.4 branch?
          Hide
          A B added a comment -

          > I'll give it a shot!

          Probably not something that should be rushed Post 10.4 is fine with me, I was just looking for a clear update on that issue. The filing of DERBY-3505 is good enough for me right now.

          Show
          A B added a comment - > I'll give it a shot! Probably not something that should be rushed Post 10.4 is fine with me, I was just looking for a clear update on that issue. The filing of DERBY-3505 is good enough for me right now.
          Hide
          Thomas Nielsen added a comment -

          Looks like the doc-3 patch still hasn't been comitted. I'll resolve once that has gone in

          Show
          Thomas Nielsen added a comment - Looks like the doc-3 patch still hasn't been comitted. I'll resolve once that has gone in
          Hide
          Dyre Tjeldvoll added a comment -

          Patch file: d2998-followup-doc3.diff
          Committed revision 634587.

          Show
          Dyre Tjeldvoll added a comment - Patch file: d2998-followup-doc3.diff Committed revision 634587.

            People

            • Assignee:
              Thomas Nielsen
              Reporter:
              Thomas Nielsen
            • Votes:
              5 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development