-
Notifications
You must be signed in to change notification settings - Fork 28.7k
[SPARK-3412][SQL]add missing row api #2529
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
QA tests have started for PR 2529 at commit
|
QA tests have finished for PR 2529 at commit
|
Test PASSed. |
@@ -98,6 +99,7 @@ trait MutableRow extends Row { | |||
def setByte(ordinal: Int, value: Byte) | |||
def setFloat(ordinal: Int, value: Float) | |||
def setString(ordinal: Int, value: String) | |||
def setAs[T](ordinal: Int, value: T) = update(ordinal, value) |
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 don't think there is any reason to have this function. The update call works independent of type already.
Thanks for working on this! A few minor comments. |
QA tests have started for PR 2529 at commit
|
Tests timed out after a configured wait of |
Test FAILed. |
retest this please. |
Test FAILed. |
QA tests have started for PR 2529 at commit
|
retest this please. |
Test FAILed. |
QA tests have started for PR 2529 at commit
|
QA tests have finished for PR 2529 at commit
|
QA tests have finished for PR 2529 at commit
|
@@ -306,4 +306,8 @@ final class SpecificMutableRow(val values: Array[MutableValue]) extends MutableR | |||
override def getByte(i: Int): Byte = { | |||
values(i).asInstanceOf[MutableByte].value | |||
} | |||
|
|||
override def getAs[T](i: Int): T = { | |||
values(i).asInstanceOf[MutableAny].value.asInstanceOf[T] |
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.
You don't want the first .asInstanceOf
here as it can cause errors.
scala> new SpecificMutableRow(IntegerType :: Nil).getAs[Int](0)
java.lang.ClassCastException: org.apache.spark.sql.catalyst.expressions.MutableInt cannot be cast to org.apache.spark.sql.catalyst.expressions.MutableAny
Since all MutableValue
classes have a .value
method I think you can just leave it out.
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.
Also, looks like this file has always been named wrong. Would you mind fixing that (to SpecificMutableRow.scala
) while you are at it?
Only one small change then this LGTM. |
QA tests have started for PR 2529 at commit
|
Test FAILed. |
retest this please. |
QA tests have started for PR 2529 at commit
|
QA tests have finished for PR 2529 at commit
|
Test PASSed. |
QA tests have finished for PR 2529 at commit
|
Test FAILed. |
retest this please. |
QA tests have started for PR 2529 at commit
|
QA tests have finished for PR 2529 at commit
|
Test PASSed. |
@@ -306,4 +306,15 @@ final class SpecificMutableRow(val values: Array[MutableValue]) extends MutableR | |||
override def getByte(i: Int): Byte = { | |||
values(i).asInstanceOf[MutableByte].value | |||
} | |||
|
|||
override def getAs[T](i: Int): T = values(i) match { |
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't this just be override def getAs[T](i: Int): T = values(i).boxed.asInstanceOf[T]
?
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.
And I'm sorry for going back again. I should have said boxed
instead of value
in my previous comment.
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.
Sorry for miss that, I didn't notice .boxed
method there...
QA tests have started for PR 2529 at commit
|
QA tests have finished for PR 2529 at commit
|
Test PASSed. |
Thanks! Merged to master. |
@chenghao-intel assigned this to me, check PR #2284 for previous discussion