Change filtering language to MQL #84

foglcz · 2018-08-28T10:43:25Z


Type	Feature
Fixes issues	RAD-1689
Documentation	updated
BC Break	YES
Tests updated	yes
Notes	Requires #82 to be merged first.

Adds support for Matej Query Language in the recommendation requests, which is now default way of sending requests.

Removes default filter.

Legacy filtering using regexes can be done by setting filter type to FILTER_TYPE_RGX. In the request itself, the legacy way of filtering is implemented by absence of parameter filter_type. However, we don't want to encourage Matej users to use the RGX filter, as MQL is overall faster, and implements more features. Therefore, I did not mention this explicitly in the documentation.

jirinovak · 2018-08-28T22:35:55Z

CHANGELOG.md

 ### Fixed
 - Exceptions occurring during async request now generate rejected promise (as they should) and are no longer thrown directly.

+### Added
+- `UserRecommendation` now sends which item properties should be returned alongside with item_ids.
+- **BC BREAK** | `UserRecommendation` now uses MQL query language by default for filtering.


Maybe this should be in Changed section.

jirinovak · 2018-08-28T22:42:40Z

README.md

+
+```php
+$recommendation = UserRecommendation::create('user-id', 5, 'test-scenario', 1.0, 3600, ['item_id', 'item_url')
+    ->addProperty('item_title')


typo addResponseProperty according to text above

Good catch! thanks :)

jirinovak · 2018-08-28T23:07:20Z

README.md

+// }
+```
+
+Matej will always return `item_id`, even if you don't ask for it explicitly.


What will be return when i do not request any response properties? Only item_id or all item properties?

Matej will return only item_id - but in the new format of array of stdClass-es.

I've updated the doc to be more explicit.

OndraM

I'm missing some examples and documentation of the filters in README, do you plan to add it? (or later?)

OndraM · 2018-08-29T10:37:19Z

tests/unit/Model/Command/UserRecommendationTest.php

@@ -63,6 +65,7 @@ public function shouldUseCustomParameters(): void
                    'hard_rotation' => true,
                    'min_relevance' => UserRecommendation::MINIMAL_RELEVANCE_HIGH,
                    'filter' => 'foo = bar and baz = ban',
+                    // when a filter type is RGX, the parameter is absent.


which parameter?

(I know which, but its not obvious from the comment :) )

OndraM · 2018-08-29T10:50:21Z

src/Model/Command/UserRecommendation.php

+     */
+    public function setFilterType(string $filterType): self
+    {
+        Assertion::typeIdentifier($filterType);


Also assert the allowed values:

Assertion::choice( $filterType, [static::FILTER_TYPE_RGX, static::FILTER_TYPE_MQL] );

(like we do in setMinimalRelevance() etc.)

OndraM · 2018-08-29T10:52:29Z

src/Model/Command/UserRecommendation.php

@@ -30,7 +32,9 @@ class UserRecommendation extends AbstractCommand implements UserAwareInterface
    /** @var string */
    private $minimalRelevance = self::MINIMAL_RELEVANCE_LOW;
    /** @var string[] */
-    private $filters = ['valid_to >= NOW'];
+    private $filters = [];


Also phpdoc of addFilter() and setFilters() method needs to be updated to reflect this change (= there is no default filter).

OndraM · 2018-08-29T10:56:28Z

tests/unit/Model/Command/UserRecommendationTest.php

@@ -77,14 +80,14 @@ public function shouldAssembleFilters(): void
        $command = UserRecommendation::create('user-id', 333, 'test-scenario', 1.0, 3600);

        // Default filter
-        $this->assertSame('valid_to >= NOW', $command->jsonSerialize()['parameters']['filter']);
+        $this->assertSame('', $command->jsonSerialize()['parameters']['filter']);

        // Add custom filters to the default one
        $command->addFilter('foo = bar')


This test method tests just the legacy regexp filters, right?

I'd thus rename it (shouldAssembleLegacyRgxFilters() or something like this) and add another one using some basic MQL filters. Test serve as documentation, and right now, there is no testcase actually using MQL filters.

foglcz · 2018-08-30T08:50:10Z

Updated the PR as per rebiew remarks.
ping @OndraM @jirinovak

jirinovak · 2018-08-30T19:16:09Z

tests/unit/Model/Command/UserRecommendationTest.php

+                "third_string_property NOT LIKE '%bar' and " .
+                'bool_property = true and ' .
+                'nullable_property IS NULL and ' .
+                "'some_value' in set_property",


Maybe NOWDOC would be more readable.

Or at least i would align ".

OndraM · 2018-08-31T11:15:12Z

src/Model/Command/UserRecommendation.php

+    /**
+     * Specify the filter type being used.
+     */
+    public function setFilterType(string $filterType): self


This whole method seems uncovered by unit tests. I think there should be (simple) testcase for the rgx type.

After discussion, we've removed this one :)

OndraM · 2018-08-31T13:23:13Z

Marged to master, thanks @foglcz 💯

foglcz added 2.0 bc-break labels Aug 28, 2018

foglcz requested review from OndraM, dudla and VincTheSecond August 28, 2018 10:43

foglcz force-pushed the feature/new-query-language branch from d0aa3be to 78f90dc Compare August 28, 2018 10:46

jirinovak requested changes Aug 28, 2018

View reviewed changes

OndraM requested changes Aug 29, 2018

View reviewed changes

foglcz force-pushed the feature/new-query-language branch 3 times, most recently from 3084d26 to e56fd1f Compare August 30, 2018 08:49

foglcz changed the base branch from master to feature/item-properties-in-recommendation-requests August 30, 2018 09:32

jirinovak approved these changes Aug 30, 2018

View reviewed changes

foglcz force-pushed the feature/item-properties-in-recommendation-requests branch from c3e9134 to af3d80e Compare August 31, 2018 08:09

foglcz force-pushed the feature/new-query-language branch from e56fd1f to 71fb7b4 Compare August 31, 2018 08:14

foglcz force-pushed the feature/item-properties-in-recommendation-requests branch 2 times, most recently from 98460d3 to 41a1327 Compare August 31, 2018 08:19

foglcz force-pushed the feature/new-query-language branch 2 times, most recently from ebdbbbe to 44fe4cf Compare August 31, 2018 08:22

OndraM requested changes Aug 31, 2018

View reviewed changes

foglcz force-pushed the feature/new-query-language branch from 44fe4cf to 4c51c00 Compare August 31, 2018 12:26

foglcz force-pushed the feature/item-properties-in-recommendation-requests branch from 41a1327 to a3e5789 Compare August 31, 2018 12:31

OndraM changed the title ~~Add MQL support for recommendations filtering~~ Change filtering language to MQL Aug 31, 2018

Implement support for Matej Query Language in recommendation requests.

92d29c3

foglcz force-pushed the feature/new-query-language branch from 4c51c00 to 92d29c3 Compare August 31, 2018 12:32

OndraM changed the base branch from feature/item-properties-in-recommendation-requests to master August 31, 2018 13:12

OndraM approved these changes Aug 31, 2018

View reviewed changes

OndraM merged commit c66a7e3 into master Aug 31, 2018

OndraM deleted the feature/new-query-language branch September 4, 2018 14:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change filtering language to MQL #84

Change filtering language to MQL #84

foglcz commented Aug 28, 2018 •

edited

Loading

jirinovak Aug 28, 2018

jirinovak Aug 28, 2018

foglcz Aug 30, 2018

jirinovak Aug 28, 2018

foglcz Aug 30, 2018

OndraM left a comment

OndraM Aug 29, 2018

OndraM Aug 29, 2018

OndraM Aug 29, 2018

OndraM Aug 29, 2018

foglcz commented Aug 30, 2018

jirinovak Aug 30, 2018

OndraM Aug 31, 2018

foglcz Aug 31, 2018

OndraM commented Aug 31, 2018

Change filtering language to MQL #84

Change filtering language to MQL #84

Conversation

foglcz commented Aug 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OndraM left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

foglcz commented Aug 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OndraM commented Aug 31, 2018

foglcz commented Aug 28, 2018 •

edited

Loading