Wrap scalar generation into spark session in integration test

[thirtiseven]

Fixes #9404

When calling f.lit, error message:

pyspark.sql.utils.AnalysisException: decimal can only support precision up to 38

will be reported when spark.sql.legacy.allowNegativeScaleOfDecimal is unset.

This config is in the default config of integration test, but those config will only be set when calling with_spark_session, but calling f.lit can happen before any of them in some edge cases when CI running IT parallel.

This PR add the negative scale config before calling f.lit when generating scalars.

Also fixed the cache_repr of TimestampGen to add the new parameter.

[thirtiseven]

build

[res-life]

LGTM

[revans2]

What is the exception that actually triggered this? This is essentially the reason that we turned it on by default everywhere.

spark-rapids/integration_tests/src/main/python/spark_session.py

Line 55 in ca79e8d

'spark.sql.legacy.allowNegativeScaleOfDecimal': 'true',

I just want to understand why the default settings are not applying.

[thirtiseven]

@revans2 For example, in following case form the issue 9404:

@pytest.mark.parametrize('data_gen', [DecimalGen(34, -5)], ids=idfn)
def test_greatest1(data_gen):
    num_cols = 20
    s1 = gen_scalar(data_gen, force_no_nulls=not isinstance(data_gen, NullGen))
    # we want lots of nulls
    gen = StructGen([('_c' + str(x), data_gen.copy_special_case(None, weight=100.0))
        for x in range(0, num_cols)], nullable=False)
    command_args = [f.col('_c' + str(x)) for x in range(0, num_cols)]
    command_args.append(s1)
    data_type = data_gen.data_type
    assert_gpu_and_cpu_are_equal_collect(
            lambda spark : gen_df(spark, gen).select(
                f.greatest(*command_args)))

If we run this case individually, it will fail because f.lit was called in gen_scalar, but the config was set in assert_gpu_and_cpu_are_equal_collect for the first time.

In the pre-merge CI job, there are 4 xdist agents running in parallel, and the cases are assigned to different agents by round robin. So if this case happens to be assigned to the first one on the list for a particular agent, the CI will fail.

We believe that this is the reason why #9288's pre-merge keeps failing.

[jlowe]

Seems to me the issue is that one or more tests is generating data outside of the normal spark session context that sets up the configs properly. (i.e.: move the data generation to within the dataframe callback that is currently a lambda).

Personally, I'm not a fan of the current PR approach where data_gen can silently smash config values and leave them in that smashed state. Can be very surprising behavior and annoying to track down if it bites someone explicitly trying to test without that config setting.

[thirtiseven]

@jlowe Ok, I moved the gen_scalars into a spark session.

[revans2]

Putting this in a cpu_session fixes the current problem, but it adds a new one. If gen_scalars is called from inside a with_*_session it will have other problems. with_spark_session calls reset_spark_session_conf which does more than just reset the conf. It clears out the catalog too with no way to get the original config or catalog back after it exits. That means with_gpu_session -> gen_scalars will result in the query running on the CPU after the gen_scalars.

I see a few ways to properly fix this.

1. We set spark.sql.legacy.allowNegativeScaleOfDecimal when launching spark and have the test framework throw an exception if it is not set. Then we remove references to it in all of the tests for consistency. Then we file a follow on issue to fix with_spark_session to not allow nesting and to throw an exception if it is nested.
2. We fix with_spark_session to throw an exception if it is ever nested and do what you are doing today + update the docs for it to be clear that it can never be called from within a with_spark_session
3. We fix the test to call gen_scalar from within a with_spark_session and add a doc fix for gen_scalar to indicate that negative scale decimals can have problems if called from outside of with_spark_session block. Then we file a follow on issue to fix with_spark_session to not allow nesting and to throw an exception if it is nested.

I personally prefer option 1 but I am fine with option 2 or 3. Talking to @jlowe he really prefers option 3. The main difference between option 3 and option 2 for me really about the amount of code that needs to change. If we just fix the one test and add some docs, that feels like a really small change. If we have to fix nesting/etc that feels a bit larger, but it is something we need to do either way and would mean all tests that use gen_scalar would be good to deal with all decimal values properly.

[jlowe]

I'm not a fan of 2. It's again surprising behavior (who would expect it to spawn a Spark session?). I'm fine with either 1 or 3, and even with 1, I still think we should fix the test(s). We should be putting all data generation inside a spark session context of some kind.

[thirtiseven]

Thanks, updated code to option 3.

Now I wrap all scalar generation with a with_cpu_session, no matter if it calls f.lit or uses DecimalGen. Not sure if we only want to move the cases that are possible to fail into Spark sessions.

[thirtiseven]

Follow-on issue: #9412

[thirtiseven]

build

[thirtiseven]

build

[thirtiseven]

build

[thirtiseven]

build

[jlowe]

PR headline should be updated to reflect the new approach.

[thirtiseven]

@jlowe Thanks for review, all done.

[thirtiseven]

build

[res-life]

This PR is intended to put f.lit into with_cpu_session, not only the f.lit in the gen_scala but also the f.lit in other places. Maybe change to the following:

with_cpu_session(lambda spark: f.lit(gen_scalar(data_gen)).cast(data_gen.data_type))

[res-life]

Please check f.lit in other places in this PR.

[thirtiseven]

Good catch, checked and fixed.

[thirtiseven]

build

[thirtiseven]

Hi @revans2 please take another look thanks.