Bug 126228

*** Bug 126229 has been marked as a duplicate of this bug. ***

I'd thought of using the coalesce function but I didn't want to introduce more IF statements into that function (setOptions). At the moment, the latest SVN ignores NULLs. 

The result of this is:
1) we get a similar result set to earlier versions where we didn't use a LEFT JOIN.
2) unfortunately, like before, we don't get a _real_ result set, because we ignore the NULLs.

I'll take a look at your patch and make sure it doesn't break MySQL et al.

To expand on my previous comments.

Point #2 is actually redundant, because postgres's AVG function _ignores_ NULLs, so an 
    AVG (1, 2, NULL) returns 3/2=1.5, instead of 3/3=1.

To make matters worse, so far, the potential NULLs which would need to be 'coalesced' occur for:
- score
- playcounter
- rating

If this increases in the future, we have to expand our IF condition.... :-(

I think it's best if we wait until postgres accept the SQL-standard ORDER BY x NULLS LAST syntax before we apply any patches to the sorting code.

Ok i just checked on mysql the coalesce function seems to be the same there and also the
syntax  of writing statistics.percentage is NULL, statistics.percentage DESC
gives the same result (null at last).

The following worked on my db using either mysql or postgresql giving the same result.

SELECT statistics.url, statistics.percentage, statistics.rating, tags.sampler
FROM tags LEFT JOIN statistics on tags.url=statistics.url
--ORDER BY statistics.percentage is NULL, statistics.percentage DESC
ORDER BY coalesce(statistics.percentage,0) DESC
LIMIT 20;

If i understand the coalesce function right,  all it does is give back the virst value not NULL, and by setting the second value to 0 all NULL should become 0.
So we have two aspects 
1. the cost of this coalsce 
2. and is there a problem with replacing NULL with 0 in our queries

One question though, when talking about latest svn, ignoring NULLs. I've  had this with svn (1 max 2 days old) where there changes with respect to NULLs or am i misunderstanding you?

I made the changes to ignore NULLs in latest SVN. 

At the moment, we filter out all NULL percentages, so the query looks something like:

SELECT statistics.url, statistics.percentage, statistics.rating, tags.sampler 
 FROM tags LEFT JOIN statistics on tags.url=statistics.url 
WHERE statistics.percentage >= 0
ORDER BY statistics.percentage DESC LIMIT 20;

which IMHO returns the same results as what you're specifying.

Ah i see (haven't checked out today) thanks.
I see no problem with filtering out NULL  values and therefore tracks i haven't played yet, because 
showing those as favourite doesn't seem right to me either. So i think this is not a bad solution.

P.S.: ignoring NULL values in aggregation functions as mentioned above is afaik sql standard
P.P.S i see you didn#t meen this ^ when talking about ignoring NULLs here :P

Created attachment 15783 [details]
Fix for NULL values

Simon,

I've gone thru MySQL, SQLite & PostgreSQL docs and it seems you're right about
the global use of the coalesce function.

Please test this patch (against latest SVN)

Peter

ok it seems to work at the first look, although i see things like this:
So there is definitively the need to filter for which columns it's used.

 [CollectionDB] POSTGRESQL QUERY FAILED: ERROR:  COALESCE types text and integer cannot be matched
amarok: 
amarok: FAILED QUERY: SELECT DISTINCT album.name,album.id,year.name as __discard ,LOWER( album.name) as __discard  FROM tags LEFT JOIN album ON album.id=tags.album LEFT JOIN year ON year.id=tags.year  WHERE true AND ( false OR tags.artist  ILIKE '1862' ESCAPE '/'  ) AND tags.sampler = false  ORDER BY COALESCE (year.name, 0) DESC ,LOWER( album.name ) ;
amarok:     [CollectionDB] POSTGRESQL QUERY FAILED: ERROR:  COALESCE types text and integer cannot be matched
amarok: 
amarok: FAILED QUERY: SELECT DISTINCT album.name,album.id,year.name as __discard ,LOWER( album.name) as __discard  FROM tags LEFT JOIN album ON album.id=tags.album LEFT JOIN year ON year.id=tags.year  WHERE true AND ( false OR tags.artist  ILIKE '1862' ESCAPE '/'  ) AND tags.sampler = true  ORDER BY COALESCE (year.name, 0) DESC ,LOWER( album.name ) ;

Yep, doing that now.

SVN commit 534217 by pndiku:

CCBUG: 126228
CCMAIL: s7mon@web.de

Use the coalesce function when sorting. NULLs will thus be sorted last.
More intelligent than simply filtering out NULL values on a case-by-case
basis.

Thanks to Simon <s7mon AT web DOT de> for pointing me in the right
direction.



 M  +21 -19    collectiondb.cpp  
 M  +0 -2      contextbrowser.cpp  
 M  +0 -2      playlistbrowser.cpp  
 M  +0 -5      statistics.cpp  


--- trunk/extragear/multimedia/amarok/src/collectiondb.cpp #534216:534217
@@ -4911,21 +4911,19 @@
          table & tabYear )
         b = false;
 
+	// only coalesce for certain columns
+	bool c = false;
+    if ( value & valScore || value & valRating || value & valPlayCounter || value & valPercentage )
+		c = true;
+
     if ( !m_sort.isEmpty() ) m_sort += ",";
     if ( b ) m_sort += "LOWER( ";
-    if ( table & tabYear ) m_sort += "(";
+    if ( c ) m_sort += "COALESCE( ";
 
     m_sort += tableName( table ) + ".";
     m_sort += valueName( value );
 
-    if (CollectionDB::instance()->getType() == DbConnection::postgresql)
-    {
-        if ( table & tabYear ) m_sort += ")";
-    }
-    else
-    {
-        if ( table & tabYear ) m_sort += "+0)";
-    }
+    if ( c ) m_sort += ", 0 )";
 
     if ( b ) m_sort += " ) ";
     if ( descending ) m_sort += " DESC ";
@@ -4956,23 +4954,27 @@
          table & tabYear )
         b = false;
 
+	// only coalesce for certain columns
+	bool c = false;
+    if ( value & valScore || value & valRating || value & valPlayCounter || value & valPercentage )
+		c = true;
+
     if ( !m_sort.isEmpty() ) m_sort += ",";
     //m_sort += functionName( function ) + "(";
     if ( b ) m_sort += "LOWER( ";
-    if ( table & tabYear ) m_sort += "(";
+    if ( c && CollectionDB::instance()->getType() != DbConnection::mysql) m_sort += "COALESCE( ";
 
-    QString columnName = functionName( function )+tableName( table )+valueName( value );
-    m_sort += columnName;
+	QString columnName;
 
     if (CollectionDB::instance()->getType() == DbConnection::postgresql)
-    {
-        if ( table & tabYear ) m_sort += ")";
-    }
-    else
-    {
-        if ( table & tabYear ) m_sort += "+0)";
-    }
+		columnName = functionName( function )+"("+tableName( table )+"."+valueName( value )+")";
+	else
+		columnName = functionName( function )+tableName( table )+valueName( value );
 
+    m_sort += columnName;
+
+	if ( c && CollectionDB::instance()->getType() != DbConnection::mysql) m_sort += ", 0 )";
+
     if ( b ) m_sort += " ) ";
     //m_sort += " ) ";
     if ( descending ) m_sort += " DESC ";
--- trunk/extragear/multimedia/amarok/src/contextbrowser.cpp #534216:534217
@@ -1375,7 +1375,6 @@
     qb.addReturnFunctionValue( QueryBuilder::funcAvg, QueryBuilder::tabStats, QueryBuilder::valPercentage );
     qb.addReturnValue( QueryBuilder::tabArtist, QueryBuilder::valID );
     qb.sortByFunction( QueryBuilder::funcAvg, QueryBuilder::tabStats, QueryBuilder::valPercentage, true );
-    qb.addFilter( QueryBuilder::tabStats, QueryBuilder::valPercentage, QString::number( -1 ), QueryBuilder::modeGreater, true);
     qb.excludeMatch( QueryBuilder::tabAlbum, i18n( "Unknown" ) );
     qb.groupBy( QueryBuilder::tabAlbum, QueryBuilder::valID );
     qb.groupBy( QueryBuilder::tabArtist, QueryBuilder::valID );
@@ -2001,7 +2000,6 @@
     qb.addReturnValue( QueryBuilder::tabStats, QueryBuilder::valScore );
     qb.addReturnValue( QueryBuilder::tabStats, QueryBuilder::valRating );
     qb.addMatch( QueryBuilder::tabSong, QueryBuilder::valArtistID, QString::number( artist_id ) );
-    qb.addFilter( QueryBuilder::tabStats, QueryBuilder::valPercentage, QString::number( -1 ), QueryBuilder::modeGreater, true);
     qb.sortBy( QueryBuilder::tabStats, QueryBuilder::valPercentage, true );
     qb.setLimit( 0, 10 );
     values = qb.run();
--- trunk/extragear/multimedia/amarok/src/playlistbrowser.cpp #534216:534217
@@ -584,7 +584,6 @@
 
     /********** Favorite Tracks **************/
     qb.initSQLDrag();
-    qb.addFilter( QueryBuilder::tabStats, QueryBuilder::valPercentage, QString::number( -1 ), QueryBuilder::modeGreater, true);
     qb.sortBy( QueryBuilder::tabStats, QueryBuilder::valPercentage, true );
     qb.setLimit( 0, 15 );
 
@@ -601,7 +600,6 @@
 
     /********** Most Played **************/
     qb.initSQLDrag();
-    qb.addFilter( QueryBuilder::tabStats, QueryBuilder::valPlayCounter, QString::number( -1 ), QueryBuilder::modeGreater, true);
     qb.sortBy( QueryBuilder::tabStats, QueryBuilder::valPlayCounter, true );
     qb.setLimit( 0, 15 );
 
--- trunk/extragear/multimedia/amarok/src/statistics.cpp #534216:534217
@@ -320,7 +320,6 @@
         qb.addReturnValue( QueryBuilder::tabSong, QueryBuilder::valURL );
         qb.addReturnValue( QueryBuilder::tabStats, QueryBuilder::valScore );
         qb.addReturnValue( QueryBuilder::tabStats, QueryBuilder::valRating );
-        qb.addFilter( QueryBuilder::tabStats, QueryBuilder::valScore, QString::number( -1 ), QueryBuilder::modeGreater, true);
         qb.setGoogleFilter( QueryBuilder::tabSong | QueryBuilder::tabArtist, m_filter );
         qb.sortBy( QueryBuilder::tabStats, QueryBuilder::valPercentage, true );
         qb.setLimit( 0, 50 );
@@ -354,7 +353,6 @@
         qb.addReturnValue( QueryBuilder::tabSong, QueryBuilder::valURL );
         qb.addReturnValue( QueryBuilder::tabStats, QueryBuilder::valPlayCounter );
         qb.addReturnValue( QueryBuilder::tabStats, QueryBuilder::valRating );
-        qb.addFilter( QueryBuilder::tabStats, QueryBuilder::valPlayCounter, QString::number( -1 ), QueryBuilder::modeGreater, true);
         qb.setGoogleFilter( QueryBuilder::tabSong | QueryBuilder::tabArtist, m_filter );
         qb.sortBy( QueryBuilder::tabStats, QueryBuilder::valPlayCounter, true );
         qb.setLimit( 0, 50 );
@@ -387,7 +385,6 @@
         qb.addReturnValue( QueryBuilder::tabArtist, QueryBuilder::valName );
         qb.addReturnFunctionValue( QueryBuilder::funcAvg, QueryBuilder::tabStats, QueryBuilder::valPercentage );
         qb.addReturnFunctionValue( QueryBuilder::funcAvg, QueryBuilder::tabStats, QueryBuilder::valRating );
-        qb.addFilter( QueryBuilder::tabStats, QueryBuilder::valPercentage, QString::number( -1 ), QueryBuilder::modeGreater, true);
         qb.sortByFunction( QueryBuilder::funcAvg, QueryBuilder::tabStats, QueryBuilder::valPercentage, true );
         qb.setGoogleFilter( QueryBuilder::tabArtist, m_filter );
         qb.groupBy( QueryBuilder::tabArtist, QueryBuilder::valName);
@@ -425,7 +422,6 @@
         qb.addReturnFunctionValue( QueryBuilder::funcAvg, QueryBuilder::tabStats, QueryBuilder::valPercentage );
         qb.addReturnFunctionValue( QueryBuilder::funcAvg, QueryBuilder::tabStats, QueryBuilder::valRating );
         qb.addReturnValue( QueryBuilder::tabSong, QueryBuilder::valIsCompilation );
-        qb.addFilter( QueryBuilder::tabStats, QueryBuilder::valPercentage, QString::number( -1 ), QueryBuilder::modeGreater, true);
         qb.setGoogleFilter( QueryBuilder::tabAlbum | QueryBuilder::tabArtist, m_filter );
         qb.sortByFunction( QueryBuilder::funcAvg, QueryBuilder::tabStats, QueryBuilder::valPercentage, true );
         qb.excludeMatch( QueryBuilder::tabAlbum, i18n( "Unknown" ) );
@@ -469,7 +465,6 @@
         qb.addReturnValue( QueryBuilder::tabGenre, QueryBuilder::valName );
         qb.addReturnFunctionValue( QueryBuilder::funcAvg, QueryBuilder::tabStats, QueryBuilder::valPercentage );
         qb.addReturnFunctionValue( QueryBuilder::funcAvg, QueryBuilder::tabStats, QueryBuilder::valRating );
-        qb.addFilter( QueryBuilder::tabStats, QueryBuilder::valPercentage, QString::number( -1 ), QueryBuilder::modeGreater, true);
         qb.setGoogleFilter( QueryBuilder::tabGenre, m_filter );
         qb.sortByFunction( QueryBuilder::funcAvg, QueryBuilder::tabStats, QueryBuilder::valScore, true );
         qb.groupBy( QueryBuilder::tabGenre, QueryBuilder::valName);

Just saw that in collectionbrowser  with the first patch: i couldn't open an artist (speaking + > albums and titles from this artist showing up)

 [CollectionDB] POSTGRESQL QUERY FAILED: ERROR:  for SELECT DISTINCT, ORDER BY expressions must appear in select list
amarok: 
amarok: FAILED QUERY: SELECT DISTINCT tags.title,tags.url,LOWER( tags.title) as __discard ,LOWER( tags.discnumber) as __d                                   iscard ,tags.track as __discard ,LOWER( tags.url) as __discard  FROM tags LEFT JOIN artist ON artist.id=tags.artist  WHERE true AND ( false OR artist.name  ILIKE '4Lyn' ESCAPE '/'  )  ORDER BY LOWER( tags.title ) ,LOWER( tags.discnumber ) ,CO                                   ALESCE (tags.track, 0),LOWER( tags.url ) ;

I think we can mark this solved now - everything is now working as it should (with respect to this bug)