Skip to content

Use regular expressions to parse segment string into segment logical tree #21669

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

Merged

Conversation

michalkleiner
Copy link
Contributor

Description:

Tweak to parseTree method using regular expressions

Review

@michalkleiner michalkleiner requested review from sgiehl and bx80 December 11, 2023 10:37
@michalkleiner michalkleiner added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Dec 11, 2023
@michalkleiner michalkleiner requested a review from a team December 11, 2023 12:43
Copy link
Contributor

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Dec 19, 2023
@michalkleiner michalkleiner removed the Stale The label used by the Close Stale Issues action label Dec 19, 2023
Copy link
Contributor

@bx80 bx80 left a comment

Choose a reason for hiding this comment

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

This produced the exact same output in my tests and is more efficient, nice 👍

@sgiehl sgiehl added this to the 5.0.1 milestone Dec 19, 2023
@bx80 bx80 merged commit 48fae99 into 20467-segment-not-in-optimizations Dec 21, 2023
@bx80 bx80 deleted the 20467-parse-tree-negative-lookbehind branch December 21, 2023 03:58
sgiehl added a commit that referenced this pull request Jan 4, 2024
* refactor segment parsing to use a tree structure for logical operators internally instead of flat structure (this is to make re-ordering easier)

* first optimization: merge subquery segment conditions together when they are within the same OR sequence or are alone within the same AND sequence

* remove todo

* fix test and adjust segmentformatter to use new segment expression tree structure

* update expected test output

* fix CustomVariables plugin

* remove code redundancy and add some docs

* convert backslash escaped operator characters to urlencoded characters so they are still supported

* refactor segment parsing to use a tree structure for logical operators internally instead of flat structure (this is to make re-ordering easier)

* first optimization: merge subquery segment conditions together when they are within the same OR sequence or are alone within the same AND sequence

* remove todo

* fix test and adjust segmentformatter to use new segment expression tree structure

* update expected test output

* remove code redundancy and add some docs

* convert backslash escaped operator characters to urlencoded characters so they are still supported

* Update submodule reference

* Fix for segment not supported exception for MATCH_IDVISIT_NOT_IN expressions

* Update submodule

* correctly escape segment value

* fix tests

* Rework parseTree to avoid removing non-escaping backslashes

* Update submodule

* Update tests (minor whitespace change)

* Temporarily reworked new behaviour to be behind a feature flag for additional testing

* Revert feature flag

* Use regular expressions to parse segment string into segment logical tree (#21669)

Co-authored-by: Ben Burgess <88810029+bx80@users.noreply.github.com>

* fix cs

---------

Co-authored-by: Ben <ben.burgess@innocraft.com>
Co-authored-by: sgiehl <stefan@matomo.org>
Co-authored-by: Ben Burgess <88810029+bx80@users.noreply.github.com>
Co-authored-by: Michal Kleiner <michal@innocraft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Development

Successfully merging this pull request may close these issues.

3 participants