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

Allow a Table to be used to rename_columns. #9940

Merged
merged 8 commits into from
May 14, 2024

Conversation

jdunkerley
Copy link
Member

@jdunkerley jdunkerley commented May 14, 2024

Pull Request Description

Following an example from Steve, adjusted rename_column to allow a Table to define the mapping.

  • A single Text column table gives a list of new names.
  • A two Text column table gives a map from old name to new name.
  • If a DB_Table is passed errors and tells the user to materialize the table.

image

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.

@jdunkerley jdunkerley added the CI: No changelog needed Do not require a changelog entry for this PR. label May 14, 2024
Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

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

Looks good, but I think we should test the error scenarios as well (using DB table with DB table and with in-memory table).

Because Enso language is so dynamic, I just can't trust an error scenario that is untested - we've historically had far too many cases of untested error scenarios that actually crashed because of a typo. Well, at least I wouldn't trust myself about it if I had written this without tests.

@@ -2967,7 +2961,7 @@ default_join_condition table join_kind = case join_kind of
## PRIVATE
Table.from (that:DB_Table) =
_ = [that]
Error.throw (Illegal_Argument.Error "Currently cross-backend operations are not supported. Materialize the table using `.read` before mixing it with an in-memory Table.")
Error.throw (Illegal_Argument.Error "This operation requires the data in-memory, materialize the table using `.read`.")
Copy link
Member

Choose a reason for hiding this comment

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

Sounds much better :)

@jdunkerley jdunkerley added CI: Ready to merge This PR is eligible for automatic merge labels May 14, 2024
@mergify mergify bot merged commit 2d6735e into develop May 14, 2024
38 checks passed
@mergify mergify bot deleted the wip/jd/rename-columns-table branch May 14, 2024 15:57
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. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants