Uh oh!
There was an error while loading. Please reload this page.
Spark 4.0: Add variant round trip test for Spark - #14276
Conversation
huaxingao
commented
Oct 7, 2025
CI will pass once #14261 is in. |
huaxingao
commented
Oct 9, 2025
cc @aihuaxu@amogh-jahagirdar@singhpk234 Could you please take a look when you have a moment? Thanks! |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
singhpk234
left a comment
There was a problem hiding this comment.
LGTM, Thanks @huaxingao !
| vv1 = new Variant(((VariantVal) v1row1).getValue(), ((VariantVal) v1row1).getMetadata()); | ||
| vv2 = new Variant(((VariantVal) v1row2).getValue(), ((VariantVal) v1row2).getMetadata()); | ||
| } else { | ||
| fail("Expected Variant/VariantVal but got: " + (v1row1 == null ? "null" : v1row1.getClass())); |
There was a problem hiding this comment.
The Assertions#fail method supports string template, so it would be better to use it directly instead of concatenating strings on the caller side.
| import org.junit.jupiter.params.ParameterizedTest; | ||
| import org.junit.jupiter.params.provider.ValueSource; | ||
| public class TestSparkVariantRead extends TestBase { |
There was a problem hiding this comment.
Why do we include "Read" in the test class name? It looks like there are some write operations too.
There was a problem hiding this comment.
There is already a TestSparkVariants, but for different test purpose. Even though there are write operations, this test is mainly used for test read path.
| Object v1row2 = directRows.get(1).get(1); | ||
| Variant vv1; | ||
| Variant vv2; | ||
| if (v1row1 instanceof Variant) { |
There was a problem hiding this comment.
Why do we have Variant or VariantVal here? In Spark, would it always be VariantVal since it's from Spark?
There was a problem hiding this comment.
You are right. This should only be VariantVal
| import org.junit.jupiter.params.ParameterizedTest; | ||
| import org.junit.jupiter.params.provider.ValueSource; | ||
| public class TestSparkVariantRead extends TestBase { |
There was a problem hiding this comment.
Seems we are covering the variant query as a whole column. The variant extraction such as v1:k::string is not part of this PR, correct?
There was a problem hiding this comment.
Right, currently this only tests variant query as a whole column. I will add more tests as followup.
aihuaxu
left a comment
There was a problem hiding this comment.
Minor comment. Otherwise, LGTM.
| Object v1row2 = directRows.get(1).get(1); | ||
| Variant vv1; | ||
| Variant vv2; | ||
| if (v1row1 instanceof VariantVal) { |
There was a problem hiding this comment.
nit: we can assert VariantVal for v1row1 and v1row2 to VariantVal and then addd
vv1 = new Variant(((VariantVal) v1row1).getValue(), ((VariantVal) v1row1).getMetadata());
vv2 = new Variant(((VariantVal) v1row2).getValue(), ((VariantVal) v1row2).getMetadata());
Uh oh!
There was an error while loading. Please reload this page.
huaxingao
commented
Oct 15, 2025
Thanks @singhpk234@aihuaxu@ebyhr for the review! |
* Spark 4.0: Add variant round trip test for Spark * add a test for variant null value projection * address comment * address comments * address comments * remove un-used import
* Spark 4.0: Add variant round trip test for Spark * add a test for variant null value projection * address comment * address comments * address comments * remove un-used import
Adding variant round trip test for Spark, covering projection and filtering.