Skip to content
This repository was archived by the owner on May 12, 2021. It is now read-only.

TAJO-1265: min(), max() does not handle null properly#315

Closed
sirpkt wants to merge 11 commits intoapache:masterfrom
sirpkt:TAJO-1265
Closed

TAJO-1265: min(), max() does not handle null properly#315
sirpkt wants to merge 11 commits intoapache:masterfrom
sirpkt:TAJO-1265

Conversation

@sirpkt
Copy link
Copy Markdown
Contributor

@sirpkt sirpkt commented Dec 22, 2014

min() and max() handle null value separately from non-null values.
It computes min() or max() among non-null values first.
If no non-null value exists, they return null value.
Implementation style of min() and max() is also changed that
there are abstract classes of Min and Max common for all the data types
and only type specific methods are implemented for each type.

@hyunsik
Copy link
Copy Markdown
Member

hyunsik commented Dec 22, 2014

This patch seems to not pass 'mvn clean install'.

@sirpkt
Copy link
Copy Markdown
Contributor Author

sirpkt commented Dec 22, 2014

@hyunsik Sorry for the incomplete patch.
There are many test cases related with max(), min() and I missed the testing for all those cases.
I apologize for stealing your time by faulty patch.
I'll check and update the patch.

@sirpkt
Copy link
Copy Markdown
Contributor Author

sirpkt commented Dec 24, 2014

I updated the patch as followings

  • null handling bug fix in getPartialResult() of Min and Max
  • getEmptyTuple() in DistinctGroupbySortAggregationExec.java is changed to make Tuple according to the type of aggregation functions: currently, null datum for min and max and 0 for other types.
  • Test result is fixed as correct value

mvn clean install passed.

sum() and avg() also need to be changed to handle null differently from non-null values.
I'll make another issue about sum() and avg().

@sirpkt
Copy link
Copy Markdown
Contributor Author

sirpkt commented Jan 5, 2015

I also modified sum() and avg() to handle null properly.

@hyunsik
Copy link
Copy Markdown
Member

hyunsik commented Jan 5, 2015

I'll review it tonight. Thank you for your contribution.

@hyunsik
Copy link
Copy Markdown
Member

hyunsik commented Jan 5, 2015

The patch looks good to me. It corrects the wrong implementation. There are some trivial things that I need to check. I'll finish the review by tomorrow.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused import

@hyunsik
Copy link
Copy Markdown
Member

hyunsik commented Jan 6, 2015

+1
Thank you for finding these bugs. It seems to correct wrong null handling behavior of aggregation operators.

@hyunsik
Copy link
Copy Markdown
Member

hyunsik commented Jan 6, 2015

Hi @sirpkt,

I leave some trivial comments. Because they are trivial, you can commit after you remove them.

@sirpkt
Copy link
Copy Markdown
Contributor Author

sirpkt commented Jan 8, 2015

Thank you for the review, @hyunsik.
I removed unused imports.
I checked 'mvn clean install', however, I'll commit after Travis CI build is done without problem.

@asfgit asfgit closed this in a1e0328 Jan 8, 2015
@sirpkt
Copy link
Copy Markdown
Contributor Author

sirpkt commented Jan 8, 2015

I just committed the patch.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants