FYI some tests are failing also

@tsteur

Multiple action related segments. eg entryPageUrl, pageUrl, pageTitle, ... in one. Ideally sometimes even the same segment multiple times present
Multiple action related segments containing And / or segments.

These two already exist in: https://github.com/matomo-org/matomo/blob/4.x-dev/tests/PHPUnit/Integration/SegmentTest.php#L1427. But they only use pageUrl. I'll add one that does something similar w/ different action segments.

Also ensuring that when we use a == that it speeds up the query correctly where it effectively does a where idaction = $id.

Curently not doing that, but instead, LEFT JOIN ON log_action idaction_url_log_action WHERE idaction_url_log_action.name = ? AND idaction_url_log_action.hash = CRC32(?) AND idaction_url_log_action.type = ?. This should have the same effect but require less queries, correct?

This should also be covered by existing SegmentTest tests.

And ideally, we would maybe also add a system test for some complicated segment if it's not present yet to ensure we get the correct value.

Will add a system test.

@tsteur updated

And in regards to the URL encode. Below segment is actually one from our production account as it is stored in the database. As mentioned in 4.x-dev the bind parameters don't include the HTTP. I see you added the urlencode to fix this. I do wonder though if this could have any implications on already existing stored segments? Could anything regress there?

Reverted this.

FYI trying a segment string like pageUrl=@foo.com;pageUrl!@baz;pageUrl=@test;pageUrl!@hello generates a query like below. Not sure if there's anything we can do to optimise it. I saw this today in production. It's basically about optimising "NOT IN" but maybe it can't be, not sure.

I was thinking the same thing, but it looks like a significant amount of work to look into so I would consider it out of scope here. I could work on it as a follow up PR though, if desired.

I was thinking the same thing, but it looks like a significant amount of work to look into so I would consider it out of scope here. I could work on it as a follow up PR though, if desired.

Agreed 👍

So far things are looking good. I've been doing more testing and more performance testing too and just seen one query that it improves by 80 times! That's because so far according to explain in the past all these subqueries where dependent subqueries and executed for each row very likely which is of course very ineffecient and slow.

The only issue I'm still seeing is that it used to behave differently for the segment entryPageUrl=@https%253A%252F%252Fmatomo.org%252Ffaq%252F,entryPageUrl=@https%253A%252F%252Fmatomo.org%252Fdocs%252F,entryPageUrl=@https%253A%252F%252Fmatomo.org%252Fcontact%252F,entryPageUrl=@https%253A%252F%252Fmatomo.org%252Fhelp%252F,entryPageUrl=@https%253A%252F%252Fmatomo.org%252Ftag-manager-training%252F,entryPageUrl=@https%253A%252F%252Fmatomo.org%252Fweb-analytics-training%252F,entryPageUrl=@https%253A%252F%252Fdeveloper.matomo.org%252F,entryPageUrl=@https%253A%252F%252Fmatomo.org%252Ftranslations%252F,entryPageUrl=@https%253A%252F%252Fmatomo.org%252Fchangelog%252F,entryPageUrl=@https%253A%252F%252Fmatomo.org%252Fsupport%252F,entryPageUrl=@https%253A%252F%252Fmatomo.org%252Fjobs%252F. In 4.x-dev it would have bind parameters like matomo.org vs now it has bind parameters like https://matomo.org. Somehow, it doesn't seem to remove the HTTP protocol prefix anymore. Not sure where or why.

I'm testing this with below test

    public function test_getSelectQuery_whenUrls()
    {
        $select = 'sum(log_visit.idvisit)';
        $from = 'log_visit';
        $where = 'log_visit.idsite = ?';
        $bind = array(1);

        $segment = 'entryPageUrl=@https%253A%252F%252Fmatomo.org%252Ffaq%252F,entryPageUrl=@https%253A%252F%252Fmatomo.org%252Fdocs%252F,entryPageUrl=@https%253A%252F%252Fmatomo.org%252Fcontact%252F,entryPageUrl=@https%253A%252F%252Fmatomo.org%252Fhelp%252F,entryPageUrl=@https%253A%252F%252Fmatomo.org%252Ftag-manager-training%252F,entryPageUrl=@https%253A%252F%252Fmatomo.org%252Fweb-analytics-training%252F,entryPageUrl=@https%253A%252F%252Fdeveloper.matomo.org%252F,entryPageUrl=@https%253A%252F%252Fmatomo.org%252Ftranslations%252F,entryPageUrl=@https%253A%252F%252Fmatomo.org%252Fchangelog%252F,entryPageUrl=@https%253A%252F%252Fmatomo.org%252Fsupport%252F,entryPageUrl=@https%253A%252F%252Fmatomo.org%252Fjobs%252F';
        $segment = new Segment($segment, $idSites = array());

        $query = $segment->getSelectQuery($select, $from, $where, $bind);

        $this->assertQueryDoesNotFail($query);

        $expected = array(
            "sql"  => "....",
            "bind" => array('matomo.org...'));

        $this->assertEquals($this->removeExtraWhiteSpaces($expected), $this->removeExtraWhiteSpaces($query));
    }

And getting these binds. This would not give us any results since the URLs have the protocol removed.

Can you reproduce this @diosmosis ?

@tsteur

Can you reproduce this @diosmosis ?

Yes, I can reproduce. Before this logic was handled by the TableLogAction method, but that isn't invoked when joining the segment. I'll look into a fix.

@tsteur should be fixed

Awesome! Thanks for this @diosmosis . I've just tested this and works nicely. Seeing partially some great performance gains!

This looks all good from my perspective. Not sure if @matomo-org/core-reviewers had any extra thoughts? We'll likely want to cherry pick this also into 4.x-dev as it doesn't seem to be breaking API and would bring us some great performance improvement. I'd create separate issues for this once merged.

@sgiehl updated except for the one comment that still has a question

@sgiehl applied review feedback

@sgiehl yes I did performance checks here and they looked really good 👍 We're looking forward to this improvement a lot.

Once merged we can also run this on production for a while and keep a closer look on how it's impacting things overall in case certain things are causing issues.