PCRE matching of string fields #74

Totktonada · 2018-03-10T22:37:04Z

Related to #73.

Khatskevich · 2018-03-13T10:11:04Z

graphql/accessor_general.lua

+        if obj[field_name] == nil then
+            return false
+        end
+        assert(rex ~= nil, 'we should not pass over :compile() ' ..


Is this comment or assert msg?)

Propose one better or I’ll leave it as is.

Khatskevich · 2018-03-13T10:11:57Z

graphql/accessor_general.lua

@@ -696,6 +702,28 @@ local function validate_collections(collections, schemas)
    end
 end

+--- XXX
+local function match_using_re(obj, pcre)
+    if pcre == nil then return true end


Is this situation normal?
I suppose assert is more suitable here and this function is called only if necessary. But it is up to you...

pcre is like filter. You maybe confuse it with rex — module variable.

Khatskevich · 2018-03-13T10:14:04Z

graphql/accessor_general.lua

@@ -741,9 +769,11 @@ local function process_tuple(state, tuple, opts)
                    qstats.fetched_object_cnt, fetched_object_cnt_max))
    assert(qcontext.deadline_clock > clock.monotonic64(),
           'query execution timeout exceeded, use `timeout_ms` to increase it')
+    local collection_name = opts.collection_name


I do not like creating variables for one use.

I do not like unpacking some opts, but don’t unpacking others. Debatable and non-important. Going to leave as is.

Khatskevich · 2018-03-13T10:19:42Z

graphql/accessor_general.lua

@@ -696,6 +702,28 @@ local function validate_collections(collections, schemas)
    end
 end

+--- XXX
+local function match_using_re(obj, pcre)


I prefer regexp name over pcre.
Pcre dis

can be confusing for newcomers

emphasizes implementation.

I don’t know a developer that don’t know PCRE term.

It holds a standard, not an implementation. Consider comment with link to the alternative lua PCRE implementation.

I’m tentative about your proposal. Maybe.

Khatskevich · 2018-03-13T10:25:20Z

graphql/accessor_general.lua

@@ -828,6 +859,8 @@ local function select_internal(self, collection_name, from, filter, args, extra)
    -- XXX: save type at parsing and check here
    --assert(args.offset == nil or type(args.offset) == 'number',
    --    'args.offset must be a number of nil, got ' .. type(args.offset))
+    assert(args.pcre == nil or type(args.pcre) == 'table',


I do not like, that we loose structure of filters.
I suggest:

Create single attribute filter

filter is a table which contains

regexp

brute (i do not insist on the naming, I insist on sense)

index

I believe that this approach

much more extensible

would speed up future coding

do not ban such names of fields like pcre

Not in the scope of this issue.

Khatskevich · 2018-03-13T10:30:45Z

graphql/accessor_general.lua

+        local collection = self.collections[collection_name]
+        local schema = self.schemas[collection.schema_name]
+        for _, field in ipairs(schema.fields) do
+            if field.name == field_name then


What is about rested records?

XXX mark above. Will file an issue re nested records and 1:1 connections (nested filters).

Khatskevich · 2018-03-13T10:32:07Z

graphql/accessor_general.lua

@@ -976,6 +1010,87 @@ local function init_qcontext(accessor, qcontext)
        settings.timeout_ms * 1000 * 1000
 end

+--- XXX
+local function get_primary_key_type(self, collection_name)


rename pk_avro_type

Here all types are avro.

I prefer to omit abbreviations where possible.

Debatable, non-important. Going to leave as is.

Khatskevich · 2018-03-13T10:38:47Z

graphql/accessor_general.lua

+--- connection
+local function get_pcre_argument_type(self, collection_name)
+    local collection = self.collections[collection_name]
+    assert(collection ~= nil, 'cannot found collection ' ..


Please do not write long messages to errors which should not occur. It just makes code-base huge and increases maintenance costs.

cannot found collection %s

This error message is not long. You are about some other assert I guess. Propose your wording.

Khatskevich · 2018-03-13T10:42:26Z

graphql/accessor_general.lua

+                local pcre_field
+                if rex ~= nil then
+                    local pcre_type = get_pcre_argument_type(self, collection_name)
+                    pcre_field = {name = 'pcre', type = pcre_type}


I have wrote above. I do not lile pcre name. Moreover in user-visible part.

I’m going to think about this bunch of problems in the scope of #39. Not now.

Khatskevich · 2018-03-13T10:55:25Z

test/testdata/compound_index_testdata.lua

+        }
+    ]]
+
+    utils.show_trace(function()


What is the purpose of this diff?

Do you read 215c1df comment?

The criteria of correctness is that the following queries should produce the same results. ``` local gql = ... local query = [[ query foo_list($offset: foo_offset) { foo(offset: $offset) { ... } } ]] local variables = { offset = { bar = ..., baz = ..., } } print(yaml.encode(gql:compile(query):execute(variables))) ``` ``` local gql = ... local query = [[ query foo_list($bar: String, $baz: Long) { foo(offset: {bar: $bar, baz: $baz}) { ... } } ]] local variables = { bar = ..., baz = ..., } print(yaml.encode(gql:compile(query):execute(variables))) ```

Related to #73.

It is for make the function general and don't have fears of corner cases. It will loop in case of x = { ofType = x }, but it is anyway incorrect input.

Totktonada added enhancement New feature or request prio1 labels Mar 10, 2018

Totktonada force-pushed the gh-73-support-pcre branch 2 times, most recently from 5951f73 to df60de0 Compare March 12, 2018 11:05

Totktonada self-assigned this Mar 13, 2018

Totktonada force-pushed the gh-73-support-pcre branch from df60de0 to 1e14b99 Compare March 13, 2018 01:03

Khatskevich reviewed Mar 13, 2018

View reviewed changes

Totktonada force-pushed the gh-73-support-pcre branch from 1e14b99 to ecf5b22 Compare March 13, 2018 15:28

Totktonada changed the title ~~WIP: PoC of PCRE match of string fields~~ PCRE matching of string fields Mar 13, 2018

Totktonada requested review from Kasen, Khatskevich and SudoBobo March 13, 2018 15:29

Totktonada mentioned this pull request Mar 13, 2018

Add PCRE matching support for string fields #73

Closed

Totktonada added 3 commits March 14, 2018 00:14

PCRE matching of string fields

beeba2c

Related to #73.

tarantool_graphql::nullable for nesting NonNulls

bc012ff

It is for make the function general and don't have fears of corner cases. It will loop in case of x = { ofType = x }, but it is anyway incorrect input.

Totktonada force-pushed the gh-73-support-pcre branch from ecf5b22 to bc012ff Compare March 13, 2018 21:16

Totktonada merged commit f555d70 into master Mar 13, 2018

Totktonada deleted the gh-73-support-pcre branch March 13, 2018 21:16

Totktonada mentioned this pull request Mar 13, 2018

Support case-insensetive matching #79

Closed

PCRE matching of string fields #74

PCRE matching of string fields #74

Uh oh!

Conversation

Totktonada commented Mar 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Totktonada Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Totktonada commented Mar 10, 2018 •

edited

Loading

Totktonada Mar 13, 2018 •

edited

Loading