-
Notifications
You must be signed in to change notification settings - Fork 987
DRILL-5457: Spill implementation for Hash Aggregate #822
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,8 @@ | |
| */ | ||
| package org.apache.drill.exec.physical.base; | ||
|
|
||
| import com.fasterxml.jackson.annotation.JsonIgnore; | ||
| import org.apache.drill.common.exceptions.DrillRuntimeException; | ||
| import org.apache.drill.common.graph.GraphVisitor; | ||
| import org.apache.drill.exec.record.BatchSchema.SelectionVectorMode; | ||
|
|
||
|
|
@@ -102,17 +104,31 @@ public void setCost(double cost) { | |
| this.cost = cost; | ||
| } | ||
|
|
||
| // Not available. Presumably because Drill does not currently use | ||
| // this value, though it does appear in some test physical plans. | ||
| // public void setMaxAllocation(long alloc) { | ||
| // maxAllocation = alloc; | ||
| // } | ||
|
|
||
| @Override | ||
| public long getMaxAllocation() { | ||
| return maxAllocation; | ||
| } | ||
|
|
||
| /** | ||
| * Any operator that supports spilling should override this method | ||
| * @param maxAllocation The max memory allocation to be set | ||
| */ | ||
| @Override | ||
| public void setMaxAllocation(long maxAllocation) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem with this method, which is why it was commented out. Trying to remember... Something about the internal, temporary serializations in planning causing values to be overwritten. Jackson will see this method and use it for deserializing the value. May have to fiddle around a bit to remember the issue.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's what happens when you give a software "intelligence" to make decisions for you ..... shall we pick another method name to fool Jackson ?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we just want to fool Jackson we can use |
||
| this.maxAllocation = maxAllocation; | ||
| /*throw new DrillRuntimeException("Unsupported method: setMaxAllocation()");*/ | ||
| } | ||
|
|
||
| /** | ||
| * Any operator that supports spilling should override this method (and return true) | ||
| * @return false | ||
| */ | ||
| @Override @JsonIgnore | ||
| public boolean isBufferedOperator() { return false; } | ||
|
|
||
| // @Override | ||
| // public void setBufferedOperator(boolean bo) {} | ||
|
|
||
| @Override | ||
| public String getUserName() { | ||
| return userName; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,12 +49,19 @@ public int getOperatorType() { | |
| return CoreOperatorType.EXTERNAL_SORT_VALUE; | ||
| } | ||
|
|
||
| // Set here, rather than the base class, because this is the only | ||
| // operator, at present, that makes use of the maximum allocation. | ||
| // Remove this, in favor of the base class version, when Drill | ||
| // sets the memory allocation for all operators. | ||
|
|
||
| /** | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can just remove the Javadoc so it is inherited from the base method.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But still need a local comment to be clear about this code .... so does not matter much. |
||
| * | ||
| * @param maxAllocation The max memory allocation to be set | ||
| */ | ||
| @Override | ||
| public void setMaxAllocation(long maxAllocation) { | ||
| this.maxAllocation = maxAllocation; | ||
| } | ||
|
|
||
| /** | ||
| * The External Sort operator supports spilling | ||
| * @return true | ||
| */ | ||
| @Override | ||
| public boolean isBufferedOperator() { return true; } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class was refactored in the external sort unit test PR. Let's coordinate on merging the two sets of changes.