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

Add replace_text method to In-Memory Table #3793

Merged
merged 6 commits into from
Oct 14, 2022

Conversation

Frizi
Copy link
Contributor

@Frizi Frizi commented Oct 13, 2022

Pull Request Description

Implements https://www.pivotaltracker.com/n/projects/2539304/stories/183415329

Important Notes

Checklist

  • 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.

@CLAassistant
Copy link

CLAassistant commented Oct 13, 2022

CLA assistant check
All committers have signed the CLA.

@Frizi Frizi force-pushed the wip/frizi/table-replace-text-183415329 branch from f896f0a to 0bd225f Compare October 13, 2022 17:03
@Frizi Frizi marked this pull request as ready for review October 14, 2022 08:13
@Frizi Frizi force-pushed the wip/frizi/table-replace-text-183415329 branch from d65cca0 to a01d73d Compare October 14, 2022 08:19
@Frizi Frizi requested a review from 4e6 as a code owner October 14, 2022 10:46
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 to me, I really like the tests 😁

Ideally, I think we should have the replace operation also implemented as a vectorized operation on StringStorage (see for example how Column.like is implemented) - it would likely be quite a bit faster because Column.map is slow due to constant switches between Java and Enso. But I'm not sure if that is in the scope of this task - so I think this is free to go and we can think about the optimizations in the future too.

@Frizi Frizi force-pushed the wip/frizi/table-replace-text-183415329 branch from d023028 to 1e76cd2 Compare October 14, 2022 14:19
@Frizi Frizi force-pushed the wip/frizi/table-replace-text-183415329 branch from 1e76cd2 to 69b40a0 Compare October 14, 2022 14:24
@Frizi Frizi requested a review from radeusgd October 14, 2022 14:43
result.stack.head != Elem.Formatter(typ) && result.stack.nonEmpty
result.stack.nonEmpty && result.stack.head != Elem.Formatter(typ)
Copy link
Member

Choose a reason for hiding this comment

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

This check was originally in wrong order - if result.stack was empty the latter would be false, but before it's checked we'd call result.stack.head which would throw an exception (due to an empty stack).

For some reason the changes in this PR have started triggering this. It is a bit curious why, but I think we should just have it changed to correctly finish the loop if the stack is empty.

@mwu-tow mwu-tow merged commit ce6267f into develop Oct 14, 2022
@mwu-tow mwu-tow deleted the wip/frizi/table-replace-text-183415329 branch October 14, 2022 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants