Skip to content

Conversation

@savolgin
Copy link
Contributor

Issue #7

@olegrok
Copy link
Contributor

olegrok commented Sep 24, 2020

Thanks for your patch, please drop the first commit (Support tuples as result type) from current pull request, it's not related to "Support replace and upsert operations"

@dokshina
Copy link
Contributor

Thanks a lot!

Please, alseo describe new operations in README.

@savolgin
Copy link
Contributor Author

Please, alseo describe new operations in README.

redme updated

Thanks for your patch, please drop the first commit (Support tuples as result type) from current pull request, it's not related to "Support replace and upsert operations"

dropped

@olegrok
Copy link
Contributor

olegrok commented Sep 24, 2020

I suggest you to add tests for both engines - memtx and vinyl.

See example - #4

@olegrok
Copy link
Contributor

olegrok commented Sep 24, 2020

See my last comment. After fixing it - LGTM

@olegrok
Copy link
Contributor

olegrok commented Sep 24, 2020

LGTM. Seems you need to update changelog as well

@savolgin
Copy link
Contributor Author

Change log updated

end
end

if bucket_id ~= nil and field_format.name == 'bucket_id' then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a nit: since we already have system_fields conception, maybe it can be

function utils.flatten(object, space_format, system_values)
    system_values = system_values or {}
    ...
    if system_values.bucket_id ~= nil and field_format.name == 'bucket_id' then ...
    -- if sometimes we have more system fields, it can be done in a cycle
    ...
end

Copy link
Contributor

@dokshina dokshina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for your patch!

Only one: please wait for new CI based on GitHub Actions is merged and rebase on it.
Then, it will be merged.

@dokshina dokshina merged commit d281e1d into tarantool:master Sep 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants