my suggestion is to change the type of join for the module from inner to left otherwise, nodes which don't have associated radioactivity information (yet) will never appear in the view

please note that the patch also contains the fix for http://drupal.org/node/251651

CommentFileSizeAuthor
#7 radioactivity-module-251656-7.patch5.44 KBskiminki
#4 radioactivity-joins.patch5.19 KBguardian
#3 radioactivity-module-251656-3.patch2.95 KBskiminki
radioactivity.patch1 KBguardian

Comments

skiminki’s picture

skiminki commented

Is this required by a real use case? I ask this because inner joins should be faster than left outer joins.

In medium/large sites you can expect 10:1, 100:1 or even greater ratio of nodes per rows in radioactivity (per decay profile) which justifies the inner join. This is the case in a site where this module was originally written for. If you really need left join, I see two possibilities:

  • I wonder if the choice of left/inner join per view can be made user-configurable in the views UI?
  • Have separate views criteria for inner and left joins, like 'Radioactivity: Node energy (@e), include zeros' and 'Radioactivity: Node energy (@e), don't include zeros (faster)'

We could also make this configurable in the radioactivity admin menu, but I wouldn't want to make this kind of setting global.

guardian’s picture

guardian commented

my use case is the following: i have a view that sorts the nodes by radioactivity then by stickiness then by created time

for my home page, i extract the top node of the view and render it highlighted in a separate div so it makes sense to always have a node returned by the view

this enables me to give more importance to the latest sticky node of the site in case of low activity, but as well keep track of the currently most viewed node in case your site has been digged

if you think that performance might be a problem for medium / large sites, i would prefer having an option on the criterium like "All" and "Only Radioactive". having this option means we find a way to change the type of the join dynamically when the view is built. in case we can't do that, having 2 criteria per profile is an acceptable fallback

thx for the follow up ! i'm eager to see this polished :)

skiminki’s picture

skiminki commented
Status: Needs review » Needs work
StatusFileSize
new radioactivity-module-251656-3.patch2.95 KB

Ok, I looked briefly into this, and attached is a patch that provides 'all' and 'radioactive only' versions of field/sort/filter criteria. However, I'm not fully comfortable with this for two reasons:

  1. I'd rather have the join type selectable in views ui. I couldn't figure out how to do that. Maybe someone more familiar with views could help?
  2. The ALL option doesn't currently work with PostgreSQL, because NULLs have infinity value in sorting. In SQL, this can be remedied using 'ORDER BY COALESCE(energy, 0)' but again, I couldn't get this working properly. BTW, I'm not sure at all that ORDER BY COALESCE(...) is able to do good job performance-wise... but I might be wrong.

I'd like to have at least the second item sorted out before inclusion. So, until someone's up to the task...

guardian’s picture

guardian commented
StatusFileSize
new radioactivity-joins.patch5.19 KB

please review this one, in the end it seems to work like intended

disclaimer: i have nearly no experience in databases in general but i googled some information about the problem you mentioned regarding PostgreSQL, also this is the first time i'm coding a sort criterion for views

guardian’s picture

guardian commented
Status: Needs work » Needs review
guardian’s picture

guardian commented

works as intended on mysql, since i don't have postgresql

skiminki’s picture

skiminki commented
StatusFileSize
new radioactivity-module-251656-7.patch5.44 KB

Ok, I used the patch in comment 4 as a basis and came up with the attached patch. There's some notable changes (in addition to white space changes):

  • Field, filter and sort definitions has been split to use different join aliases. This means the join type defined in sort does not interfere with join types in field and filter.
  • Field uses left outer join, as energy can be considered as just another node property when displaying a node. So if it's not there the node is still displayed with null energy.
  • Filter definitions use inner join for speed reasons. This could be made configurable like the join type of sort definitions.
  • Field click sorting uses COALESCE for PostgreSQL compatibility.

Please, test this patch against the current CVS HEAD ( http://cvs.drupal.org/viewvc.py/drupal/contributions/modules/radioactivi... )

Note that existing views with radioactive energy will get broken after applying this patch.

guardian’s picture

guardian commented

works for me

you mentioned "field click sorting uses COALESCE for PostgreSQL compatibility" - i'm not experienced in PostgreSQL at all but the reason why my previous patch uses $query->orderby[] = "$table.$field IS NOT NULL $order, $table.$field $order"; and not COALESCE is that google told me it was a way not to have NULLs at the top of the results when sorting

maybe your implementation of 

 function radioactivity_field_sort_handler_energy($fielddata, $fieldinfo) {
  return 'COALESCE('.$fielddata['tablename'].'.energy, 0)';
}

could as well use IS NOT NULL instead of COALESCE depending on the performance ? you decide, as i said i don't know PostgreSQL and can't benchmark it right now

apart from the point mentioned above, ready to be committed ?

skiminki’s picture

skiminki commented

Ok, the radioactivity-module-251656-7.patch was almost ready for commit. Two small adjustments:

  • I removed the COALESCE from the mysql case. With PostgreSQL we just need to rely on its optimizer for now. AFAICT, we would need to write a real handler to be able to use the IS NOT NULL trick. Anyway, field click sorting and PostgreSQL combination is not probably that important, even if there was a performance penalty.
  • Renamed energy_f to energy in 'fields' section. Allows non-hardcoded sort handler.

With these two, here's the commit: http://drupal.org/cvs?commit=114748

Thanks!

skiminki’s picture

skiminki commented
Status: Needs review » Fixed
Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.