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

Data analysts should be able to use Text.location_of to find indexes within string using various matchers #3324

Merged
merged 21 commits into from
Mar 12, 2022

Conversation

radeusgd
Copy link
Member

@radeusgd radeusgd commented Mar 7, 2022

Pull Request Description

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

Important Notes

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 dist and ./run watch.

@radeusgd radeusgd self-assigned this Mar 7, 2022
@radeusgd radeusgd force-pushed the wip/radeusgd/text-find-181266029 branch 4 times, most recently from 328e60f to 1c3112d Compare March 9, 2022 20:02
@radeusgd radeusgd changed the title Data analysts should be able to use Text.find to find indexes within string using various matchers Data analysts should be able to use Text.location_of to find indexes within string using various matchers Mar 10, 2022
@radeusgd radeusgd force-pushed the wip/radeusgd/text-find-181266029 branch from 1c3112d to 963fe9a Compare March 10, 2022 12:52
@radeusgd radeusgd marked this pull request as ready for review March 10, 2022 14:49
@radeusgd radeusgd requested a review from 4e6 as a code owner March 10, 2022 14:49
@jdunkerley
Copy link
Member

LGTM - not sure whether we need the IntArrayBuilder over just using a Java List but no harm having it.

@radeusgd
Copy link
Member Author

radeusgd commented Mar 12, 2022

LGTM - not sure whether we need the IntArrayBuilder over just using a Java List but no harm having it.

Fair, but I wanted to avoid the cost of boxing which is incurred by the ArrayList.

I did a very rudimentary benchmark (should use JMH to measure this properly, but just to get a general feeling) and over multiple runs the IntArrayBuilder seems to be more than twice as fast as the ArrayList (sometimes even ~30x faster). So I think it is worth it - while the cost of boxing is low, for such a low level operation it is still visible. Honestly I'm surprised that the standard library does not provide such a helper data structure - seems that such a structure is included in 'extensions' like Guava.

@4e6 4e6 added the CI: Ready to merge This PR is eligible for automatic merge label Mar 12, 2022
@mergify mergify bot merged commit 247b284 into develop Mar 12, 2022
@mergify mergify bot deleted the wip/radeusgd/text-find-181266029 branch March 12, 2022 19:42
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.

3 participants