List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:June 18 2011 10:09am
Subject:review of WL#5860: Make COST_VECT a properly encapsulated C++ class
View as plain text  
Hello,

1)
+  bool is_zero() const
+  {
+    if (io_cost || cpu_cost || import_cost || mem_cost)
+      return false;
+    return true;

return !(io_cost || cpu_cost || import_cost || mem_cost);
is simpler.

2) In definition of class, short comments like:
+  /**
+    Add to IO cost
+  */
could be shorter on one line:
/// Add to IO cost

3) While you're changing DsMrr_impl::choose_mrr_impl(), could you move 
the declaration of "COST_VECT dsmrr_cost" down to right before the call 
to get_disk_sweep_mrr_cost()?

4) Biggest question:
@@ -14105,7 +14107,8 @@ void Optimize_table_order::advance_sj_st

    DBUG_ENTER("Optimize_table_order::advance_sj_state");

-  pos->prefix_cost.convert_from_cost(*current_read_time);
+  DBUG_ASSERT(pos->prefix_cost.is_zero());
+  pos->prefix_cost.add_io(*current_read_time);
I'm not 100% sure that the assertion will never fire, as a POSITION 
serves during evaluation of multiple candidate plans, without, IIUC, any 
resetting in between; could a next pass find a non-zero cost left by a 
previous pass? I suggest asking Roy to double check this.

Otherwise, ok to push.

-- 
Mr. Guilhem Bichot <guilhem.bichot@stripped>
Oracle / MySQL / Optimizer team, Lead Software Engineer
Bordeaux, France
www.oracle.com / www.mysql.com
Thread
review of WL#5860: Make COST_VECT a properly encapsulated C++ classGuilhem Bichot19 Jun
  • Re: review of WL#5860: Make COST_VECT a properly encapsulated C++classRoy Lyseng20 Jun