Skip to content
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

Integrate the Spark-Connect tests with the existing test suite & CI #256

Conversation

nijanthanvijayakumar
Copy link
Contributor

Proposed changes

This is related to the #241 issue and the following changes have been made:

  1. Updates to the ci.yml:
    Invoke the tests using Spark-Connect & make

  2. Updates to the CONTRIBUTING.md document.
    Brings the changes in from the main branch describing the pre-commit installation & setup, and auto-assigning the issues.

  3. Refactor the following *.py files to work with the Spark-Connect tests and also update the tests accordingly
    a) dataframe_helpers.py
    b) functions.py
    c) transformations.py
    d) test_functions.py
    e) test_transformations.py

  4. [Highlight]: Handling unsupported functions on Spark-Connect less than 3.5.2
    a) The functions array_choice and the sort_columns don't work properly on the Spark-Connect v3.5.2 and below. So, raising an exception if the Spark version is < 3.5.2 and if SPARK_CONNECT_MODE is enabled.
    b) Create a wrapper within the test_transformations.py, as the sort_columns function/method is being used widely across 10+ test cases. Using a wrapper would help reduce the duplicate lines of code.

Types of changes

What types of changes does your code introduce to Quinn?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update (if none of the other choices apply)

Further comments

@nijanthanvijayakumar
Copy link
Contributor Author

nijanthanvijayakumar commented Aug 9, 2024

@SemyonSinchenko - let me know your thoughts on the PR. I've updated it to raise exception for anything below Spark-Connect v3.5.2 & updated the test cases too.

The .md file updates you see are the ones I've pulled from the planning-1.0-release branch to this.

CONTRIBUTING.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@SemyonSinchenko SemyonSinchenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you @nijanthanvijayakumar !
I will merge it by the end of the day if no new comments.
@jeffbrennan @MrPowers FYI

@nijanthanvijayakumar
Copy link
Contributor Author

Thank you for your guidance and support @SemyonSinchenko .

Phew😮‍💨 that was a massive PR, at least for me.

@SemyonSinchenko SemyonSinchenko merged commit 86c5f18 into mrpowers-io:planning-1.0-release Aug 10, 2024
5 checks passed
@nijanthanvijayakumar nijanthanvijayakumar deleted the feature/issue-241-integrate-sc-tests branch August 10, 2024 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants