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

SNOW-1733956: fix sql count of modin tests with cte enabled #2491

Merged
merged 2 commits into from
Oct 24, 2024

Conversation

sfc-gh-jdu
Copy link
Collaborator

  1. Which Jira issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

    Fixes SNOW-NNNNNNN

  2. Fill out the following pre-review checklist:

    • I am adding a new automated test(s) to verify correctness of my new code
      • If this test skips Local Testing mode, I'm requesting review from @snowflakedb/local-testing
    • I am adding new logging messages
    • I am adding a new telemetry message
    • I am adding new credentials
    • I am adding a new dependency
    • If this is a new feature/behavior, I'm adding the Local Testing parity changes.
    • I acknowledge that I have ensured my changes to be thread-safe. Follow the link for more information: Thread-safe Developer Guidelines
  3. Please describe how your code solves the related issue.

    Please write a short description of how your code change solves the related issue.

@sfc-gh-jdu sfc-gh-jdu added the NO-CHANGELOG-UPDATES This pull request does not need to update CHANGELOG.md label Oct 22, 2024
@sfc-gh-jdu sfc-gh-jdu force-pushed the jdu-SNOW-1733956-fix-sql-count branch from 803e1bb to 9860f7c Compare October 23, 2024 18:05
@sfc-gh-jdu sfc-gh-jdu changed the title Jdu snow 1733956 fix sql count SNOW-1733956: fix sql count of modin tests with cte enabled Oct 23, 2024
@sfc-gh-jdu sfc-gh-jdu force-pushed the jdu-SNOW-1733956-fix-sql-count branch from 9860f7c to 473650b Compare October 23, 2024 18:07
Copy link
Contributor

@sfc-gh-aalam sfc-gh-aalam left a comment

Choose a reason for hiding this comment

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

if this change also affects daily precommit, can you post a successful run for those as well

@@ -242,8 +242,8 @@ jobs:
.tox/.coverage
.tox/coverage.xml
test-enable-cte-optimization:
name: Test Enable CTE Optimization modin-${{ matrix.os.download_name }}-${{ matrix.python-version }}-${{ matrix.cloud-provider }}
test-disable-cte-optimization:
Copy link
Contributor

@sfc-gh-helmeleegy sfc-gh-helmeleegy Oct 23, 2024

Choose a reason for hiding this comment

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

I'm not sure I'm getting it. How come we're disabling the CTE optimization here and in many other places, but still decreasing the sql counts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a new test with disable cte optimization. In this test, we actually ignore sql count check (

PYTEST_ADDOPTS: --color=yes --tb=short --enable_cte_optimization --skip_sql_count_check --ignore=tests/integ/modin/test_sql_counter.py
)

Copy link
Contributor

@sfc-gh-nkumar sfc-gh-nkumar left a comment

Choose a reason for hiding this comment

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

This is great!!

@sfc-gh-jdu
Copy link
Collaborator Author

if this change also affects daily precommit, can you post a successful run for those as well

yes, https://github.com/snowflakedb/snowpark-python/actions/runs/11485902072, though some tests seem failing with an unrelated error:

[31mFAILED�[0m tests/integ/modin/io/test_read_csv.py::�[1mtest_read_csv_usecols_nonexistent_negative[usecols1]�[0m - snowflake.connector.errors.ProgrammingError: 000612 (02000): Result not found

@sfc-gh-jdu sfc-gh-jdu merged commit 475b0be into main Oct 24, 2024
36 checks passed
@sfc-gh-jdu sfc-gh-jdu deleted the jdu-SNOW-1733956-fix-sql-count branch October 24, 2024 18:01
@github-actions github-actions bot locked and limited conversation to collaborators Oct 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
NO-CHANGELOG-UPDATES This pull request does not need to update CHANGELOG.md snowpark-pandas
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants