ilovesoup / hyracks

Automatically exported from code.google.com/p/hyracks
Apache License 2.0
0 stars 0 forks source link

MinMaxAggregatorFactory init #52

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
What steps will reproduce the problem?
1.Any MinMaxAggregatorFactory test case for Min aggregation, i.e. type = false

What is the expected output? What do you see instead?
Min value from tuples. However, 0 is returned.

What version of the product are you using? On what operating system?
0.1.8, ubuntu 11.04

Please provide any additional information below.
The init function initializes minmax as 0. however, the values can be 
negatives. One way to initialize minmax would be as follows:

                minmax = type ? Float.MIN_VALUE : Float.MAX_VALUE;

Original issue reported on code.google.com by manish.h...@gmail.com on 2 Dec 2011 at 5:28

GoogleCodeExporter commented 9 years ago
Further, is there a limitation on the data type that you can aggregate on? It 
is implemented as a float. I'm not too sure how SerializerDeserializer works, 
but for negative numbers wouldn't reading int as a float be a problem?

Original comment by manish.h...@gmail.com on 2 Dec 2011 at 7:10

GoogleCodeExporter commented 9 years ago
Thanks for the bug report. Actually Float.MIN_VALUE is not a fix to the 
negative min value either, since FLOAT.MIN_VALUE is the smallest positive 
number a float can represent. 

A quick fix may be -1*FLOAT.MAX_FALUE, but as you said, handle all numbers as 
float may not be a final choice (for efficiency and storage issue). I will try 
to find other better alternatives for this.  

Original comment by jarod...@gmail.com on 2 Dec 2011 at 5:14

GoogleCodeExporter commented 9 years ago
Could you not initialize the aggregator with the input value to the initialize 
function?

Original comment by vinay...@gmail.com on 2 Dec 2011 at 5:25

GoogleCodeExporter commented 9 years ago
Jarod - ah yes, my bad. -1*Float.MAX_VALUE should do the trick. What's your 
idea to abstract the data type?

Vinayak - As of now the init function in the aggregators takes only the id of 
the aggregate field to be aggregated. Also, the init function is called during 
the build/processing of the left table/entity. That would also need to be 
changed if the input value needs to be passed to it - involving all the group 
operator descriptors and our groupjoin operator descriptor as well. If the init 
is changed, it won't be a big deal of rework in the groupjoin.

Original comment by manish.h...@gmail.com on 3 Dec 2011 at 3:12

GoogleCodeExporter commented 9 years ago
Actually we are working on a complete fix for this issue now. This may
have some changes on the existing aggregator interface, but for your
joingroupby operator, you can stick to the existing interface for your
development.

On Dec 2, 2011, at 7:13 PM, "hyracks@googlecode.com"
<hyracks@googlecode.com> wrote:

Original comment by jarod...@gmail.com on 3 Dec 2011 at 4:00

GoogleCodeExporter commented 9 years ago
This issue was updated by revision r860.

Added new aggregator interfaces and group operator implementations, in order to 
merge the old interfaces.

A simple integer sum aggregator is added to show the usage of the new 
interfaces.

Original comment by jarod...@gmail.com on 7 Dec 2011 at 12:54

GoogleCodeExporter commented 9 years ago
This issue was updated by revision r861.

Added integration test for new aggregator interface: contains tests for sum 
aggregation on both in-mem and external group operators.

Original comment by jarod...@gmail.com on 7 Dec 2011 at 12:57

GoogleCodeExporter commented 9 years ago
This issue was updated by revision r862.

Added AVG aggregator using new interface; added two integration tests for AVG 
aggregator.

Original comment by jarod...@gmail.com on 7 Dec 2011 at 2:03

GoogleCodeExporter commented 9 years ago
This issue was updated by revision r863.

Added Min/Max String aggregator using the new interface; added two test cases 
for both in-mem and external hash group operators.

Original comment by jarod...@gmail.com on 7 Dec 2011 at 5:48

GoogleCodeExporter commented 9 years ago
This issue was updated by revision r864.

Replaced the aggregate state interface by using a single aggregate state class.

Original comment by jarod...@gmail.com on 7 Dec 2011 at 5:57

GoogleCodeExporter commented 9 years ago
This issue was updated by revision r879.

- rewrote the aggregator interface to create a state factory; 
- added a wrapper interface for aggregation and changed the original 
aggregators to be field aggregators, and added a multi-field aggregator wrapper;
- rewrote test cases for new interface; 
- added count field aggregator.

Original comment by jarod...@gmail.com on 11 Dec 2011 at 3:15

GoogleCodeExporter commented 9 years ago
Is this issue still valid? If it has been fixed can we close it?

Original comment by vinay...@gmail.com on 3 Feb 2012 at 1:20

GoogleCodeExporter commented 9 years ago
The bug has been fixed since r1076.

Original comment by jarod...@gmail.com on 3 Feb 2012 at 2:40