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-1360263: [Local Testing] Tests for several issues. #2410

Merged
merged 21 commits into from
Oct 10, 2024

Conversation

sfc-gh-jrose
Copy link
Contributor

@sfc-gh-jrose sfc-gh-jrose commented Oct 8, 2024

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

    Fixes SNOW-1360263
    Fixes SNOW-1668862
    Fixes SNOW-1694649
    Fixes SNOW-1707286

  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.
  3. Please describe how your code solves the related issue.

This PR adds tests to verify fixes proposed by community member @tvdboom.

Notes from their PR #2373:

SNOW-1668981

The code block

for k, v in expr_to_alias.items():
if v == exp.child.name:
expr_to_alias[k] = quoted_name

wrongly overwrites the expr_to_alias of the columns in dataframe anti (see minimal reproducible example in #2305). I am unsure when this code block would be desirable. Tests pass without them. Still, better for a maintainer to have a good look at this change. If it's required, we could make it optional under a condition yet to be determined.

SNOW-1360263

The windows variable needs to be ordered by the order of the index in res_index.

SNOW-1668862

Literal columns were always created with nullable=False, which made lit(None) columns fail. Now, lit(None) returns a nullable column.

SNOW-1694649

In the merge statement, filtering on the join_condition resulted in a dataframe with unordered indices. This resulted in a failure when calling new_column[either_isna]. Reseting the index solves the issue.

SNOW-1707286

Can't operate on None and datetime. Return None directly.

jrose Notes:

SNOW-1668981: I've removed the fix for this from the original PR as it appears to also affect live mode and will need a larger change.

SNOW-1360263: The proposed change broke in some edge cases when I ran tests against it. I've amended this part of the change to extract the ordering from the rolling window operation instead.

SNOW-1668862, SNOW-1694649, SNOW-1707286: All look good as is.

@sfc-gh-jrose sfc-gh-jrose requested a review from a team October 8, 2024 00:32
@github-actions github-actions bot added the local testing Local Testing issues/PRs label Oct 8, 2024
@sfc-gh-jrose sfc-gh-jrose changed the title Jrose snow 1668981 local testing tvdboom fixes SNOW-1668981: [Local Testing] Tests for several issues. Oct 8, 2024
@sfc-gh-jrose sfc-gh-jrose changed the title SNOW-1668981: [Local Testing] Tests for several issues. SNOW-1360263: [Local Testing] Tests for several issues. Oct 8, 2024
@sfc-gh-jrose sfc-gh-jrose marked this pull request as ready for review October 8, 2024 22:12
@sfc-gh-jrose sfc-gh-jrose requested a review from a team as a code owner October 8, 2024 22:12
df = test_data.with_column("b", lit(None))

try:
df.write.save_as_table(table_name=table_name, mode="truncate")
Copy link
Contributor

Choose a reason for hiding this comment

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

shall we validate the table after saving?

Comment on lines 93 to 96
@pytest.mark.skipif(
not installed_pandas,
reason="Test requires pandas.",
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably not, will update.

@sfc-gh-jrose sfc-gh-jrose merged commit 43d37a3 into main Oct 10, 2024
34 checks passed
@sfc-gh-jrose sfc-gh-jrose deleted the jrose_snow_1668981_local_testing_tvdboom_fixes branch October 10, 2024 15:50
@github-actions github-actions bot locked and limited conversation to collaborators Oct 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
local testing Local Testing issues/PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants