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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
