[SPARK-52181] Increase variant size limit to 128MiB #50913

chenhao-db · 2025-05-16T04:21:16Z

What changes were proposed in this pull request?

Increase variant size limit from 16MiB to 128MiB. It is difficult to control the size limit with a flag, because the limit is accessed in many places where the SQL config is not available (e.g., VariantVal.toString). Future memory instability is possible, but this change won't break any existing workload.

Why are the changes needed?

It enhances the ability of the variant data type to process larger data.

Does this PR introduce any user-facing change?

Yes, as stated above.

How was this patch tested?

Unit test.

Was this patch authored or co-authored using generative AI tooling?

No.

chenhao-db · 2025-05-16T04:22:00Z

@cloud-fan Could you help review? Thanks!

...cala/org/apache/spark/sql/catalyst/expressions/variant/VariantExpressionEvalUtilsSuite.scala

…essions/variant/VariantExpressionEvalUtilsSuite.scala

cloud-fan · 2025-05-16T15:15:15Z

...cala/org/apache/spark/sql/catalyst/expressions/variant/VariantExpressionEvalUtilsSuite.scala

    }
    for (json <- Seq("\"" + "a" * (16 * 1024 * 1024) + "\"",
-      (0 to 4 * 1024 * 1024).mkString("[", ",", "]"))) {
+      (0 to 100 * 1024 * 1024).mkString("[", ",", "]"))) {


still can't trigger the size limitation, I'll leave it to @chenhao-db

cloud-fan · 2025-05-16T19:35:35Z

seems a real test failure in from_csv with variant *** FAILED ***

chenhao-db · 2025-05-16T19:46:56Z

@cloud-fan It is just another test that assumes the old limit. Fixed.

cloud-fan · 2025-05-16T22:23:07Z

thanks, merging to master!

cloud-fan · 2025-05-16T22:24:12Z

@chenhao-db can you open a backport PR for 4.0? thanks!

Increase variant size limit from 16MiB to 128MiB. It is difficult to control the size limit with a flag, because the limit is accessed in many places where the SQL config is not available (e.g., `VariantVal.toString`). Future memory instability is possible, but this change won't break any existing workload. It enhances the ability of the variant data type to process larger data. Yes, as stated above. Unit test. No. Closes apache#50913 from chenhao-db/variant_large_size_limit. Lead-authored-by: Chenhao Li <[email protected]> Co-authored-by: Wenchen Fan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

### What changes were proposed in this pull request? This is a cherry-pick of #50913 Increase variant size limit from 16MiB to 128MiB. It is difficult to control the size limit with a flag, because the limit is accessed in many places where the SQL config is not available (e.g., `VariantVal.toString`). Future memory instability is possible, but this change won't break any existing workload. ### Why are the changes needed? It enhances the ability of the variant data type to process larger data. ### Does this PR introduce _any_ user-facing change? Yes, as stated above. ### How was this patch tested? Unit test. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #50927 from chenhao-db/variant_large_size_limit_4.0. Authored-by: Chenhao Li <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

LuciferYang · 2025-05-19T11:03:11Z

It seems that java.lang.OutOfMemoryError: Java heap space issues started occurring in GitHub Action tests after this pull request was merged:

Could you take a look when you have time? @chenhao-db
also cc @cloud-fan

cloud-fan · 2025-05-19T12:30:47Z

sql/core/src/test/scala/org/apache/spark/sql/CsvFunctionsSuite.scala

-    // case is the input exceeds the variant size limit (16MiB).
-    val largeInput = "a" * (16 * 1024 * 1024)
+    // case is the input exceeds the variant size limit (128MiB).
+    val largeInput = "a" * (128 * 1024 * 1024)


Maybe the GA machines are not powerful enough to run tests that take 128 MB. One idea is to have a hardcoded testing limit of 16 MB. e.g.

public static final int SIZE_LIMIT = if (System.getenv("SPARK_TESTING") != null) U24_MAX + 1 else 128 * 1024 * 1024;

should use sys.env.contains("GITHUB_ACTIONS")

then local test and GA test will have different error messages, we will have to complicate the test cases to make them pass for both environments.

I've opened #50950

### What changes were proposed in this pull request? This is a follow-up of #50913 . It sets the variant size limit back to 16 MB in test environment, to reduce OOM in tests. ### Why are the changes needed? make tests more reliable. ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? existing tests ### Was this patch authored or co-authored using generative AI tooling? no Closes #50950 from cloud-fan/test. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: yangjie01 <[email protected]>

This is a follow-up of apache#50913 . It sets the variant size limit back to 16 MB in test environment, to reduce OOM in tests. make tests more reliable. no existing tests no Closes apache#50950 from cloud-fan/test. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: yangjie01 <[email protected]>

This is a 4.0 backport of #50950 ### What changes were proposed in this pull request? This is a follow-up of #50913 . It sets the variant size limit back to 16 MB in test environment, to reduce OOM in tests. ### Why are the changes needed? make tests more reliable. ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? existing tests ### Was this patch authored or co-authored using generative AI tooling? no Closes #50954 from cloud-fan/test. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

tedyu · 2025-05-21T15:32:05Z

The instantiation of BigDecimal is used by VariantValueConverter. parseDecimal.

      def parseDecimal(): DataType = {
        try {
          var d = decimalParser(s)
          if (d.scale() < 0) {
            d = d.setScale(0)
          }
          if (d.scale() <= VariantUtil.MAX_DECIMAL16_PRECISION &&
            d.precision() <= VariantUtil.MAX_DECIMAL16_PRECISION) {
            builder.appendDecimal(d)
            // The actual decimal type doesn't matter. `appendDecimal` will use the smallest
            // possible decimal type to store the value.
            DecimalType.USER_DEFAULT
          } else {
            if (options.preferDate) parseDate() else parseTimestampNTZ()
          }

The above code can be refactored so that we get scale and precision from the string directly. Date and timestamp parsing doesn't require BigDecimal instance.
This way, memory consumption can be reduced.

I have a feeling that increasing the limit without providing a new config knob may result in surprising OOM issues when the new release goes to production.

cloud-fan · 2025-05-22T01:59:51Z

@tedyu I'm confused, how can increasing the limit bring new OOM issues? We don't have a knob before this PR either.

tedyu · 2025-05-22T12:05:38Z

I am still trying to understand the implication of OOM in tests. The previous comment provided one refactor that avoids unnecessary creation of bigdecimal's.
In my opinion, providing a config knob for the limit would give us enough room for handling potential issues after release.

cloud-fan · 2025-05-22T14:16:50Z

@tedyu you are welcome to add such a config, but I think it's hard as variant component is an individual module that does not depend on spark sql.

tedyu · 2025-05-22T14:31:14Z

It seems we can pass Configuration explicitly to VariantVal.toString:

def toString(conf: VariantConf): String

where VariantConf is a lightweight case class.
This requires a large change across modules. Not sure it is worth the effort.

chenhao-db · 2025-05-22T17:22:18Z

Increasing the limit won't bring any new OOM to existing workloads. The new OOM in tests is because we changed the test code to use a bigger input.

I'm not sure whether we are able to find all the use cases of toString, which can happen in many places implicitly. Even if you provide this new version of toString, you cannot easily guarantee the no-parameter version is not called.

tedyu · 2025-05-22T17:28:26Z

bq. The new OOM in tests is because we changed the test code to use a bigger input.

With the release containing this PR, some users would feed their workloads with bigger input.

cloud-fan · 2025-05-23T15:25:14Z

@tedyu Please note that we don't have a size limitation for many data types: UTF8String, binary, struct/array/map values. I don't think VARIANT makes things worse. And it's a new feature, so there is nothing wrong with shipping it using 128 MB as the limit.

### What changes were proposed in this pull request? Increase variant size limit from 16MiB to 128MiB. It is difficult to control the size limit with a flag, because the limit is accessed in many places where the SQL config is not available (e.g., `VariantVal.toString`). Future memory instability is possible, but this change won't break any existing workload. ### Why are the changes needed? It enhances the ability of the variant data type to process larger data. ### Does this PR introduce _any_ user-facing change? Yes, as stated above. ### How was this patch tested? Unit test. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#50913 from chenhao-db/variant_large_size_limit. Lead-authored-by: Chenhao Li <[email protected]> Co-authored-by: Wenchen Fan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

### What changes were proposed in this pull request? This is a follow-up of apache#50913 . It sets the variant size limit back to 16 MB in test environment, to reduce OOM in tests. ### Why are the changes needed? make tests more reliable. ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? existing tests ### Was this patch authored or co-authored using generative AI tooling? no Closes apache#50950 from cloud-fan/test. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: yangjie01 <[email protected]>

### What changes were proposed in this pull request? This is a cherry-pick of apache#50913 Increase variant size limit from 16MiB to 128MiB. It is difficult to control the size limit with a flag, because the limit is accessed in many places where the SQL config is not available (e.g., `VariantVal.toString`). Future memory instability is possible, but this change won't break any existing workload. ### Why are the changes needed? It enhances the ability of the variant data type to process larger data. ### Does this PR introduce _any_ user-facing change? Yes, as stated above. ### How was this patch tested? Unit test. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#50927 from chenhao-db/variant_large_size_limit_4.0. Authored-by: Chenhao Li <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

This is a 4.0 backport of apache#50950 ### What changes were proposed in this pull request? This is a follow-up of apache#50913 . It sets the variant size limit back to 16 MB in test environment, to reduce OOM in tests. ### Why are the changes needed? make tests more reliable. ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? existing tests ### Was this patch authored or co-authored using generative AI tooling? no Closes apache#50954 from cloud-fan/test. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

initial

4f9ac0e

github-actions bot added the SQL label May 16, 2025

cloud-fan approved these changes May 16, 2025

View reviewed changes

Update VariantExpressionEvalUtilsSuite.scala

e6ee3e6

cloud-fan reviewed May 16, 2025

View reviewed changes

...cala/org/apache/spark/sql/catalyst/expressions/variant/VariantExpressionEvalUtilsSuite.scala Outdated Show resolved Hide resolved

Update sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expr…

ea307ee

…essions/variant/VariantExpressionEvalUtilsSuite.scala

cloud-fan reviewed May 16, 2025

View reviewed changes

fix test

2c61040

fix csv test

89997df

cloud-fan closed this in af6499f May 16, 2025

chenhao-db mentioned this pull request May 16, 2025

[SPARK-52181][4.0] Increase variant size limit to 128MiB #50927

Closed

cloud-fan reviewed May 19, 2025

View reviewed changes

cloud-fan mentioned this pull request May 20, 2025

[SPARK-52181][SQL][FOLLOWUP] Avoid OOM in variant tests #50950

Closed

cloud-fan mentioned this pull request May 20, 2025

[SPARK-52181][SQL][FOLLOWUP][4.0] Avoid OOM in variant tests #50954

Closed

[SPARK-52181] Increase variant size limit to 128MiB #50913

[SPARK-52181] Increase variant size limit to 128MiB #50913

Uh oh!

Conversation

chenhao-db commented May 16, 2025

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

Uh oh!

chenhao-db commented May 16, 2025

Uh oh!

Uh oh!

cloud-fan May 16, 2025

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented May 16, 2025

Uh oh!

chenhao-db commented May 16, 2025

Uh oh!

cloud-fan commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloud-fan commented May 16, 2025

Uh oh!

LuciferYang commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloud-fan May 19, 2025

Choose a reason for hiding this comment

Uh oh!

LuciferYang May 19, 2025

Choose a reason for hiding this comment

Uh oh!

cloud-fan May 20, 2025

Choose a reason for hiding this comment

Uh oh!

cloud-fan May 20, 2025

Choose a reason for hiding this comment

Uh oh!

tedyu commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloud-fan commented May 22, 2025

Uh oh!

tedyu commented May 22, 2025

Uh oh!

cloud-fan commented May 22, 2025

Uh oh!

tedyu commented May 22, 2025

Uh oh!

chenhao-db commented May 22, 2025

Uh oh!

tedyu commented May 22, 2025

Uh oh!

cloud-fan commented May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cloud-fan commented May 16, 2025 •

edited

Loading

LuciferYang commented May 19, 2025 •

edited

Loading

tedyu commented May 21, 2025 •

edited

Loading

cloud-fan commented May 23, 2025 •

edited

Loading