Kh/to avro #58

Khatskevich · 2018-02-28T14:58:30Z

No description provided.

Totktonada

Questions and comments for the code are attached. Answers for such questions can be good basis for proper documentation and code comments.

Please, give attention to my expectation to have backlog trackerized once the feature will be implemented in the current state (see related comment).

Totktonada · 2018-03-04T03:00:24Z

test/common/lua/test_data_user_order.lua

@@ -41,23 +84,74 @@ testdata.meta = {
        "order_collection": {
            "schema_name": "order",
            "connections": [
+                {
+                    "type": "1:N",
+                    "name": "order__order_item",


Having schema/collection name as part of connection makes understanding of a query harder.

Totktonada · 2018-03-04T19:12:58Z

test/common/lua/test_data_user_order.lua

+--    - id
+--    - first_name
+--    - last_name
+--  - order: Orders of users.


I propose to clarify (comment here) that order is a shopping cart to better understanding the example.

Totktonada · 2018-03-04T19:20:10Z

test/common/lua/test_data_user_order.lua

+    local item_id_max = #items
+    for _, item in ipairs(items) do
+       virtbox.item_collection:replace(
+               {item.id, item.description, item.name, item.price}


Broken indent.

Totktonada · 2018-03-04T19:31:34Z

test/common/lua/test_data_user_order.lua

+                order_item_cnt = order_item_cnt + 1
+                local item_id = order_item_cnt % item_id_max + 1
+                virtbox.order_item_collection:replace(
+                        {


Broken indent.

Totktonada · 2018-03-04T20:43:43Z

graphql/query_to_avro.lua

+        return result
+    end
+    error('Unknown type "' .. fieldTypeName .. '" for field "' ..
+            field.name .. '"')


luacheck output:

graphql/query_to_avro.lua:62:13: accessing undefined variable field

Totktonada · 2018-03-04T21:49:10Z

graphql/query_to_avro.lua

+    end
+
+    if fieldTypeName == 'Scalar' or fieldTypeName == 'Enum' then
+        return fieldType.serialize(result)


Can you describe what is going on here? We have no any value to serialize… Also it is unexpected to see scalars here, as you process it separatelly.

Totktonada · 2018-03-04T21:50:21Z

graphql/query_to_avro.lua

+        return result
+        --return collection_to_avro(fieldType, result, subSelections, context)
+    elseif fieldTypeName == 'Interface' or fieldTypeName == 'Union' then
+        local objectType = fieldType.resolveType(result)


We trying to resolve union to a certain type based on what information? We have no values here to choose one of variants. Can you describe it, please?

My mistake. It should not work properly, but I did not know what to do with unions. 1. They are not implemented. 2. They are represented in different ways in gql and tarantool. A suttose at this point we should assert here.

Totktonada · 2018-03-04T21:50:36Z

graphql/query_to_avro.lua

+    elseif fieldTypeName == 'Interface' or fieldTypeName == 'Union' then
+        local objectType = fieldType.resolveType(result)
+        result.type = collection_to_avro(objectType, result,
+                                         subSelections, context)


Python-style indent.

Totktonada · 2018-03-04T21:52:18Z

graphql/query_to_avro.lua

+    return result
+end
+
+collection_to_avro = function(objectType, object, selections, context)


Maybe 'object_to_avro' is better name? I think about a case with inner record within a collection object or with an object within an array.

Totktonada · 2018-03-04T22:07:13Z

graphql/query_to_avro.lua

+        return result
+    end
+
+    if fieldTypeName == 'Scalar' or fieldTypeName == 'Enum' then


I don’t know how to receive 'scalar' from our avro-schemas. Maybe we should ban it?

We have no enum / union support for now (and maybe interfaces too). I think we need to test it or stated explicitly that it need to be tested once will be available. So, I expect that you will file bunch of issues to support various types in AST or at least add notes re query schemas to existing issues for supporting of such features.

(is not for offend) If you don’t sure current implementation will work properly, maybe it is better to remove it for now to keep code compact and readable?

Totktonada · 2018-03-04T23:13:44Z

Added @SudoBobo as optional reviewer.

Totktonada

I think it is good enough (there was worse code from me). Despite that I leave some comments to grow our feeling of a 'healthy code'.

Totktonada · 2018-03-09T23:07:25Z

graphql/query_to_avro.lua

+---
+--- @treturn table `avro_schema` avro schema for any `query:execute()` result.
+function query_to_avro.convert(query)
+    local state = query.state


Optional: it is good to catch compiled_query.avro_schema() (period instead of colon) calls and produce a meaningfull message in the case. Just assert that the query parameter is a table.

Totktonada · 2018-03-09T23:21:28Z

graphql/query_to_avro.lua

+-- The function converts avro type to nullable.
+-- In current tarantool/avro-schema implementation we simply add '*'
+-- to the end of type name.
+-- The function do not copy the resulting type but changes it in place.


It changes and returns changed object both. Personally I prefer to implement one of that approaches to decrease possible area to misuse a function (one can expect that it does not change the parameter, because it returns the new result).

@Khatskevich You just ignored the comment. So I don’t know your point and I have mine preferences. I’ll fix it according to what I know — mine preferences.

I prefer here to leave it both return and change an input in place, because of immutability of strings in Lua.

Totktonada · 2018-03-09T23:24:06Z

graphql/query_to_avro.lua

+-- @tparam table avro schema node to be converted to nullable
+--
+-- @tresult table schema node; basically it is the passed schema node,
+-- however in nullable type implementation through unions it can be different


Don't get you in two ways:

As I see we always return the same table (but with change field).

What is nullable type implementation through unions? The above statement says that nulable type is implemented via '*'. Maybe more context needed here.

@Khatskevich You just ignored the comment. I’ll fix it according to mine preferences.

Removed confusing part about an union.

Totktonada · 2018-03-09T23:25:41Z

graphql/query_to_avro.lua

+        avro.type = make_avro_type_nullable(avro.type)
+        return avro
+    end
+    assert(false, "Avro type should be a string or table")


It is good to have "got " .. type_type at the end (saves some time at debug).

Totktonada · 2018-03-09T23:32:41Z

graphql/query_to_avro.lua

+    return result
+end
+
+-- The function converts avro type to nullable.


The function cannot be applied to its result, yep? We need to check wheter passed type is nullable or stated non-self-applicability explicitly in the comment.

Assert on nullable types is worse option then self-applicability, IMO, but ok, you fixed it.

Reworked to allow nulllable type as an input.

Totktonada · 2018-03-10T08:41:44Z

graphql/query_to_avro.lua

+        local innerType = fieldType.ofType
+        -- Steal type from virtual object.
+        -- This is necessary because in case of arrays type should be
+        -- "completed" into results `items` field, but in case of 1:1 relations


Better to oppose an array (List) vs a record (Object) and a scalar (Scalar). That is exactly what this part of the code doing. When you say about 1:1 connection it at least yield the question what is going with nested/top-level records and scalars.

Totktonada · 2018-03-10T10:05:19Z

graphql/query_to_avro.lua

+--- root to the current object
+---
+--- @treturn table corresponding Avro schema
+object_to_avro = function(object_type, selections, context, nullable)


Unused and undocumented parameter nullable.

We have --no-unused-args in luacheck, because it is too restricting in some cases. Say, when you don’t use self parameter of a function.

Totktonada · 2018-03-10T10:09:48Z

graphql/query_to_avro.lua

+    context.namespace_parts = {}
+    local rootType = state.schema[context.operation.operation]
+    local selections = context.operation.selectionSet.selections
+    return object_to_avro(rootType, selections, context, false)


If you’ll remove nullable parameter, don’t miss to remove it here.

Totktonada · 2018-03-10T10:14:19Z

graphql/query_to_avro.lua

+        NonNull)
+    local fieldTypeName = fieldType.__type
+    if fieldTypeName == 'NonNull' then
+        -- In case the field is NonNull, the real type is in ofType attibute.


attibute → attribute

Totktonada · 2018-03-10T10:53:20Z

graphql/query_to_avro.lua

+local function complete_field_to_avro(fieldType, result, subSelections, context,
+        NonNull)
+    local fieldTypeName = fieldType.__type
+    if fieldTypeName == 'NonNull' then


This if is… hmm… hard to read. Let’s look over with me:

The fieldTypeName == 'NonNull' branch.

Here we see renaming that works like recursion, but expressed via fall-through.

By the way, it cannot process several nested NonNull’s.

NonNull ~= true branch.

Never use x ~= true. Your reader does not want to normalize the code in the brain.

Here we use recursion and don’t use fall-through.

How to make the code readable? Options I see:

Save is_nonnull or is_nullable flag. Process fieldType/fieldType.ofType and save the result. Make it nullable in dependence of the flag. Return the result.

Save is_nonnull or is_nullable flag. In case of NonNull resursively call the function, but pass the flag in a parameter. Process result. Make it nullable in dependence of the flag. Return the result.

I appeal you to leave it as is if you comfortable with your approach, but I think that it is good to share argued opinions between us.

@Khatskevich You just ignored the comment. I’ll fix it according to mine preferences.

Reworked according to the first option. Make it returning a result using return instead of the mutable table parameter.

The changes are debatable, but I'm trying to improve code readability as stated in the comments to [1]. [1]: tarantool/graphql#58

Add goods (item) and order_item relation.

This patch is important for graphql query -> avro schema convertor implementation, because both executor and convertor walks over query AST. Part of #7

Extend query object: add `avro_schema` method, which produces Avro schema which can be used to verify or flatten any `query_exequte` result. Closes #7

The changes are debatable, but I'm trying to improve code readability as stated in the comments to [1]. [1]: tarantool/graphql#58

This follows up the PR #58 (the work in the scope of the issue #7).

The changes are debatable, but I'm trying to improve code readability as stated in the comments to [1]. [1]: tarantool/graphql#58

This follows up the PR #58 (the work in the scope of the issue #7).

Khatskevich requested a review from Totktonada February 28, 2018 14:58

Khatskevich force-pushed the kh/to_avro branch from 97a22b8 to bd7a2a2 Compare March 2, 2018 13:33

Totktonada reviewed Mar 4, 2018

View reviewed changes

Totktonada requested a review from SudoBobo March 4, 2018 23:13

Khatskevich force-pushed the kh/to_avro branch 5 times, most recently from fe69468 to 49de748 Compare March 5, 2018 15:47

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

Khatskevich force-pushed the kh/to_avro branch from 49de748 to 1fc559b Compare March 6, 2018 21:06

Totktonada approved these changes Mar 10, 2018

View reviewed changes

Khatskevich force-pushed the kh/to_avro branch from 1fc559b to 4764e2b Compare March 12, 2018 23:25

Totktonada self-assigned this Mar 13, 2018

Totktonada force-pushed the kh/to_avro branch from 4764e2b to 184a951 Compare March 13, 2018 21:05

Totktonada referenced this pull request Mar 13, 2018

Refactor query_to_avro a bit

184a951

The changes are debatable, but I'm trying to improve code readability as stated in the comments to [1]. [1]: tarantool/graphql#58

Khatskevich and others added 4 commits March 14, 2018 00:10

Improve test_data_user_order

007bc25

Add goods (item) and order_item relation.

Move general parts of executor to query_util.lua

7f6f892

This patch is important for graphql query -> avro schema convertor implementation, because both executor and convertor walks over query AST. Part of #7

Feature: convert graphql query to avro schema

0aaf2b1

Extend query object: add `avro_schema` method, which produces Avro schema which can be used to verify or flatten any `query_exequte` result. Closes #7

Refactor query_to_avro a bit

fcc7007

The changes are debatable, but I'm trying to improve code readability as stated in the comments to [1]. [1]: tarantool/graphql#58

Totktonada force-pushed the kh/to_avro branch from 184a951 to fcc7007 Compare March 13, 2018 21:12

Totktonada merged commit 831ead8 into master Mar 13, 2018

Totktonada deleted the kh/to_avro branch March 13, 2018 21:13

Totktonada added a commit that referenced this pull request Mar 13, 2018

Check the corner case in an avro-schema generation

960cc49

This follows up the PR #58 (the work in the scope of the issue #7).

Totktonada mentioned this pull request Mar 13, 2018

Check the corner case in an avro-schema generation #77

Merged

Totktonada added a commit that referenced this pull request Mar 13, 2018

Check the corner case in an avro-schema generation

68dd4ae

This follows up the PR #58 (the work in the scope of the issue #7).

SudoBobo referenced this pull request Mar 14, 2018

Refactor query_to_avro a bit

c2dd319

The changes are debatable, but I'm trying to improve code readability as stated in the comments to [1]. [1]: tarantool/graphql#58

SudoBobo pushed a commit that referenced this pull request Mar 14, 2018

Check the corner case in an avro-schema generation

b73c0e1

This follows up the PR #58 (the work in the scope of the issue #7).

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kh/to avro #58

Kh/to avro #58

Khatskevich commented Feb 28, 2018

Totktonada left a comment

Totktonada Mar 4, 2018

Totktonada Mar 4, 2018

Totktonada Mar 4, 2018

Totktonada Mar 4, 2018

Totktonada Mar 4, 2018

Totktonada Mar 4, 2018

Totktonada Mar 4, 2018

Khatskevich Mar 5, 2018

Totktonada Mar 4, 2018

Totktonada Mar 4, 2018

Totktonada Mar 4, 2018

Totktonada commented Mar 4, 2018

Totktonada left a comment

Totktonada Mar 9, 2018

Totktonada Mar 9, 2018

Totktonada Mar 13, 2018 •

edited

Loading

Totktonada Mar 13, 2018

Totktonada Mar 9, 2018

Totktonada Mar 13, 2018

Totktonada Mar 13, 2018

Totktonada Mar 9, 2018

Totktonada Mar 9, 2018

Totktonada Mar 13, 2018

Totktonada Mar 13, 2018

Totktonada Mar 10, 2018

Totktonada Mar 10, 2018

Totktonada Mar 10, 2018

Totktonada Mar 10, 2018

Totktonada Mar 10, 2018

Totktonada Mar 10, 2018

Totktonada Mar 13, 2018

Totktonada Mar 13, 2018

Kh/to avro #58

Kh/to avro #58

Conversation

Khatskevich commented Feb 28, 2018

Totktonada 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

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

Totktonada commented Mar 4, 2018

Totktonada left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Totktonada Mar 13, 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

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

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

Totktonada Mar 13, 2018 •

edited

Loading