-
Notifications
You must be signed in to change notification settings - Fork 28.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-28286][SQL][PYTHON][TESTS] Convert and port 'pivot.sql' into UDF test base #25122
Conversation
@HyukjinKwon I've raised this PR as a WIP till I incorporate your comments. I had some doubts regarding the tests in pivot.sql and was hoping you could clear it for me. While porting 'pivot.sql', I ran the command below on the original sql and it fails when running for configs
On inspection it seems like there is some discrepancy while handling the This error persists in the port also. As per the guide, I tried looking for a related Jira but couldn't find one, so I thought I'd run this by you first before creating one. Stacktrace:
Any help will be appreciated. Thanks, |
ok to test |
add to whitelist |
The |
sql/core/src/test/resources/sql-tests/results/udf/udf-pivot.sql.out
Outdated
Show resolved
Hide resolved
sql/core/src/test/resources/sql-tests/results/udf/udf-pivot.sql.out
Outdated
Show resolved
Hide resolved
Test build #107571 has finished for PR 25122 at commit
|
@HyukjinKwon Thanks for your review comments, I've replied to them. I feel the suggested workaround(this and this) will not work in this case as certain udf's like As per the guide in SPARK-27921 , do you suggest I create a separate Jira for this and comment the problematic test cases for now ? |
sql/core/src/test/resources/sql-tests/results/udf/udf-pivot.sql.out
Outdated
Show resolved
Hide resolved
Test build #107592 has finished for PR 25122 at commit
|
@chitralverma, #25130 is merged. Can you rebase and sync against the current master? |
retest this please |
Looks fine otherwise if the tests pass. I will take another look before merging it in. |
Test build #107813 has finished for PR 25122 at commit
|
@HyukjinKwon I'm merging the changes and testing. will ping you once its done. thanks |
retest this please |
Test build #107838 has finished for PR 25122 at commit
|
@HyukjinKwon you can review this now. thanks. I've also updated the diff in the OP |
) | ||
PIVOT ( | ||
udf(sum(earnings)) | ||
FOR course IN ('dotNET', 'Java') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can test it by myself but I thought you know the results - just out of curiosity, what do we get if we do FOR udf(course)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FOR udf(course)
in line 29 will result in a org.apache.spark.sql.catalyst.parser.ParseException
Test build #107839 has finished for PR 25122 at commit
|
Merged to master. Thanks for working on this, @chitralverma |
Thanks for merging this @HyukjinKwon. This was my first contribution, looking forward to doing more. :D |
Test build #107849 has finished for PR 25122 at commit
|
Test build #107851 has finished for PR 25122 at commit
|
Thank YOU @chitralverma for staying focused on each diff here and writing a PR even without nits :D. Nowdays, those details are a key. |
What changes were proposed in this pull request?
This PR adds some tests converted from pivot.sql to test UDFs following the combination guide in SPARK-27921.
Diff comparing to 'pivot.sql'
How was this patch tested?
Tested as guided in SPARK-27921.