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

 
 

bug #11975: AGGREGATE should support MEDIAN function

Submitter:  Ben Pfaff <blp>
Submitted:  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
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

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

Looks good!

Ben Pfaff <blp>
Group 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>
Group 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>
Group 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>
Group 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>
Group 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>
Group 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>
Group 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>
Group 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>
Group 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>
Group administrator
Sun 13 Feb 2005 11:47:59 PM UTC, original submission:  

AGGREGATE should support the MEDIAN aggregation function.

Ben Pfaff <blp>
Group 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)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    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.13-f8d8.
    Corresponding source code