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

Fix Sorting in Snowflake #11636

Merged
merged 1 commit into from
Nov 28, 2024
Merged

Fix Sorting in Snowflake #11636

merged 1 commit into from
Nov 28, 2024

Conversation

AdRiley
Copy link
Member

@AdRiley AdRiley commented Nov 22, 2024

Pull Request Description

Fixes #10835

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.
  • If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,
    or the Snowflake database integration, a run of the Extra Tests has been scheduled.
    • If applicable, it is suggested to paste a link to a successful run of the Extra Tests.

@AdRiley AdRiley changed the title Fixe Sorting in Snowflake Fix Sorting in Snowflake Nov 22, 2024
@AdRiley AdRiley added the CI: No changelog needed Do not require a changelog entry for this PR. label Nov 22, 2024
@radeusgd
Copy link
Member

Can we add a test for case insensitive ordering ? As mentioned in the ticket we currently seem to be lacking one.

It'd be good to have one so that we avoid regressions, and ensure that we correctly handle this edge case when adding future dialects.

I was originally planning to add a test case as part of this ticket, if we want to add such test in separate PR I'm not against that, but let's then create a ticket to keep track of it - otherwise I'd be worried we may forget to do it.

@jdunkerley jdunkerley merged commit cd31e16 into develop Nov 28, 2024
37 of 43 checks passed
@jdunkerley jdunkerley deleted the wip/adr/fix-snowflake-sorting branch November 28, 2024 09:23
@AdRiley
Copy link
Member Author

AdRiley commented Nov 28, 2024

I think there is a case insensitive test

    group_builder.specify "should support natural and case insensitive ordering at the same time" <|
        t1 = data.table.sort [..Name "psi"] text_ordering=(..Case_Insensitive sort_digits_as_numbers=True)
        case setup.flagged ..Supports_Sort_Digits_As_Numbers of
            True -> t1.at "psi" . to_vector . should_equal [Nothing, "c01", "C2", "c10"]
            False -> t1.should_fail_with (Unsupported_Database_Operation.Error "sort_digits_as_numbers")

        t2 = data.table.sort [..Name "psi"] text_ordering=(..Default sort_digits_as_numbers=True)
        case setup.flagged ..Supports_Sort_Digits_As_Numbers of
            True -> t2.at "psi" . to_vector . should_equal [Nothing, "C2", "c01", "c10"]
            False -> t2.should_fail_with (Unsupported_Database_Operation.Error "sort_digits_as_numbers")

        t3 = data.table.sort [..Name "psi"] text_ordering=(..Case_Insensitive)
        t3.at "psi" . to_vector . should_equal [Nothing, "c01", "c10", "C2"]

        t4 = data.table.sort [..Name "psi"]
        case setup.flagged ..Case_Insensitive_Ordering of
            True -> t4.at "psi" . to_vector . should_equal [Nothing, "c01", "c10", "C2"]
            False -> t4.at "psi" . to_vector . should_equal [Nothing, "C2", "c01", "c10"]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Table.order_by fails on Snowflake if ..Case_Sensitive ordering is specified
5 participants