[gravity] added support for non recursive common table expressions in gravity

Review Request #542 — Created Feb. 14, 2018 and submitted

k.hengsdijk
Lunr
CTEbranch
lunr

added support for non recursive common table expressions in gravity



  • 0
  • 0
  • 49
  • 2
  • 51
Description From Last Updated
There are no open issues
smillernl
  1. 
      
  2. The issue has been resolved. Show all issues

    please use the format @return <class> <optional var> <description>

  3. The issue has been resolved. Show all issues

    Is this also supported in sqlite? Because this interface is shared with sqlite.

    1. yes the syntax of a WITH statement is exacly the same in sqlite so it will also work on sqlite.

  4. src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php (Diff revision 1)
     
     
     
     
     
     
    The issue has been resolved. Show all issues

    Please use the same style as $is_unfinished_join

  5. The issue has been resolved. Show all issues

    wouldn't this reset the entire object?

    1. it doesnt reset the entire object it just restores the default values to all the query parts needed for a SELECT statement because otherwise the SELECT statement thats supposed to be inside the WITH statement will also be added at the end of the query.

  6. 
      
k.hengsdijk
k.hengsdijk
k.hengsdijk
k.hengsdijk
smillernl
  1. 
      
  2. The issue has been resolved. Show all issues

    indentation

  3. The issue has been resolved. Show all issues

    indentation

  4. src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php (Diff revision 3)
     
     
     
     
     
     
    The issue has been dropped. Show all issues

    Is this really ending the "with" statement though? isn't it a part of the statement?

    1. yeah its technically part of the with statement but the closing brace defines the end of the with statement so thats why i called the function end_sql_with.

  5. 
      
k.hengsdijk
pprkut
  1. 
      
  2. src/Lunr/Gravity/Database/DMLQueryBuilderInterface.php (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    The issue has been resolved. Show all issues

    Splitting this makes it rather inconvenient to use

  3. The issue has been resolved. Show all issues

    $with is fine. We know it's part of the query

  4. 
      
k.hengsdijk
k.hengsdijk
k.hengsdijk
smillernl
  1. 
      
  2. The issue has been resolved. Show all issues

    indentation

  3. 
      
pprkut
  1. 
      
  2. The issue has been resolved. Show all issues

    Are you sure $with is the best name for this?

  3. The issue has been resolved. Show all issues

    Where does this come from?

    1. this is an option that the user can use the change the name of the result column of the with statement. Found the info about this on this site https://modern-sql.com/feature/with

    2. Ok, that's fine then. Double check that mysql/mariadb/sqlite actually really support that though.
      Additionally, optional parameters should be optional. Like you wrote it, it has to be specified all the time, even if you don't care about them. Make them optional by making them the last argument to the function.

  4. The issue has been resolved. Show all issues

    We don't need recursive for non-recursive CTEs. Keep this for the later review

  5. The issue has been resolved. Show all issues

    keep this for the later review

  6. 
      
k.hengsdijk
pprkut
  1. Please set up and run phpcs on your code

  2. The issue has been resolved. Show all issues

    'alias' is good enough. We know we're working with 'WITH'

    missing type definition

  3. The issue has been resolved. Show all issues

    missing empty comment line between params and @return

  4. The issue has been resolved. Show all issues

    missing @return

  5. src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php (Diff revision 7)
     
     
     
     
     
     
     
     
    The issue has been resolved. Show all issues

    indentation

  6. 
      
k.hengsdijk
smillernl
  1. 
      
  2. The issue has been resolved. Show all issues

    this is an list of strings correct?

    1. Makes sense to keep this the same as column_names(), yes

    2. okay i have changed the parameter to an array instead so a list of strings an be used

  3. The issue has been resolved. Show all issues

    unneeded space

  4. 
      
k.hengsdijk
pprkut
  1. 
      
  2. The issue has been resolved. Show all issues

    Please make that one actually optional

  3. The issue has been resolved. Show all issues

    wrong class name

  4. The issue has been resolved. Show all issues

    Missing '.'

  5. The issue has been resolved. Show all issues

    this looks way too complicated. What are you trying to achieve here?

    1. this is to take all the strings out of the array add commas between the strings and wrap the string in parentheses so that the result will look like this examle:
      '( columname1, columname2, columname3 )'.

      perhaps i can make this clearer by using a for each loop to get the string out of the array?

    2. How is that different from

      '(' . implode(', ', $column_names) . ')'

      ?

    3. turns out it isnt different

    4. ;)

  6. 
      
k.hengsdijk
pprkut
  1. Looks good :)
    Ready for unit tests

  2. 
      
k.hengsdijk
pprkut
  1. Quite good for a first try :)

  2. src/Lunr/Gravity/Database/SQLDMLQueryBuilder.php (Diff revision 11)
     
     
     
     
     
     
     
     
     
     
     
    The issue has been resolved. Show all issues

    Missing tests

  3. The issue has been resolved. Show all issues

    when would column_names be FALSE?

  4. The issue has been resolved. Show all issues

    This is probably nicer without the implode. Easier to verify correct syntax

  5. The issue has been resolved. Show all issues

    don't invoke multiple times. Call it once and make sure the necessary preconditions are met

  6. 
      
k.hengsdijk
p.valk
  1. 
      
  2. The issue has been resolved. Show all issues

    missing title * Define a WITH clause.

  3. 
      
smillernl
  1. 
      
  2. src/Lunr/Gravity/Database/SQLDMLQueryBuilder.php (Diff revision 12)
     
     
     
     
     
     
    The issue has been resolved. Show all issues

    This is missing unittests

  3. The issue has been resolved. Show all issues

    This usually defines the minimal supported version.

  4. The issue has been resolved. Show all issues

    Shouldn't this include the current year?

  5. 
      
k.hengsdijk
k.hengsdijk
smillernl
  1. 
      
  2. The issue has been resolved. Show all issues

    Desciptions start with uppercase

  3. The issue has been resolved. Show all issues

    Contains more than just the with

  4. The issue has been resolved. Show all issues

    Only in select queries?

    1. yes a CTE is only used with select queries

  5. src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php (Diff revision 13)
     
     
     
     
     
    The issue has been resolved. Show all issues

    Desciptions start with uppercase

  6. src/Lunr/Gravity/Database/SQLDMLQueryBuilder.php (Diff revision 13)
     
     
     
     
    The issue has been resolved. Show all issues

    Desciptions start with uppercase

  7. 
      
pprkut
  1. 
      
  2. The issue has been resolved. Show all issues

    Missing '.'

  3. The issue has been resolved. Show all issues

    generally, start decriptions in upper case

  4. The issue has been resolved. Show all issues

    !== or is_array()

  5. The issue has been resolved. Show all issues

    $this->with is empty in this case. You probably don't want .=

  6. src/Lunr/Gravity/Database/SQLDMLQueryBuilder.php (Diff revision 13)
     
     
     
     
    The issue has been resolved. Show all issues

    description in upper case

  7. The issue has been resolved. Show all issues

    2018

  8. The issue has been resolved. Show all issues

    please use [] instead of array()

  9. src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderQueryPartsWithTest.php (Diff revision 13)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    The issue has been resolved. Show all issues

    same test as testNonRecursiveWithWithoutColumnNames

  10. The issue has been resolved. Show all issues

    What is this supposed to accomplish?

    1. i created it when i was thinking about all possible ways to create a unit test but in hindsight its kinda useless

  11. The issue has been resolved. Show all issues

    no DELETE

  12. 
      
k.hengsdijk
pprkut
  1. Almost :)

  2. The issue has been resolved. Show all issues

    that's not correct afaics

  3. 
      
k.hengsdijk
smillernl
  1. 
      
  2. The issue has been resolved. Show all issues

    WithWith

  3. The issue has been resolved. Show all issues

    WithWith

  4. 
      
k.hengsdijk
k.hengsdijk
p.valk
  1. 
      
  2. The issue has been dropped. Show all issues

    isnt it better to use empty($this->with) here?

    1. thats not used anywhere else in the code for similar statements tho

    2. better check with heinz, not sure myself.
      i tought most of the time empty() and isset() was prefered.

    3. Not if you only want to check for an empty string. Which you're not doing here.

    4. In doubt, keep it consistent with the rest of the code. I don't mind changing it, but then it should be changed everywhere, and that should be a separate review

    5. okay so in that case this line is consistent with the rest of the code which means issue dropped or fixed?

    6. I don't think you fixed anything about it right?

  3. 
      
smillernl
  1. Ship It!
  2. 
      
k.hengsdijk
Review request changed

Status: Closed (submitted)

Change Summary:

Merged into master

Loading...