WIP: Add ranges support #14

thePanz · 2018-04-04T18:05:07Z

Support for [a TO b] ranges
Support for {a TO b} ranges
Support for asymmetric ranges (such as [a TO b})
Support for ranges with dates xxxx-xx-xx format
Support for * in ranges, example: [123 TO *]
Support for quoted string in ranges, example: ["2008-07-28T14:47:31Z" TO NOW]

codecov-io · 2018-04-04T18:07:19Z

Codecov Report

Merging #14 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##             master    #14   +/-   ##
=======================================
  Coverage       100%   100%           
- Complexity      260    279   +19     
=======================================
  Files            47     49    +2     
  Lines           689    743   +54     
=======================================
+ Hits            689    743   +54

Flag	Coverage Δ	Complexity Δ
#all	`100% <100%> (ø)`	`279 <17> (+19)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
lib/Languages/Galach/Tokenizer.php	`100% <ø> (ø)`	`3 <0> (ø)`	⬇️
lib/Languages/Galach/TokenExtractor/Full.php	`100% <100%> (ø)`	`10 <2> (+4)`	⬆️
lib/Languages/Galach/Values/Token/Range.php	`100% <100%> (ø)`	`3 <3> (?)`
lib/Languages/Galach/Generators/Native/Range.php	`100% <100%> (ø)`	`12 <12> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a06fe70...e9ca9e8. Read the comment docs.

lib/Languages/Galach/Values/Token/Range.php

pspanja · 2018-04-05T07:18:36Z

lib/Languages/Galach/Values/Token/Range.php

+
+    /**
+     * @param string $lexeme
+     * @param int    $position


No alignment in PHPDoc, it unnecessarily adds to diff when updated.

I just run the php-cs-fixer with the current settings, I guess it was never run before on the code.
I guess it is a good thing to comply to a CS here :)

pspanja · 2018-04-05T07:21:55Z

lib/Languages/Galach/TokenExtractor/Full.php

@@ -46,43 +48,63 @@ protected function getExpressionTypeMap()
    protected function createTermToken($position, array $data)
    {
        $lexeme = $data['lexeme'];
+        $token = null;


TBH I think Code Climate is a bit too aggressive, this now looks harder to read than before :)

Can you change the code climate settings? :)

OK, updated.

OK, I reverted that change! :)

Now disabled argument count as well. Unfortunately it has no separate config for constructors.

pspanja · 2018-04-05T07:33:28Z

This looks great :) Do you plan to go for the exclusive ranges here as well?

thePanz · 2018-04-05T08:08:44Z

@pspanja the exclusive should be "easy" to achive, but there is something not clear to me in the regexp you're using in the phrase token:
What's the (?<!\\\\) needed for? should it be used in the range too?

And for now, the "208-04-05" format in the range is not supported yet

thePanz · 2018-04-05T08:45:52Z

@pspanja inclusive/exclusive range added, I'll add later the support for the date ranges

pspanja · 2018-04-05T12:56:10Z

What's the (?<!\\\\) needed for? should it be used in the range too?

It's negative lookbehind, see here: http://www.rexegg.com/regex-lookarounds.html
Basically it forbids preceding backslash, useful in phrase matching to allow everything until unescaped quote.

About left and right value in range matching - I think we should somehow reuse the pattern for matching a word. Will take a closer look in the evening.

thePanz · 2018-04-05T18:01:20Z

Thanks for the explanation @pspanja!
Should the range implementation be split into inclusive/exclusive (as this patch) and an additional MR for the other cases?

pspanja · 2018-04-05T21:25:29Z

lib/Languages/Galach/Values/Token/Range.php

+     */
+    public static function getTypeByStart($startSymbol)
+    {
+        if ('[' === $startSymbol) {


Having this check here means if someone customizes the symbol, this class will also need to be changed. So it should be rather done outside, in the Full TokenExtractor implementation.

pspanja · 2018-04-05T21:33:16Z

lib/Languages/Galach/Generators/Native/Range.php

+
+        switch ($token->type) {
+            case RangeToken::TYPE_INCLUSIVE:
+                return $domainPrefix . '[' . $token->rangeFrom . ' TO ' . $token->rangeTo . ']';


Symbols should be captured by the expression and contained in the token, so if they are customized this class does not need to know about it. And we should support mixed case as well, {a TO b] and [a TO b}.

It will probably mean a truckload of constructor arguments, but I'm OK with that :)

pspanja · 2018-04-05T21:40:18Z

Should the range implementation be split into inclusive/exclusive (as this patch) and an additional MR for the other cases?

It can be split, I'm OK with that.

Thinking about the implementation a bit, handling both two-sided and one-sided ranges seems like too much both for the regex and token, so should probably be handled with a separate pattern and a separate token.

TomasPilar · 2018-09-08T14:57:40Z

Hi guys, thank you for this cool PR!
@pspanja Can it be merged?

pspanja · 2018-10-09T17:58:43Z

Hi @TomasPilar, I'll try to find time in the next few days to see how to push this further.

thePanz · 2018-10-09T20:50:48Z

FYI: this MR has been used in a pre-production system from April 6 :)

j13k · 2019-06-18T06:33:52Z

This looks like a very useful extension — any chance it will be merged/released in the near future?

pspanja · 2019-06-19T09:31:50Z

Hi @j13k, I've neglected this because I was too busy, but I'll find time in the next weeks to take care of it. It's obviously needed :)

j13k · 2019-06-20T02:17:16Z

Hi @j13k, I've neglected this because I was to busy, but I'll find time in the next weeks to take care of it. It's obviously needed :)

That's fine @pspanja, I know what it's like! I'm struggling to find time for an outstanding PR on a small project that I maintain. ;)

Just so you know, it's a very useful library and really well-executed. I've implemented a set of generator classes that convert queries to Elasticsearch DSL.

thePanz · 2019-06-20T07:36:44Z

I agree with @j13k , this library was very useful for me to:

validate the user-query (parsing the query from a string and building the corresponding tree)
re-write the query-tree with the specific field names in SOLR (the user doesn't know the real fields the query will be executed on, like the specific SOLR pre- or sub- fixes)

ilukac · 2019-06-21T09:22:29Z

@thePanz @j13k feel free to PR whatever you think is generally useful :)

drigolin · 2022-04-22T13:13:32Z

Any plan to merge this PR? Thank you.

thePanz mentioned this pull request Apr 4, 2018

Implement support for ranges #2

Open

pspanja reviewed Apr 5, 2018

View reviewed changes

lib/Languages/Galach/Values/Token/Range.php Outdated Show resolved Hide resolved

pspanja reviewed Apr 5, 2018

View reviewed changes

Add support for range [a TO b]

6af5e8b

thePanz force-pushed the add-ranges-support branch from feb20c4 to 6af5e8b Compare April 5, 2018 07:57

Added inclusive and exclusive range tokenization

285543d

thePanz force-pushed the add-ranges-support branch from 25e49ce to 285543d Compare April 5, 2018 09:11

Add Range node generator and tests

a7c3867

thePanz force-pushed the add-ranges-support branch from 6ce8597 to a7c3867 Compare April 5, 2018 18:06

pspanja reviewed Apr 5, 2018

View reviewed changes

thePanz force-pushed the add-ranges-support branch from 528b2b4 to 0aee897 Compare April 6, 2018 08:26

Refactor range start/end symbol handling, allow asymmetric ranges

771b486

thePanz force-pushed the add-ranges-support branch from 0aee897 to 771b486 Compare April 6, 2018 08:30

thePanz added 2 commits April 6, 2018 10:38

Allow dates and * for ranges

f3d3f26

Initial support for ranges with quotes

e9ca9e8

patdunlavey mentioned this pull request Jul 4, 2022

Add ranges support thePanz/query-translator#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Add ranges support #14

WIP: Add ranges support #14

thePanz commented Apr 4, 2018 •

edited

Loading

codecov-io commented Apr 4, 2018 •

edited

Loading

pspanja Apr 5, 2018

thePanz Apr 5, 2018

pspanja Apr 5, 2018

thePanz Apr 5, 2018

pspanja Apr 5, 2018

thePanz Apr 5, 2018

pspanja Apr 5, 2018

pspanja commented Apr 5, 2018

thePanz commented Apr 5, 2018

thePanz commented Apr 5, 2018

pspanja commented Apr 5, 2018

thePanz commented Apr 5, 2018

pspanja Apr 5, 2018

pspanja Apr 5, 2018

pspanja commented Apr 5, 2018

TomasPilar commented Sep 8, 2018

pspanja commented Oct 9, 2018

thePanz commented Oct 9, 2018

j13k commented Jun 18, 2019

pspanja commented Jun 19, 2019 •

edited

Loading

j13k commented Jun 20, 2019

thePanz commented Jun 20, 2019

ilukac commented Jun 21, 2019

drigolin commented Apr 22, 2022

WIP: Add ranges support #14

Are you sure you want to change the base?

WIP: Add ranges support #14

Conversation

thePanz commented Apr 4, 2018 • edited Loading

codecov-io commented Apr 4, 2018 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pspanja commented Apr 5, 2018

thePanz commented Apr 5, 2018

thePanz commented Apr 5, 2018

pspanja commented Apr 5, 2018

thePanz commented Apr 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pspanja commented Apr 5, 2018

TomasPilar commented Sep 8, 2018

pspanja commented Oct 9, 2018

thePanz commented Oct 9, 2018

j13k commented Jun 18, 2019

pspanja commented Jun 19, 2019 • edited Loading

j13k commented Jun 20, 2019

thePanz commented Jun 20, 2019

ilukac commented Jun 21, 2019

drigolin commented Apr 22, 2022

thePanz commented Apr 4, 2018 •

edited

Loading

codecov-io commented Apr 4, 2018 •

edited

Loading

pspanja commented Jun 19, 2019 •

edited

Loading