You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@spark.apache.org by "mihailoale-db (via GitHub)" <gi...@apache.org> on 2026/07/08 14:37:42 UTC

[PR] [SPARK-58037] [PYTHON] [TESTS] Add DataFrame API golden file test framework for PySpark [spark]

mihailoale-db opened a new pull request, #57122:
URL: https://github.com/apache/spark/pull/57122

   ### 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?
   Yes.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-58037] [PYTHON] [TESTS] Add DataFrame API golden file test framework for PySpark [spark]

Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.

zhengruifeng commented on PR #57122:
URL: https://github.com/apache/spark/pull/57122#issuecomment-5275919991

Thanks. To clarify, the existing pipeline already covers more than unresolved-plan stability: `JVM DataFrame API → protobuf golden → analyzed Catalyst plan golden`

`ProtoToParsedPlanTestSuite` transforms and analyzes each protobuf plan. It seems straightforward to extend executable cases to also check the optimized plan, output schema, and result through QueryExecution. Execution should be
opt-in because some existing plans are streaming, schema-only, or otherwise non-executable.

The new Python framework does not assert protobuf stability and duplicates much of the server-side golden machinery. I would prefer extending the existing Scala framework for language-neutral DataFrame semantics. Any genuinely
Python-specific behavior can remain in focused, conventional PySpark tests.

Running a large golden corpus directly in Scala should also use fewer OSS CI resources by avoiding Python, Connect round trips, repeated analysis, and Python-side result conversion.

--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-58037] [PYTHON] [TESTS] Add DataFrame API golden file test framework for PySpark [spark]

Posted by "mihailoale-db (via GitHub)" <gi...@apache.org>.

mihailoale-db commented on PR #57122:
URL: https://github.com/apache/spark/pull/57122#issuecomment-4992295055

Hey @gaogaotiantian, thanks for sharing your concerns.
1. I don't think that it's necessary to include these files in `MANIFEST.in`. I can revert it if that makes sense.
2. I would say that having a script per file is convenient from the perspective of parsing both the input and results. IMO it's easily manageable in the agentic world. Although we could choose a different option we went with "script per file" approach, it's a design decision, as simple as that.
3. I'm not sure that I understand this part. Where would you place it?
4. There are couple of new things introduced with this framework. As you can see the results are inlined right after the script so we don't have to jump around inputs, end results and analyzer plan results. Also we introduced the optimized results as an improvement.
5. Yes the initial plan was to introduce the framework and initial tests in this PR and to add more tests in subsequent PRs. We model the tests based on SQL queries from the `sql/core/src/test/resources/sql-tests/inputs` and I would expect people who contribute to SQL golden files to add DF golden files as well.

Thanks.

--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-58037] [PYTHON] [TESTS] Add DataFrame API golden file test framework for PySpark [spark]

Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.

zhengruifeng commented on PR #57122:
URL: https://github.com/apache/spark/pull/57122#issuecomment-5265694036

   We already have one golden file framework in scala side, the PlanGenerationTestSuite → ProtoToParsedPlanTestSuite pipeline. It already covers `DataFrame API → connect proto → analyzed plan`.
   
   https://github.com/apache/spark/blob/274049387f96976c1bda1bedca3e6f96894c7e8e/sql/connect/client/jvm/src/test/scala/org/apache/spark/sql/PlanGenerationTestSuite.scala#L413-L427
   
   isn't this new one kind of redundant? or it is testing some python-specific features?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-58037] [PYTHON] [TESTS] Add DataFrame API golden file test framework for PySpark [spark]

Posted by "dtenedor (via GitHub)" <gi...@apache.org>.

dtenedor commented on PR #57122:
URL: https://github.com/apache/spark/pull/57122#issuecomment-4983760047

   LGTM, merging to master + 4.x


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-58037] [PYTHON] [TESTS] Add DataFrame API golden file test framework for PySpark [spark]

Posted by "dtenedor (via GitHub)" <gi...@apache.org>.

dtenedor closed pull request #57122: [SPARK-58037] [PYTHON] [TESTS] Add DataFrame API golden file test framework for PySpark
URL: https://github.com/apache/spark/pull/57122


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-58037] [PYTHON] [TESTS] Add DataFrame API golden file test framework for PySpark [spark]

Posted by "gaogaotiantian (via GitHub)" <gi...@apache.org>.

gaogaotiantian commented on PR #57122:
URL: https://github.com/apache/spark/pull/57122#issuecomment-4986389565

   I have a few questions about this PR:
   1. Is it intentional to include the test scripts in `MANIFEST.in`? Why do we need to package these files for pyspark?
   2. Why do we choose to have a million test fixture files which are invalid Python, rather than a file with test cases which can be a valid Python? The test results are aggregated and compared in a single test file, but the fixtures are different files - that does not quite make sense to me. We had to do some tricks like injecting `spark` to global namespace - that's a bit unusual. I think we can achieve the exact same thing if we just put everything in a single file and make every test fixture a method.
   3. Do we expect spark end users to use this framework? Is it officially supported as a spark feature? `pyspark/testing` is user-interfacing and having `df_golden.py` there means this is usable by users and we need to maintain the utility (and keep backwards compatibility, which could be some effort).
   4. Is the golden file format matching the Scala SQL or it's something new we design for Python side?
   5. What is the ultimate goal for this framework? Do we expect more tests written in this format? I don't think this is very conventional in Python world. Is there some specific benefit from it?
   
   Thanks.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-58037] [PYTHON] [TESTS] Add DataFrame API golden file test framework for PySpark [spark]

Posted by "dtenedor (via GitHub)" <gi...@apache.org>.

dtenedor commented on PR #57122:
URL: https://github.com/apache/spark/pull/57122#issuecomment-4983767560

   **Merge Summary:**
   - merged into master https://github.com/apache/spark/commit/cd3b583cdacf93c75257fcdc21f0b283cf79f647
   - merged into branch-4.x https://github.com/apache/spark/commit/5360370be026cad8873ad43c52dd321a2689ee7c
   
   *Posted by `merge_spark_pr.py`*


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-58037] [PYTHON] [TESTS] Add DataFrame API golden file test framework for PySpark [spark]

Posted by "mihailoale-db (via GitHub)" <gi...@apache.org>.

mihailoale-db commented on PR #57122:
URL: https://github.com/apache/spark/pull/57122#issuecomment-5267933667

   IIUC what you sent ensures that DF produced unresolved plans are stable. Golden file DF framework that we introduced here ensure that analyzed plan, optimized plan, output schema and result that come from the DF produced unresolved plans are stable (similarly to SQL produced unresolved plans). So it seems to me that we need both.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-58037] [PYTHON] [TESTS] Add DataFrame API golden file test framework for PySpark [spark]

Posted by "gaogaotiantian (via GitHub)" <gi...@apache.org>.

gaogaotiantian commented on PR #57122:
URL: https://github.com/apache/spark/pull/57122#issuecomment-4995726534

   Okay thanks for the answers. I'll share my thoughts on this.
   
   First of all, I don't fully support this in general. The reasons are:
   1. I don't really support golden file test in Python because
       a. it's not a common pattern, unlike Scala. We should not have it just because Scala has it.
       b. there's no existing mature frameworks supporting it, which means we have to maintain a whole new framework just for pyspark testing
       c. when golden file tests fail, more than often people just re-generate the golden files - that reduces the capability to catch regressions
   2. We already have a testing framework, which works fine. The new testing framework does not add any new value (as far as I can tell, at least for now). Having two or more testing framework does not make our tests better. It often makes it worse.
   3. This does not solve problems. This creates a thing to solve problem that does not exist yet, and the created thing could be a problem by its own.
   
   I can tell that you are a SQL expert, and this might be a very common solution in SQL or Scala world. I don't have any trouble with it being used there. But I do have some concerns of introducing this to Python world. The analyze/plan/optimize part is not well tested in PySpark, but it's also not part of PySpark.
   
   Here's my suggestions, feel free to discuss with me and the community if you don't agree.
   1. Things we must do
       a. revert the `MANIFEST.in` change. We should not ship these tests to users. We don't ship any tests to users.
       b. move the files put into `pyspark/testing` out of there. `pyspark/testing` is user accessible. Which means pyspark users (rather than developers) can access that directory. Having `df_golden.py` there will make this testing framework the de facto supported testing framework by pyspark, which is so much worse than being an internal testing framework which we can change or remove anytime we want.
   2. Thing we should do
       a. collect all testing mixtures into a single file and make it a real Python file instead of a piece of snippet. Having a class-like structure to contain each test mixture as a method will keep the simplicity but making it more structured.
   3. Things we should consider if we decide to keep the golden test as an experiment in Python world
       a. instead of writing our own parser to read/write raw text files, choose a common format that can fit our needs. I think `toml` would be a good candidate. It's easily readable by both humans and computers and it's natively supported by Python 3.11+. I think the result below is even more readable than the raw text, especially with syntax highlighting.
   ```toml
   [group by with multiple aggregates]
   
   tags = ["unordered"]
   script = "scripts/group_by/group_by_multiple_counts.py"
   expected_analysis_output = """
   Aggregate [a#x], [a#x, count(a#x) AS count(a)#xL, count(b#x) AS count(b)#xL]
   +- SubqueryAlias testdata
      +- View (`testData`, [a#x, b#x])
         +- Project [cast(a#x as int) AS a#x, cast(b#x as int) AS b#x]
            +- Project [a#x, b#x]
               +- SubqueryAlias testData
                  +- LocalRelation [a#x, b#x]
   """
   expected_optimized_output = """
   Aggregate [a#x], [a#x, count(a#x) AS count(a)#xL, count(b#x) AS count(b)#xL]
   +- LocalRelation [a#x, b#x]
   """
   expected_output_schema = "struct<a:int,count(a):bigint,count(b):bigint>"
   expected_result = """
   +------+----------+----------+
   | a    | count(a) | count(b) |
   +------+----------+----------+
   | 1    | 2        | 2        |
   | 2    | 2        | 2        |
   | 3    | 3        | 2        |
   | NULL | 0        | 1        |
   +------+----------+----------+
   printed all 4 rows.
   """
   expected_result_hash ="978157032220537c5982e8c4ceda91f71f8cc1159b71bf8023c71622b591dc9e"
   ```
       b. fit the whole framework into our existing testing framework, probably as a class mixture. `class GoldenFileTest` or something. If the class is mixed into the testing class, then each test method would run the test snippet against the golden file (or create one if specified). We have something similar in PySpark now and it worked fine in our existing framework. We don't have to create and maintain a whole new thing if we just need some coverage on this area.
   
   To summarize, there are some things we need to fix for this PR. And for the general direction, if we really want to do it, we can, just do it in a better and more Pythonic way.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-58037] [PYTHON] [TESTS] Add DataFrame API golden file test framework for PySpark [spark]

Posted by "mihailoale-db (via GitHub)" <gi...@apache.org>.

mihailoale-db commented on PR #57122:
URL: https://github.com/apache/spark/pull/57122#issuecomment-4957200150

   @HyukjinKwon @dtenedor PTAL when you find time. Thanks!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org