bugPSPP - Bugs: bug #11975, AGGREGATE should support MEDIAN...

 
 

bug #11975: AGGREGATE should support MEDIAN function

Submitted by:  Ben Pfaff <blp>
Submitted on:  Sun 13 Feb 2005 11:47:59 PM UTC  
 
Category:  Syntax Parser Severity:  1 - Wishlist
Status:  Fixed Assigned to:  None
Open/Closed:  Closed Release:  Post-0.6.0
Effort:  0.00

Add a New Comment (Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Fri 19 Sep 2008 03:03:42 PM UTC, comment #10: 

Looks good!

Ben Pfaff <blp>
Project Administrator
Fri 19 Sep 2008 11:07:29 AM UTC, comment #9: 

You're right (of course).

New patch attached.

(file #16524)

John Darrington <jmd>
Project Administrator
Fri 19 Sep 2008 03:54:24 AM UTC, comment #8: 

The patch looks good.  A few comments:

  • I think that the function dict_get_case_weight() could usefully simplify a small bit of logic.
  • My thought was actually slightly different from what you implemented: I had the notion that the contents of the while loop that your patch inserts into initialize_aggregate_info() would actually be put into accumulate_aggregate_info().  Then there is no additional data pass in initialize_aggregate_info(), because we use the one that is already happening.
Ben Pfaff <blp>
Project Administrator
Thu 18 Sep 2008 04:47:54 AM UTC, comment #7: 

Ben's suggestion turned out to be easier, so I've done it that way.
New patch attached.

(file #16513)

John Darrington <jmd>
Project Administrator
Tue 02 Sep 2008 05:30:15 AM UTC, comment #6: 

>How about I make a translator which drops all but the subject
>variable and the weight variable?


That would definitely be an improvement, in the situation that I raised.  OK.

Ben Pfaff <blp>
Project Administrator
Tue 02 Sep 2008 05:04:07 AM UTC, comment #5: 

You're right. I hadn't considered that.

How about I make a translator which drops all but the subject variable and the weight variable?

This reader can then be passed to sort_execute.

John Darrington <jmd>
Project Administrator
Tue 02 Sep 2008 04:07:33 AM UTC, comment #4: 

>Although there is opportunity to reduce the total number of
>passes, we can't get around the need to sort and iterate for each
>median being calculated.


I cannot dispute that.

>With this patch, the total number of sorts is N and the total
>number of passes is (N+1), where N is the number of medians to be
>calculated. With some rework, it could be got down to N sorts and
>N passes, but I'm not sure if it's worth the effort.


However, if I'm reading the code correctly, it sorts all of the data N times, whereas it only needs to sort a single column N times.  That means it's doing a factor of M more work than necessary, where M is the number of variables.  And there is the very good possibility that a single column could fit in memory even when all the data cannot.

You're right that it might not be worth it: I don't know whether anyone out there is doing lots of AGGREGATE with MEDIAN functions.  If you want to commit it as-is, though, would you mind adding a comment about the kind of optimization that is possible?

Ben Pfaff <blp>
Project Administrator
Tue 02 Sep 2008 01:58:56 AM UTC, comment #3: 

Although there is opportunity to reduce the total number of passes, we can't get around the need to sort and iterate for each median being calculated.

With this patch, the total number of sorts is N and the total number of passes is (N+1), where N is the number of medians to be calculated.  With some rework, it could be got down to N sorts and N passes, but I'm not sure if it's worth the effort.

John Darrington <jmd>
Project Administrator
Mon 01 Sep 2008 06:35:57 PM UTC, comment #2: 

This looks to me like it implements median correctly, but doesn't it add an additional data-reading pass for every median being calculated?  I think that this extra pass could be eliminated by, instead of using sort_execute(), calling sort_create_writer(), then writing a case to the writer in accumulate_aggregate_info(), then obtaining the reader with casewriter_make_reader() in dump_aggregate_info().

Ben Pfaff <blp>
Project Administrator
Sun 24 Aug 2008 07:32:07 AM UTC, comment #1: 

This patch implements this.

It must be applied on top of #file 16351

(file #16352)

John Darrington <jmd>
Project Administrator
Sun 13 Feb 2005 11:47:59 PM UTC, original submission:  

AGGREGATE should support the MEDIAN aggregation function.

Ben Pfaff <blp>
Project Administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #16524:  agg-median-3.patch added by jmd (22KiB - text/x-diff)
file #16513:  agg-median-2.patch added by jmd (23KiB - text/x-diff)

 

Digest:
   bug dependencies.

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jmd (Updated the item)
  • -email is unavailable- added by blp (Updated the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2008-09-20 jmd StatusReady for Test/Review => Fixed
        Open/ClosedOpen => Closed
    2008-09-19 jmd Attached File- => Added agg-median-3.patch, #16524
    2008-09-19 jmd Attached File#16352 => Removed
    2008-09-18 jmd Attached File- => Added agg-median-2.patch, #16513
    2008-08-26 jmd StatusNone => Ready for Test/Review
    2008-08-24 jmd Attached File- => Added agg-median.patch, #16352
    2006-12-20 jmd Dependencies- => Depends on bugs #18568
    2006-12-18 blp ReleaseNone => Post-0.6.0
    2006-07-31 blp ReleaseBefore 0.6.0 => None
    2005-05-07 blp ReleaseNone => Before 0.6.0

    Back to the top


    Powered by Savane 3.5