SPARK-17424: Fix unsound substitution bug in ScalaReflection. #15062
Conversation
Substitution fails when the actual type args are empty because they are still unknown. Instead, when there are no resolved types to subsitute, return the formal type args with unresovled type parameters.
|
Test build #65269 has finished for PR 15062 at commit
|
|
Could you add a regression test? |
|
@hvanhovell, I'm not sure what's causing this to build the test. I can step through with a debugger to see that the problem is caused by an empty actualTypeArgs list, but I don't know why it is that way. Maybe someone with more knowledge of Scala types can help fill in the details. Until then, I'm not sure how to construct a test case for this. |
|
I don't quite understand what use-case this patch fixes. Can you provide an example (in the form of a test) that reproduces the issue? |
|
Oh, I was too quick to comment, I see a complete description is in the JIRA. |
|
@jodersky, as I said above, I'm not really sure how to build a test for this because I'm not too familiar with the Scala internals that are misused here. However, since I can confirm that it works in practice, I think it's reasonable to commit it anyway, before committers that know this code well can have a look to make sure this method is tested properly. Another way to go is to revert PR #10970. That is a work-around because another test was already hitting this problem and the solution was to avoid the parameter substitution. |
|
Jenkins retest this please |
|
I'm just unclear of why this is needed . According to the linked issue, some tests were failing and this fixed them. Everything just seems very vague to me. It would be very helpful if you could provide an example (it doesn't have to be a unit-test) that reproduces the error. |
|
I think the best way to reproduce this is to revert #10970. I don't have a case that reproduces it that I can share since this was in a fairly large job. |
|
Test build #66190 has finished for PR 15062 at commit
|
|
I tried to reproduce with #10970 reverted, but I didn't hit the issue in testing. I still think it's fine to move forward on this, even if it is hard to reproduce because we know the code is wrong and this fixes it. |
|
retest this please |
|
I'm fine with this change, cc @yhuai @liancheng |
|
Test build #68939 has finished for PR 15062 at commit
|
|
any chance that is the same issue as https://issues.apache.org/jira/browse/SPARK-17109? |
|
We were seeing the problem when using the datasets API in our 1.6.1 build, which is based on Scala 2.10. I recently tried to reproduce this on master with 2.11 and #10970 reverted, but I didn't get a case that failed. Either way, I think the fix here makes sense: if there are no types to substitute, don't do it. |
|
This does look the same as SPARK-17109. Does this fix that issue? |
|
retest this please |
|
Test build #76863 has finished for PR 15062 at commit
|
## What changes were proposed in this pull request? This method gets a type's primary constructor and fills in type parameters with concrete types. For example, `MapPartitions[T, U] -> MapPartitions[Int, String]`. This Substitution fails when the actual type args are empty because they are still unknown. Instead, when there are no resolved types to subsitute, this returns the original args with unresolved type parameters. ## How was this patch tested? This doesn't affect substitutions where the type args are determined. This fixes our case where the actual type args are empty and our job runs successfully. Author: Ryan Blue <blue@apache.org> Closes #15062 from rdblue/SPARK-17424-fix-unsound-reflect-substitution. (cherry picked from commit b236933) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
## What changes were proposed in this pull request? This method gets a type's primary constructor and fills in type parameters with concrete types. For example, `MapPartitions[T, U] -> MapPartitions[Int, String]`. This Substitution fails when the actual type args are empty because they are still unknown. Instead, when there are no resolved types to subsitute, this returns the original args with unresolved type parameters. ## How was this patch tested? This doesn't affect substitutions where the type args are determined. This fixes our case where the actual type args are empty and our job runs successfully. Author: Ryan Blue <blue@apache.org> Closes #15062 from rdblue/SPARK-17424-fix-unsound-reflect-substitution. (cherry picked from commit b236933) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
## What changes were proposed in this pull request? This method gets a type's primary constructor and fills in type parameters with concrete types. For example, `MapPartitions[T, U] -> MapPartitions[Int, String]`. This Substitution fails when the actual type args are empty because they are still unknown. Instead, when there are no resolved types to subsitute, this returns the original args with unresolved type parameters. ## How was this patch tested? This doesn't affect substitutions where the type args are determined. This fixes our case where the actual type args are empty and our job runs successfully. Author: Ryan Blue <blue@apache.org> Closes #15062 from rdblue/SPARK-17424-fix-unsound-reflect-substitution. (cherry picked from commit b236933) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
|
the change looks reasonable, merging to master/2.2/2.1/2.0! |
## What changes were proposed in this pull request? This method gets a type's primary constructor and fills in type parameters with concrete types. For example, `MapPartitions[T, U] -> MapPartitions[Int, String]`. This Substitution fails when the actual type args are empty because they are still unknown. Instead, when there are no resolved types to subsitute, this returns the original args with unresolved type parameters. ## How was this patch tested? This doesn't affect substitutions where the type args are determined. This fixes our case where the actual type args are empty and our job runs successfully. Author: Ryan Blue <blue@apache.org> Closes apache#15062 from rdblue/SPARK-17424-fix-unsound-reflect-substitution.
## What changes were proposed in this pull request? This method gets a type's primary constructor and fills in type parameters with concrete types. For example, `MapPartitions[T, U] -> MapPartitions[Int, String]`. This Substitution fails when the actual type args are empty because they are still unknown. Instead, when there are no resolved types to subsitute, this returns the original args with unresolved type parameters. ## How was this patch tested? This doesn't affect substitutions where the type args are determined. This fixes our case where the actual type args are empty and our job runs successfully. Author: Ryan Blue <blue@apache.org> Closes apache#15062 from rdblue/SPARK-17424-fix-unsound-reflect-substitution.
This method gets a type's primary constructor and fills in type parameters with concrete types. For example, `MapPartitions[T, U] -> MapPartitions[Int, String]`. This Substitution fails when the actual type args are empty because they are still unknown. Instead, when there are no resolved types to subsitute, this returns the original args with unresolved type parameters. This doesn't affect substitutions where the type args are determined. This fixes our case where the actual type args are empty and our job runs successfully. Author: Ryan Blue <blue@apache.org> Closes apache#15062 from rdblue/SPARK-17424-fix-unsound-reflect-substitution.
What changes were proposed in this pull request?
This method gets a type's primary constructor and fills in type parameters with concrete types. For example,
MapPartitions[T, U] -> MapPartitions[Int, String]. This Substitution fails when the actual type args are empty because they are still unknown. Instead, when there are no resolved types to subsitute, this returns the original args with unresolved type parameters.How was this patch tested?
This doesn't affect substitutions where the type args are determined. This fixes our case where the actual type args are empty and our job runs successfully.