-
Notifications
You must be signed in to change notification settings - Fork 28.7k
[WIP] remove expensive api from InternalRow #6869
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 #35092 has finished for PR 6869 at commit
|
} | ||
abstract class InternalRow extends Serializable { | ||
/** Number of elements in the Row. */ | ||
def size: Int = length |
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.
do we need size? maybe just keep one of length or size.
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.
agree. It's internal and we don't need to follow scala convention to provide size
.
@davies can you update the pr description to include what you actually removed? |
@cloud-fan it will be fixed by #6876 |
* @throws ClassCastException when data type does not match. | ||
* @throws NullPointerException when value is null. | ||
*/ | ||
def getString(i: Int): String = getAs[UTF8String](i).toString |
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.
As it's internal row, should we just return UTF8String
?
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.
Maybe we can only define getUTF8String
at InternalRow
and define getString
at user-facing Row
?
Close this, in favor of #7003 |
This is a follow up PR for #6792, to use narrow down the APIs in InternalRow (remove those expensive ones).