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

Define four segments that search over all custom variable name/value columns instead of one per column #8754

Merged
merged 10 commits into from Sep 15, 2015

Conversation

diosmosis
Copy link
Member

This PR creates four new segments: customVariableName, customVariableValue, customVariablePageName, customVariablePageValue. Each one searches over every custom variable slot so it is not necessary to manually string the different custom variable searches together (eg, customVariableName1=...,customVariableName2=...,customVariableName3=...).

To accomplish this, I modified the Segment::setSqlSegment field so it can be an array (though this isn't documented). If found, Piwik\Segment\SegmentExpression will automatically insert ORs between conditions matching the value on each specified table column.

To get the auto suggest API to work, I had to allow the suggestedValuesCallback property Segments to be non-strings and to allow them to accept a DataTable argument. If it does accept such an argument, the Live.getLastVisitDetails data (collected in API::getSuggestedValuesForSegment) is passed to the method.

Also includes a change of base classes for CustomVariableName/CustomVariableValue dimensions so they will be included in the Dimension::getAllDimensions() result. Not sure if this will cause side effects.

The build may not pass yet.

Fixes #6031

@diosmosis diosmosis 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 Sep 11, 2015
@diosmosis diosmosis added this to the 2.15.0 milestone Sep 11, 2015
@tsteur
Copy link
Member

tsteur commented Sep 11, 2015

It is still possible to apply a segment for a specific one right? Can we maybe show a title in the UI that says that we're search in all custom variables for one segment and that results can be inaccurate if the search matches values from different custom variables? If I understand this feature right one has to be very careful using it, maybe even allowing only == comparison? Will comment this on the actual issue since it's not really related to the PR.

// Select also flattened keys (custom variables "page" scope, page URLs for one visit, page titles for one visit)
$valuesBis = $table->getColumnsStartingWith($segmentName . ColumnDelete::APPEND_TO_COLUMN_NAME_TO_KEEP);
$values = array_merge($values, $valuesBis);
}
Copy link
Member

Choose a reason for hiding this comment

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

This long method needs a refactoring one day :) not related to your PR :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I noticed many methods needing refactoring :) I even added a TODO for the first one. Then I saw the rest and lost hope :)

@tsteur
Copy link
Member

tsteur commented Sep 11, 2015

I don't understand everything but looks ok to me :) Are there any changes re performance or so? Of course using the CustomVariableName/Page segment will be slower and I think the changes in AutoSuggestAPI also only affect this segment right?

@diosmosis
Copy link
Member Author

It is still possible to apply a segment for a specific one right?

Yes, still possible. The other ones are added through an event.

Can we maybe show a title in the UI that says that we're search in all custom variables for one segment and that results can be inaccurate if the search matches values from different custom variables? If I understand this feature right one has to be very careful using it, maybe even allowing only == comparison?

This should only be a problem if doing a segment like customVariableValue=.... I don't think anyone will normally do this, favoring instead customVariableName=...;customVariableValue=.... Or just customVariableName=....

Are there any changes re performance or so?

There shouldn't be, I tried to make the old behavior the default in every change. I was most worried about accidental BC breaks.

Of course using the CustomVariableName/Page segment will be slower and I think the changes in AutoSuggestAPI also only affect this segment right?

That's the idea. My changes should always use the fastest code path. The changes to AutoSuggestAPI will partially affect any segment that uses suggestedValuesCallback, but the extra work involved is just creating a ReflectionMethod/ReflectionFunction instance, which I think is trivial.

@diosmosis
Copy link
Member Author

@mattab This is waiting for review and/or merge.

@@ -279,6 +279,12 @@
<row>
<type>dimension</type>
<category>Custom Variables</category>
<name>Custom Variable name</name>
Copy link
Member

Choose a reason for hiding this comment

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

should be Custom Variable name (scope visit)

@mattab
Copy link
Member

mattab commented Sep 14, 2015

Good to merge after fixing the Segment names!

LGTM

mattab pushed a commit that referenced this pull request Sep 15, 2015
Define four segments that search over all custom variable name/value columns instead of one per column
@mattab mattab merged commit 1d805fe into master Sep 15, 2015
@mattab mattab deleted the 6031_cvar_unified_segment branch September 15, 2015 00:18
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants