Index lookup by a partial key #65

Totktonada · 2018-03-03T23:50:05Z

Khatskevich

Watched all except last two commits

Khatskevich · 2018-03-05T16:03:51Z

test/shard_servers/master.lua

@@ -1,5 +1,7 @@
 #!/usr/bin/env tarantool


There is a typo in commit message (tarantoo) 08529e9

Khatskevich · 2018-03-05T16:03:54Z

test/shard_servers/master.lua

@@ -1,5 +1,7 @@
 #!/usr/bin/env tarantool

+shard_initialized = false


Can the variable be local for the module?

Thanks! Fixed.

Khatskevich · 2018-03-05T16:23:41Z

test/testdata/compound_index_testdata.lua

+        local variables_1_3 = {user_str = 'user_str_b'}
+        local result = gql_query_1:execute(variables_1_3)
+        results = results .. print_and_return(
+            ('RESULT\n%s'):format(yaml.encode(result)))


Please, If you write rests this way, at least give any unique identifiers to its output.
What I see in a result file is a lot of data and I can not find there the result of the particular query.

Those tests do not check if new functions work correctly(
Tests on added feature needed.

Done in the latter commit.

We check here that functionality was not broken after the change. Index lookup check depends on metainfo (Add query meta #71), will be tracked by Test we use index lookup as expected for top-level collections #72.

@Khatskevich Are you about that here?

Khatskevich · 2018-03-05T16:26:06Z

test/testdata/compound_index_testdata.lua

@@ -231,6 +231,13 @@ function compound_index_testdata.run_queries(gql_wrapper)
            ('RESULT\n%s'):format(yaml.encode(result)))
    end)

+    utils.show_trace(function()
+        local variables_1_3 = {user_str = 'user_str_b'}


This test tests that the result is correct (user_str is user_str_b), however, does it test if the index is really used?

Will be done in the scope of non-prio1 issues, tracked by #72.

Khatskevich · 2018-03-05T20:22:26Z

graphql/accessor_general.lua

+                get_best_matched_index(successor_node, new_filter)
+            if branch_index_name ~= nil and branch_len > max_len then
+                index_name = branch_index_name
+                max_len = branch_len


Should max_len = max_len + 1 be moved here?

Nope. It needed for comparing branches. I’ll rename it to max_branch_len to avoid confusion.

Also I’ll change description of len return argument as (matched parts + 1) and will describe the reason why.

Moved branch_len = branch_len + 1 to here.

Khatskevich · 2018-03-05T21:28:55Z

graphql/accessor_general.lua

+--- `value_list`
+---
+--- @treturn table `value_list` (optional) is values list from the `filter`
+--- argument ordered in the such way that can be passed to the found index (has


in such way that it can be passed

Khatskevich · 2018-03-06T11:08:40Z

graphql/accessor_general.lua

--- @treturn table `value_list` is values list from the `filter` argument
--- ordered in the such way that can be passed to the found index (has some
--- meaning only when `index_name ~= nil`)
+--- @treturn table `new_filter` is the filter value w/o values extracted into


I suggest name brute_filter (or scan_filter) for more semantics

I generally agree that common names are bad for domain-specific things. I’ll consider post_filter names or smth like so when there will less pending PRs.

There are related questions / points:

post_filter (postprocessing filter) name will loose some of its meaning once we’ll support pushdown filtering to sotrage nodes.

There are two other things of the same (at least for now) structure: pivot.filter (slow case of offset) and determinator of union-type connections (Support destination_collection deducible from a parent object #8).

Khatskevich · 2018-03-06T11:22:16Z

graphql/accessor_general.lua

@@ -402,6 +441,34 @@ local function build_lookup_index_name(indexes)
    return lookup_index_name
 end

+--- Build `parts_tree` to use in @{get_index_name} for lookup best matching
+--- index.
+---


It required from me some time to get the return value structure.
I suppose it would be great if you draw small schematic image here. e.g.

transformation example i1: f1 f2 f3 f4 i2: f1 f2 f4 become f1--- f2 -- f3 -- f4 --> i1 `-- f4 --> i2

Khatskevich · 2018-03-06T11:26:07Z

graphql/accessor_general.lua

+--- @tparam table filter map of key-value to filter objects against
+---
+--- @treturn string `index_name` or `nil` is found index
+---


I want some information about its running time here.
I tried to calculate it and I suppose it is something like O(CNT(prefix tree nodes)*SIZE(filter)) in worst case which is good for most use cases.

Khatskevich · 2018-03-06T11:57:23Z

test/testdata/compound_index_testdata.lua

@@ -14,6 +14,11 @@ local function print_and_return(...)
    return table.concat({...}, ' ') .. '\n'
 end

+local function format_result(name, query, variables, result)


This feature makes review process much easier)
However to make the previous commit readable, could you please move this commit on top?)

Actually, I do not want you to waste your time. By now it is not important anymore.

Khatskevich

Main points are negotiated verbally.
I have made minor comments, while overall it looks good enough for me.

Khatskevich · 2018-03-07T11:36:08Z

test/testdata/compound_index_testdata.lua

+        local variables_1_3 = {user_str = 'user_str_b'}
+        local result = gql_query_1:execute(variables_1_3)
+        results = results .. print_and_return(
+            ('RESULT\n%s'):format(yaml.encode(result)))


Khatskevich · 2018-03-07T11:45:01Z

graphql/accessor_general.lua

+--- Future optimizations
+--- --------------------
+---
+--- * replace table.copy() with something more light (maybe 'closed set' of


We can delete item before recursive call and insert it back after

I suspect that it is not so easy, because of pairs() invalidation.

Khatskevich · 2018-03-07T11:55:06Z

graphql/accessor_general.lua

+--- (M * K)). Nodes count (M * K) can be limited upside as count of index
+--- parts in all indexes, so we have O(N^2 * COUNT(index parts)).
+---
+--- We can consider worst case scenario when any node has any of filter keys as


I do not think it is useful. But it took effort and I do not insist on it's deletion.

Different views on the complexity problem: dependency of filter size in context of fixed indexes or in context of worst-case indexes. Maybe it is something one should not consider.

Khatskevich · 2018-03-07T12:24:51Z

graphql/accessor_general.lua

+            local new_filter = table.copy(filter)
+            new_filter[k] = nil
+            local branch_index_name, branch_len =
+                get_best_matched_index(successor_node, new_filter)


Please, move + 1 here

Done. Still don’t see why it is better, but changed because of lack of any opinion about that.

Khatskevich · 2018-03-07T12:43:54Z

graphql/accessor_general.lua

+--- Complexity
+--- ----------
+---
+--- In short: O(N^2 * COUNT(index parts)).


Provide here information, that n= len(filter)

Khatskevich · 2018-03-07T12:46:31Z

graphql/accessor_general.lua

+---          \             \
+---           \             + --> abc
+---            \
+---             + ------> abc --> efg --> hij


Please draw also index names as leaf nodes

Leaf nodes are actually 'abc', 'hij' and so on, but I added index names.

The change is about code readability only, because we don't run `app = tarantool` default server from existing snapshots. No functional changes made.

The change is needed because we allow user to provide its own `get_index` function. Lack of index existence check can lead to an error that can be hard to debug.

The case described in [1]. The function proposed to including into shard in [2]. [1]: tarantool/shard#59 [2]: tarantool/shard#60

* Fixes #30. * Fixes #38.

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

Totktonada self-assigned this Mar 3, 2018

Totktonada requested review from Khatskevich and SudoBobo March 3, 2018 23:50

Totktonada force-pushed the index-lookup-by-partial-key branch from 26cefbc to 5ed3cea Compare March 4, 2018 16:28

Totktonada requested a review from Kasen March 5, 2018 16:11

Khatskevich reviewed Mar 5, 2018

View reviewed changes

Khatskevich reviewed Mar 6, 2018

View reviewed changes

Totktonada force-pushed the index-lookup-by-partial-key branch from 5ed3cea to 5550c3e Compare March 6, 2018 23:54

Khatskevich approved these changes Mar 7, 2018

View reviewed changes

Totktonada force-pushed the index-lookup-by-partial-key branch from 5550c3e to 9243e2c Compare March 7, 2018 19:00

Totktonada added 7 commits March 7, 2018 22:02

Update test-run to remove the drop_cluster hack

5735b62

testing: init shard module once per tarantool run

1e82d5f

The change is about code readability only, because we don't run `app = tarantool` default server from existing snapshots. No functional changes made.

Guard get_index func from receive index_name == nil

9d17719

The change is needed because we allow user to provide its own `get_index` function. Lack of index existence check can lead to an error that can be hard to debug.

Add compound index test to shard suites

bfece9e

Force schema reload in shard btw tests

2aa9e8b

The case described in [1]. The function proposed to including into shard in [2]. [1]: tarantool/shard#59 [2]: tarantool/shard#60

Support lookup by a partial set of index parts

acfad61

* Fixes #30. * Fixes #38.

Show query and variables in compound_index test

db859fa

Totktonada force-pushed the index-lookup-by-partial-key branch from 9243e2c to db859fa Compare March 7, 2018 19:10

Totktonada merged commit 096fead into master Mar 7, 2018

Totktonada deleted the index-lookup-by-partial-key branch March 7, 2018 19:13

		@@ -1,5 +1,7 @@
		#!/usr/bin/env tarantool

		shard_initialized = false

Index lookup by a partial key #65

Index lookup by a partial key #65

Uh oh!

Conversation

Totktonada commented Mar 3, 2018

Uh oh!

Khatskevich left a comment

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!

Totktonada Mar 6, 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!

Khatskevich Mar 6, 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!

Khatskevich Mar 6, 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!

Khatskevich left a comment

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!

Totktonada Mar 6, 2018 •

edited

Loading

Khatskevich Mar 6, 2018 •

edited

Loading

Khatskevich Mar 6, 2018 •

edited

Loading