r50780 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r50779‎ | r50780 | r50781 >
Date:16:58, 19 May 2009
Author:freakolowsky
Status:ok (Comments)
Tags:
Comment:
Preparations for Oracle database abstraction update. Take II.
Replaced hardcoded LIMIT clauses with database function (except in maintenance).
Created unionQueries in Database and overloaded it in DatabaseOracle (not commited yet).
Replaced all UNION clauses with function calls (except in maintenance).
Modified paths:
  • /trunk/phase3/includes/db/Database.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialRecentchanges.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialRecentchangeslinked.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/db/Database.php
——@@ -1829,6 +1829,19 @@
18301830 }
18311831
18321832 /**
 1833+ * Construct a UNION query
 1834+ * This is used for providing overload point for other DB abstractions
 1835+ * not compatible with the MySQL syntax.
 1836+ * @param $sqls Array: SQL statements to combine
 1837+ * @param $all Boolean: use UNION ALL
 1838+ * @return String: SQL fragment
 1839+ */
 1840+ function unionQueries($sqls, $all) {
 1841+ $glue = $all ? ') UNION ALL (' : ') UNION (';
 1842+ return '('.implode( $glue, $sqls ) . ')';
 1843+ }
 1844+
 1845+ /**
18331846 * Returns an SQL expression for a simple conditional.
18341847 * Uses IF on MySQL.
18351848 *
Index: trunk/phase3/includes/specials/SpecialRecentchangeslinked.php
——@@ -155,9 +155,10 @@
156156 $sql = $subsql[0];
157157 else {
158158 // need to resort and relimit after union
159 - $sql = "(" . implode( ") UNION (", $subsql ) . ") ORDER BY rc_timestamp DESC LIMIT {$limit}";
 159+ $sql = $dbr->unionQueries($subsql, false).' ORDER BY rc_timestamp DESC';
 160+ $sql = $dbr->limitResult($sql, $limit, false);
160161 }
161 -
 162+
162163 $res = $dbr->query( $sql, __METHOD__ );
163164
164165 if( $res->numRows() == 0 )
Index: trunk/phase3/includes/specials/SpecialRecentchanges.php
——@@ -328,7 +328,8 @@
329329 'USE INDEX' => array('recentchanges' => 'new_name_timestamp') ),
330330 $join_conds );
331331 # Join the two fast queries, and sort the result set
332 - $sql = "($sqlNew) UNION ($sqlOld) ORDER BY rc_timestamp DESC LIMIT $limit";
 332+ $sql = $dbr->unionQueries(array($sqlNew, $sqlOld), false).' ORDER BY rc_timestamp DESC';
 333+ $sql = $dbr->limitResult($sql, $limit, false);
333334 $res = $dbr->query( $sql, __METHOD__ );
334335 }
335336

Comments

#Comment by Simetrical (talk | contribs)   17:36, 19 May 2009

Note our coding conventions with regard to spacing. E.g., 

$dbr->unionQueries( array( $sqlNew, $sqlOld ), false )

not 

$dbr->unionQueries(array($sqlNew, $sqlOld), false)

Also, try avoiding boolean parameters. It's hard to figure out from looking at the source code what the "false" does, without having to look up the method's documentation. Even if you remember that it has to do with ALL, is it true for ALL or false for ALL? I'd recommend something more like 

	 * @param $options Null, or the string 'all' to use UNION ALL
	 * @return String: SQL fragment
	 */
	function unionQueries( $sqls, $all = null ) {

Then it would normally be called as just unionQueries( $sqls ), and if ALL were desired it would be unionQueries( $sqls, 'all' ), which is nice and comprehensible.

Status & tagging log