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

New set function, parse a column #4097

Merged
merged 26 commits into from
Jan 31, 2023
Merged

New set function, parse a column #4097

merged 26 commits into from
Jan 31, 2023

Conversation

jdunkerley
Copy link
Member

@jdunkerley jdunkerley commented Jan 30, 2023

Pull Request Description

  • New set function design - takes a Column and works with that more easily and supports control of Set_Mode.
  • New simple parse API on Column.
  • Separated expression support for filter to new filter_by_expression on Table.
  • New compute function allowing creation of a column from an expression.
  • Added case sensitivity argument to Column based on starts_with, ends_with and contains.
  • Added case sensitivity argument to Filter_Condition for Starts_With, Ends_With, Contains and Not_Contains.
  • Fixed the issue in JS Table visualisation where JavaScript date was incorrectly set.
  • Some dynamic dropdown expressions - experimenting with ways to use them.
  • Fixed issue with .pretty that wasn't escaping \.
  • Changed default Postgres DB to postgres.
  • Fixed SQLite support for starts_with, ends_with and contains to be consistent (using GLOB not LIKE).

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 force-pushed the wip/jd/column-parse branch 2 times, most recently from 6621ad1 to 1ad5f13 Compare January 31, 2023 11:01
@jdunkerley jdunkerley marked this pull request as ready for review January 31, 2023 11:01
Copy link
Contributor

@farmaazon farmaazon left a comment

Choose a reason for hiding this comment

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

Approval of table visualization file.

## PRIVATE
Gets a widget set up for a Filter_Condition.
widget_for_filter_condition =
## values = ["(Filter_Condition.Equal to=_)", "(Filter_Condition.Not_Equal to=_)", "(Filter_Condition.Is_In values=_)", "(Filter_Condition.Not_In values=_)", "Filter_Condition.Is_True", "Filter_Condition.Is_False", "Filter_Condition.Is_Nothing", "Filter_Condition.Not_Nothing", "Filter_Condition.Is_Empty", "Filter_Condition.Not_Empty", "(Filter_Condition.Less than=_)", "(Filter_Condition.Equal_Or_Less than=_)", "(Filter_Condition.Greater than=_)", "(Filter_Condition.Equal_Or_Greater than=_)", "(Filter_Condition.Between lower=_ upper=_)", "(Filter_Condition.Starts_With prefix=_)", "(Filter_Condition.Ends_With suffix=_)", "(Filter_Condition.Contains substring=_)", "(Filter_Condition.Not_Contains substring=_)", "(Filter_Condition.Like pattern=_)", "(Filter_Condition.Not_Like pattern=_)"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this commented out value still needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Preferred this form but it caused IDE issues so stuck with the simpler form.
Kept for now in case want it as widgets develop.

CHANGELOG.md Outdated Show resolved Hide resolved
## PRIVATE
Gets a widget set up for a Filter_Condition.
widget_for_filter_condition =
## values = ["(Filter_Condition.Equal to=_)", "(Filter_Condition.Not_Equal to=_)", "(Filter_Condition.Is_In values=_)", "(Filter_Condition.Not_In values=_)", "Filter_Condition.Is_True", "Filter_Condition.Is_False", "Filter_Condition.Is_Nothing", "Filter_Condition.Not_Nothing", "Filter_Condition.Is_Empty", "Filter_Condition.Not_Empty", "(Filter_Condition.Less than=_)", "(Filter_Condition.Equal_Or_Less than=_)", "(Filter_Condition.Greater than=_)", "(Filter_Condition.Equal_Or_Greater than=_)", "(Filter_Condition.Between lower=_ upper=_)", "(Filter_Condition.Starts_With prefix=_)", "(Filter_Condition.Ends_With suffix=_)", "(Filter_Condition.Contains substring=_)", "(Filter_Condition.Not_Contains substring=_)", "(Filter_Condition.Like pattern=_)", "(Filter_Condition.Not_Like pattern=_)"]
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of the commented out line? If we want to include it, I'd suggest explaining what this alternative code means.

Copy link
Member Author

Choose a reason for hiding this comment

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

Kept for now as an alternative experiment. I don't expect either of these lines to be here for much longer once we get to proper structure being passed.

@@ -242,7 +242,7 @@ agg_count_distinct_include_null args =

## PRIVATE
Copy link
Member

Choose a reason for hiding this comment

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

I was surprised to learn that while Postgres has starts_with, there's no ends_with 😮

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, stoopid.

text = [starts_with, contains, ends_with, fold_case, make_case_sensitive]+concat_ops
text = [starts_with, contains, ends_with, make_case_sensitive]+concat_ops
Copy link
Member

Choose a reason for hiding this comment

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

Why is it removed?

Copy link
Member Author

@jdunkerley jdunkerley Jan 31, 2023

Choose a reason for hiding this comment

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

Using the dialect was causing other issues with making the behaviour between the 3 (SQLite, Postgres, InMemory) act consistently. Not using and relying on the LOWER(UPPER(...)) produced consistent results.

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.

Awesome changes! 🎉

I'm really glad on how the new Table.set is looking, I think it's a good direction. Column.parse and Table.compute are also very cool.

Just some comments in line, I think it would be great to add a few tests.

distribution/lib/Standard/Base/0.0.0-dev/src/Metadata.enso Outdated Show resolved Hide resolved
new_column = Expression.evaluate expression get_column make_constant "Standard.Table.Data.Column" "Column" Column.var_args_functions
if new_column.is_error then new_column else
warnings = Warning.get_all new_column
rename = new_column.rename expression
Copy link
Member

Choose a reason for hiding this comment

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

I see that here we are not 'sanitizing' the column name. Shall we try to make the generated column names consistent? Maybe let's sanitize them on both sides then? Ideally using a shared function, probably from a shared module like Table_Helpers.

I remember we had some task to do at some point to ensure that names of columns generated by operations on columns (like + etc.) should be consistent between the two backends (currently they are not); so ideally these new operations should not introduce new inconsistencies.

distribution/lib/Standard/Table/0.0.0-dev/src/Errors.enso Outdated Show resolved Hide resolved
t2.at "floats" . to_vector . should_equal c2
t2.at "bools" . to_vector . should_equal c3

Test.group "Column.parse" <|
Copy link
Member

Choose a reason for hiding this comment

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

I think I don't see any tests for column.parse Auto. Shall we add one?

@jdunkerley jdunkerley added the CI: Ready to merge This PR is eligible for automatic merge label Jan 31, 2023
@mergify mergify bot merged commit 0790ce4 into develop Jan 31, 2023
@mergify mergify bot deleted the wip/jd/column-parse branch January 31, 2023 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants