Skip to content

Conversation

@mRrvz
Copy link
Contributor

@mRrvz mRrvz commented Feb 20, 2021

Fixed not fining field in tuple on crud.update if there are is_nullable fields in front of it that were added when the schema was changed. Closes #113

Copy link
Contributor

@olegrok olegrok left a comment

Choose a reason for hiding this comment

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

Thanks for you patch consider several comments below.

I've put quite a lot comments, so I need to clarify some expectations from this patch.

  • Updates should be optimistic - we shouldn't get tuple before we understand that some field is missing: error has code box.error.NO_SUCH_FIELD_NO or box.error.NO_SUCH_FIELD_NAME
  • This logic should be performed on storage-side
  • We should try to do it only for top-level fields (don't think about jsonpath updates)
  • Tarantool allows to add fields after fields that declared in space format, but I think it's not general purpose case IMO - so we need to do something only if field name is in space format (but for some reasons some fields is missed before this field) or field number <= space format length.

Copy link
Contributor

@olegrok olegrok left a comment

Choose a reason for hiding this comment

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

Thanks for your fixes. Consider a couple of comments below.

@olegrok olegrok self-requested a review February 24, 2021 11:40
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.

Please, consider my comments and update a Changelog.

@mRrvz mRrvz changed the title Update add nullable fields Update function now adds intermediate nullable fields (if they needed) Feb 24, 2021
Copy link
Contributor

@olegrok olegrok left a comment

Choose a reason for hiding this comment

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

Thanks for your changes and fixes. Sorry for some controversial comments before.

Currently I basically agree with your patch and with your approach. I put some comments about some details. Please ask @dokshina for the second review.

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.

Nice work!

@mRrvz mRrvz force-pushed the update-nullable-fields-bugfix branch from cb996ba to cde88ed Compare February 26, 2021 09:36
@mRrvz mRrvz merged commit 6c30483 into master Feb 26, 2021
@mRrvz mRrvz deleted the update-nullable-fields-bugfix branch February 26, 2021 09:41
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.

[2pt] Add intermediate nullable fields on update if not present in tuple

4 participants