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.

Sign up for GitHub

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

Jump to bottom

Update rename_columns to new API design, add first_row, second_row and last_row functions to the table. #5719

Merged
merged 12 commits into from
Feb 23, 2023
Merged

Update rename_columns to new API design, add first_row, second_row and last_row functions to the table. #5719

merged 12 commits into from
Feb 23, 2023

Conversation

jdunkerley
Copy link
Member

@jdunkerley jdunkerley commented Feb 21, 2023 •
edited
Loading

Pull Request Description

  • Updates the rename_columns API.
  • Add first_row, second_row and last_row to the Table types.
  • New option for reading only last row of ResultSet.

Notes

  • Text Matcher and Regex Matcher still in use within Text methods but removed from everywhere else.
  • Not using ResultSet.last as SQLite doesn't support scrollable cursors so went for iterative approach.
  • Weird issue with integration test where ["A","B"].contains wouldn't work for some reason.

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the
    Scala,
    Java,
    and
    Rust
    style guides.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH
      ./run ide build and ./run ide watch.

@jdunkerley jdunkerley linked an issue Feb 21, 2023 that may be closed by this pull request
Retire the Text_Matcher and Regex_Matcher and update the column selectors. #5124
Closed
@jdunkerley jdunkerley changed the title Wip/jd/5124 retire the text matcher and regex matcher and update the column selectors Update rename_columns to new API design, add first_row, second_row and last_row functions to the table. Feb 21, 2023
@jdunkerley jdunkerley marked this pull request as ready for review February 21, 2023 17:41
@jdunkerley jdunkerley requested a review from radeusgd as a code owner February 21, 2023 17:41
@jdunkerley jdunkerley force-pushed the wip/jd/5124-retire-the-text_matcher-and-regex_matcher-and-update-the-column-selectors branch from 7625859 to 51b8f2c Compare February 23, 2023 11:47
@jdunkerley jdunkerley force-pushed the wip/jd/5124-retire-the-text_matcher-and-regex_matcher-and-update-the-column-selectors branch 2 times, most recently from 420d559 to 69a5d55 Compare February 23, 2023 15:36
@jdunkerley jdunkerley force-pushed the wip/jd/5124-retire-the-text_matcher-and-regex_matcher-and-update-the-column-selectors branch from 69a5d55 to cc262ad Compare February 23, 2023 15:40
radeusgd
radeusgd approved these changes Feb 23, 2023
View reviewed changes
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

distribution/lib/Standard/Database/0.0.0-dev/src/Data/Table.enso Outdated Show resolved Hide resolved
distribution/lib/Standard/Database/0.0.0-dev/src/Connection/Connection.enso Outdated Show resolved Hide resolved
distribution/lib/Standard/Database/0.0.0-dev/src/Data/Table.enso Outdated
Comment on lines 725 to 731
last_row : Row ! Index_Out_Of_Bounds
last_row self =
if self.internal_columns.is_empty then Error.throw (Illegal_Argument.Error "Cannot create a table with no columns.") else
sql = self.to_sql
expected_types = self.internal_columns.map .sql_type
self.connection.read_statement sql expected_types True . rows . first
Copy link
Member

Choose a reason for hiding this comment

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

This is done in completely different way than the others. I'm wondering if we shouldn't revisit this once we get take/drop, so that we can use more consistent code for all cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes I think when we come to take/drop we will want to do something cleverer.

distribution/lib/Standard/Database/0.0.0-dev/src/Internal/Result_Set.enso
column_builders = column_types.map create_builder
case last_row_only of
True ->
## Not using the `ResultSet.last` as not supported by all connection types.
Copy link
Member

Choose a reason for hiding this comment

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

I really appreciate the comment!

distribution/lib/Standard/Database/0.0.0-dev/src/Internal/Result_Set.enso
Comment on lines +120 to +142
fetch_and_append self rs i =
value = self.fetch_value rs i
self.append value

## PRIVATE
Fetches the value of ith column from the current row of the result set
fetch_value : ResultSet -> Integer -> Any
fetch_value self rs i =
value = case self of
Builder.Builder_Inferred _ -> rs.getObject i
Builder.Builder_Boolean _ -> rs.getBoolean i
Builder.Builder_Long _ -> rs.getLong i
Builder.Builder_Double _ -> rs.getDouble i
if rs.wasNull then Nothing else value

## PRIVATE
append : Any -> Nothing
append self value = if value.is_nothing then self.java_builder.appendNulls 1 else
case self of
Builder.Builder_Inferred _ -> self.java_builder.append value
Builder.Builder_Boolean _ -> self.java_builder.appendBoolean value
Builder.Builder_Long _ -> self.java_builder.appendLong value
Builder.Builder_Double _ -> self.java_builder.appendDouble value
Copy link
Member

Choose a reason for hiding this comment

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

This looks so much cleaner! 🙂

@jdunkerley jdunkerley added the CI: Ready to merge This PR is eligible for automatic merge label Feb 23, 2023
@jdunkerley
Merge branch 'develop' into wip/jd/5124-retire-the-text_matcher-and-r…
c360fc1 
…egex_matcher-and-update-the-column-selectors
@mergify mergify bot merged commit 652b8d5 into develop Feb 23, 2023
@mergify mergify bot deleted the wip/jd/5124-retire-the-text_matcher-and-regex_matcher-and-update-the-column-selectors branch February 23, 2023 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@radeusgd radeusgd radeusgd approved these changes

@GregoryTravis GregoryTravis GregoryTravis approved these changes

Assignees
No one assigned
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Retire the Text_Matcher and Regex_Matcher and update the column selectors.
3 participants
@jdunkerley @radeusgd @GregoryTravis