-
Notifications
You must be signed in to change notification settings - Fork 232
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
Remove GpuAttributeReference and GpuSortOrder #253
Conversation
} else { | ||
input: AttributeSeq): R = { | ||
val ret = expression.transform { | ||
case a: AttributeReference => |
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.
By the time we come here we will only have 2 types in the expression i.e. AttributeReference
, SortOrder
?
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.
transform walks through the tree and if the supplied pattern matches something in the tree it will be replaced with what is returned. So the goal of the code is to replace all AttributeReference
instances with GpuBoundReference
instances. Everything else passes through.
@@ -118,7 +118,7 @@ abstract class GpuUnaryExpression extends UnaryExpression with GpuExpression { | |||
def outputTypeOverride: DType = null | |||
|
|||
override def columnarEval(batch: ColumnarBatch): Any = { | |||
val input = child.asInstanceOf[GpuExpression].columnarEval(batch) | |||
val input = child.columnarEval(batch) |
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.
Can you please clarify. From what I understand so far children could possibly contain a SortOrder
??
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.
Nothing that is evaluated can contain a SortOrder
. SortOrder
is Unevaluable
. So it will not work columnar or otherwise. It is up to spark to only insert it into places that it knows will work properly, like SortExec
.
@tgravescs @jlowe Because this does have a potential impact to a lot of downstream PRs I would like to merge this in even if CI is not set up yet. All of the release changes are in except for CI. I upmerged to the latest on branch-0.2 and ran the full set of tests locally are you OK if I merge this in? |
I'm OK with it. Worst-case scenario the build is broken and we fix it with a followup or revert. We need to keep making progress. |
Signed-off-by: Jason Lowe <jlowe@nvidia.com>
This fixes #199
This fixes #187
This takes over for #244