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 ParserSeverity: 1 - Wishlist
Status: FixedAssigned to: None
Open/Closed: ClosedRelease: 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 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 File(s):
   
   
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
  • -unavailable- added by jmd (Updated the item)
  • -unavailable- added by blp (Updated the item)
  •  

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

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 11 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sat 20 Sep 2008 01:46:06 AM UTCjmdStatusReady for Test/Review=>Fixed
      Open/ClosedOpen=>Closed
    Fri 19 Sep 2008 11:07:29 AM UTCjmdAttached File-=>Added agg-median-3.patch, #16524
    Fri 19 Sep 2008 11:06:39 AM UTCjmdAttached File#16352=>Removed
    Thu 18 Sep 2008 04:47:54 AM UTCjmdAttached File-=>Added agg-median-2.patch, #16513
    Tue 26 Aug 2008 11:43:16 AM UTCjmdStatusNone=>Ready for Test/Review
    Sun 24 Aug 2008 07:32:07 AM UTCjmdAttached File-=>Added agg-median.patch, #16352
    Wed 20 Dec 2006 09:43:55 PM UTCjmdDependencies-=>Depends on bugs #18568
    Mon 18 Dec 2006 05:20:44 PM UTCblpReleaseNone=>Post-0.6.0
    Mon 31 Jul 2006 02:01:05 AM UTCblpReleaseBefore 0.6.0=>None
    Sat 07 May 2005 04:30:33 AM UTCblpReleaseNone=>Before 0.6.0

    Back to the top


    Powered by Savane 3.1-cleanup1