-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-12293][SQL] Support UnsafeRow in LocalTableScan #10283
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
Conversation
|
Test build #47628 has finished for PR 10283 at commit
|
|
Test build #47634 has finished for PR 10283 at commit
|
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.
The return type of UnsafeProjection.apply is UnsafeRow already, looks like we don't need the asInstanceOf here?
|
I think the commits to fix these test errors are worth having their PR. I will create them later. |
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.
Normally Decimal should only be used inside spark SQL as the internal representation of decimal type, and we don't need to catch it here. Do we break it in tests?
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.
Yes. constructorFor will call dataTypeFor to determine if a type is ObjectType or not. If there is not case for Decimal, it will be recognized as ObjectType and causes bug.
|
Test build #47651 has finished for PR 10283 at commit
|
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.
If the unwrapped is null, the Invoke will return null, see https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects.scala#L172-L175
I think we don't need this extra If, did I miss something here?
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.
I think you are right. I add this to make sure it is not possibly creating wrong values when I debug. I will remove these If.
|
Test build #47829 has finished for PR 10283 at commit
|
|
Test build #47888 has finished for PR 10283 at commit
|
|
retest this please. |
|
Test build #47911 has finished for PR 10283 at commit
|
|
retest this please. |
|
Test build #47920 has finished for PR 10283 at commit
|
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.
we can use encoderFor[T] here
|
SQLUserDefinedType support for encoder is proposed in another pr #10390. |
|
@cloud-fan I think I have addressed all your comments. These bugs found in implementing UnsafeRow support in LocalTableScan are submitted as other PRs with their tests, so you can review them better. |
|
Test build #48094 has finished for PR 10283 at commit
|
|
retest this please. |
|
Test build #48101 has finished for PR 10283 at commit
|
JIRA: https://issues.apache.org/jira/browse/SPARK-12293
Make LocalTableScan support UnsafeRow.