Extending DBAL query builder for boolean generation

[brunoais]

The whole idea behind this is to improve DBAL's query builder by adding a boolean query builder using a structured stack of arrays.

This can be used everywhere in an SQL query where boolean operations are used.
Currently, this can only be used in the WHERE and the JOINs' ON clause.
Initially, I'm intending on trying it just on the WHERE clause.

The main and general use-cases are:

1. To flexibly the build of the WHERE clause in SQL queries
2. To ease, simplify and prevent errors when doing SQL query editing by phpBB's extensions

Why not...

1. Doctrine dbal -> The issue with Doctrine dbal is that it's query builder is not ready for the 2nd major use case listed above. There is no way of altering an SQL query. If you want to alter something, you have to rebuild the whole SQL query. I also didn't find a way to know the currently build query except for getting the query built so far as a string.
2. Linq -> I didn't know the assistance of Linq until today. From what I searched, not only it has the same issue as Doctrine, while also its interface is unnecessarily complex for the common folk who just wants to change a small amount of information.

Quick guide on how to use

This builder uses a tree-like information organization for the boolean comparisons.
Now, there are 3 types of arrays:

1. A
2. B
3. C

(names are not thought yet)

The A type contains 3 elements:
Left hand, operator, right hand.
E.g.

Code: Select all

array('f.forum_id', '=', 1)
array('f.forum_id', '<>', 1)
array('f.forum_id', 'IN', array())
array('f.forum_id', 'IN', array(1,2,5,6,7))
array('f.forum_id', 'NOT_IN', array(1,2,5,6,7))
array('f.forum_id', 'IS', NULL)
 

The B type contains 5 elements:
Left hand, operator, sub query operator, sub query SELECT type, the sub query.

This is essentially a recursive call to sql_build_query() to parse that sub-tree which is the sub query.

Code: Select all

array('f.forum_id', '=', 'ANY', 'SELECT', array(
                    'SELECT' => array(/*...*/),
                    'FROM' => array(/*...*/),
                )
)

array('f.forum_id', '', 'IN', 'SELECT', array(
                    'SELECT' => array(/*...*/),
                    'FROM' => array(/*...*/),
                )
)
 

The C type which contains a boolean operator to apply to all other elements which are either A or B or C type:

Code: Select all

array('OR',
    array('t.forum_id', '=', 3),
    array('t.topic_type', '=', 0),
)

array('AND',
        array('t.forum_id', '=', 3),
        array('t.topic_type', '=', 0),
        array('t.topic_id', '>', 5),
        array('t.topic_poster', '<>', 5),
    ),


array('AND',
        array('t.forum_id', '=', 3),
        array('NOT',
            array('t.topic_type', '=', 0),
        ),
        array('t.topic_id', '>', 5),
        array('t.topic_poster', '<>', 5),
    ),
 

That last one produces:

Code: Select all

t.forum_id = 3
AND NOT ( t.topic_type = 0 )
AND t.topic_id > 5
AND t.topic_poster <> 5

Comparison examples
Here are some code examples of the "before" and "after" in order to fully use this feature.

Code: Select all

// Original
sql_build_query('SELECT',
    array(
        'SELECT'    => "COUNT(topic_id) AS num_topics",
        'FROM'        => array("phpbb_topics" => ''),

        'WHERE'        => 'topic_id IS NOT NULL',
    )

)

// Now
sql_build_query('SELECT',
    array(
        'SELECT'    => "COUNT(topic_id) AS num_topics",
        'FROM'        => array("phpbb_topics" => ''),

        'WHERE'        => 
                array('topic_id', 'IS NOT', 'NULL'),
    )

)
 

For the other examples, I'll shorten to show just the WHERE clause.

Code: Select all

'WHERE'        => 'forum_id = 1',

'WHERE'        => array('forum_id', '=', 1),

// Or, if you want to negate it:

'WHERE'        => 'NOT ( forum_id = 1)',

'WHERE'        => 
            array('NOT',
                array('forum_id', '=', 1),
            ),

 

Here's something more complex
source: https://github.com/phpbb/phpbb/blob/rel ... m.php#L277

Code: Select all

// original
" WHERE forum_id = 51
    AND (topic_last_post_time >= $min_post_time
        OR topic_type = " . POST_ANNOUNCE . '
        OR topic_type = ' . POST_GLOBAL . ')
    AND 1=1'


// Becomes
'WHERE'        => 
            array('AND',
                array('forum_id', '=', 51),
                array('OR',
                    array('topic_last_post_time', '>=', 1409240111),
                    array('topic_type', '=', POST_ANNOUNCE),
                    array('topic_type', '=', POST_GLOBAL),
                ),
                array('NOT',
                    array('AND',
                        array('topic_poster', '=', 48),
                        array(1, '=', 1),
                    ),
                ),
            )
 

Last example
Source: https://github.com/phpbb/phpbb/blob/rel ... m.php#L445

Code: Select all

// Original
'WHERE'        => '(t.forum_id = 3
                AND t.topic_type = 3) OR
            (' . sql_in_set('t.forum_id', array(1,2,3,4,5,6)) . '
                AND t.topic_type = 4)',
                
// Becomes
        'WHERE'        => 
            array('OR',
                array('AND',
                    array('t.forum_id', '=', 3),
                    array('t.topic_type', '=', 0),
                ),
                array('AND',
                    array('t.forum_id', 'IN', array(1,2,3,4,5,6)),
                    array('t.topic_type', '=', 0),
                ),
            ),
 

Feel free to discuss to your hearts content. Any constructive opinion is welcome.

ticket: https://tracker.phpbb.com/browse/PHPBB3-13652
PR: https://github.com/phpbb/phpbb/pull/3441

[DavidIQ]

Code: Select all

AND NOT ( t.topic_type = 0 )

I know these are just examples but would one ever do this? Is it even accepted as correct by SQL experts (DBAs) or do they consider it bad practice?

[brunoais]

I would never do it. I don't think anyone should do it.
As a DB admin, I consider it a bad move. "<>" instead of "=" should be used instead of the "NOT" operator, IMO.
I just used that because I didn't have any good idea at the time .

[Pony99CA]

I would never do it. I don't think anyone should do it.
As a DB admin, I consider it a bad move. "<>" instead of "=" should be used instead of the "NOT" operator, IMO.
I just used that because I didn't have any good idea at the time .

Perhaps, but suppose the condition inside the NOT was more complicated, like:

Code: Select all

AND NOT ( (t.topic_type = 0  OR blah <> whatever) AND (something else) )

Yes, you can use Boolean logic (like De Morgan's laws) to figure out the equivalent, but sometimes it's easier to NOT the entire thing.

Steve

[brunoais]

Yeah, and phpBB has some of those.
Regardless of that, this is supposedly ready for it. I found no limitations nor errors on that so far.

[brunoais]

I still have one doubt in this feature:
Should the use of the ternary (A type):

Code: Select all

array({left hand}, {operator}, {right hand})

Be enforced for each value operation element?
... Or should it be free to choose (i.e. it is OK to just use a string (1 array element) instead of those 3 array elements)?


One of the things I'm trying to expand in this is to also have it add the "?" automatically and then use prepared statements on the driver itself.
I've checked phpBB's code, nearly everywhere where the query builder is called, its result is directly used with the .query() method.
The only exception is in phpbb\search\fulltext_native->keyword_search(), which can be easily re-written.

If these changes get a positive response, I'm also planning on expanding to other features belonging to the SELECT, FROM, LEFT_JOIN, GROUP_BY and ORDER_BY.

[nickvergessen]

I think doing thinks like this is a step in the right direction.
Maybe you can extract the sql_build_query method into a new class because otherwise that code will just get even bigger.

[brunoais]

So make all the query builder thing into a class of its own inside the DB namespace?

[brunoais]

Extra idea:
Does it make sense to allow separating the CROSS JOIN WHERE clause conditions (conditionals that would be in the ON clause if it was an INNER JOIN) from the rest of the WHERE conditions?
I have 2 possible implementations I believe to be the best choices given the current situation.

1. Place a new possible KEY in the php array that represents the query. All its content is merged with the WHERE clause with an "AND".
2. Place a new possible KEY in the php array for the WHERE clause. All its content is merged with the WHERE clause at its root with an "AND".

This is perfectly backwards compatible. It, however, adds a new tool on how queries are built making it easier for extensions to search on it and modify it at will to add and remove tables from the FROM clause.

For example:

Code: Select all

$sql_ary = array(
    'SELECT'    => 'u.user_id',
    'FROM'        => array(
        'phpbb_users'        => 'u',
        'phpbb_user_group'    => 'ug',
        'phpbb_banlist'    => 'b',
    ),
    'JOIN_ON'     => array(
            'ug'    =>    array('u.user_id', '=', 'ug.user_id'),
            'b'        =>    array('u.user_id', '=', 'b.ban_userid'),
        ),
    'WHERE'        => array('OR',
        array('AND',
            array('ug.user_id', 'IN', array(1, 2, 3, 4)),
            array('ug.group_id', '=', 2),
        ),
        array('AND',
            array('ug.group_id', '=', 1),
            array('b.ban_id', 'IS_NOT', NULL),
        ),
    ),
);
 

Instead of how it is done now:

Code: Select all

$sql_ary = array(
    'SELECT'    => 'u.user_id',
    'FROM'        => array(
        'phpbb_users'        => 'u',
        'phpbb_user_group'    => 'ug',
        'phpbb_banlist'    => 'b',
    ),
    'WHERE'        => array('AND',
        array('u.user_id', '=', 'ug.user_id'),
        array('u.user_id', '=', 'b.ban_userid'),
        array('OR',
            array('AND',
                array('ug.user_id', 'IN', array(1, 2, 3, 4)),
                array('ug.group_id', '=', 2),
            ),
            array('AND',
                array('ug.group_id', '=', 1),
                array('b.ban_id', 'IS_NOT', NULL),
            ),
        ),
    ),
); 

As for which table to place in the KEY of that array, it should be the table that doesn't do the "glue" job in joining the tables.
While processing, all of them are joined using an 'AND'. In cases where an OR is required (I don't remember any, tbh), just use the WHERE clause.
Although here there's only 1 condition, it is part of the objective to allow it to be as complete as the WHERE clause specified above.



Alternatively (I prefer this less but it can be done too):

Code: Select all

    'WHERE'        => array('OR',
        array('AND',
            array('ug.user_id', 'IN', array(1, 2, 3, 4)),
            array('ug.group_id', '=', 2),
        ),
        array('AND',
            array('ug.group_id', '=', 1),
            array('b.ban_id', 'IS_NOT', NULL),
        ),
        'JOIN_ON'     => array('AND',
            array('u.user_id', '=', 'ug.user_id'),
            array('u.user_id', '=', 'b.ban_userid'),
        ),
    ), 

The JOIN_ON name can be anything else. That's just what popped to my mind.

Comments please.

[DavidIQ]

Not sure I like that because of the use of the keyword "JOIN". At first glance it looked like there was a join in addition to the tables under the SELECT statement. I think it might be clearer to determine what the query is doing how it is now.