Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[main < T] Bugfix for map.merge #377

Merged
merged 42 commits into from
Oct 24, 2023
Merged

[main < T] Bugfix for map.merge #377

merged 42 commits into from
Oct 24, 2023

Conversation

mpintaric55334
Copy link
Contributor

Description

Please briefly explain the changes you made here.

User noticed a bug, that when the second map contains null elements, merge will throw errors. This happened because we checked the existence of keys using At. If we check it like that, if the key exists, but contains null value, it will be as if the key doesnt exist. Instead I made a set of keys, and checked the existence of keys taht way. @antoniofilipovic maybe it would make sense to add some Keys() function to map Cpp API to automatize the checking for key existence.

Pull request type

  • Bugfix
  • Algorithm/Module
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

Related issues

Delete if this PR doesn't resolve any issues. Link the issue if it does.

######################################

Reviewer checklist (the reviewer checks this part)

Module/Algorithm

######################################

idoraban and others added 30 commits August 2, 2023 09:09
[main < T585 T586] Implement date functions
@mpintaric55334 mpintaric55334 added type: bug Something isn't working status: ready PR is ready for review Docs unnecessary Docs unnecessary labels Oct 6, 2023
@mpintaric55334 mpintaric55334 self-assigned this Oct 6, 2023
@antoniofilipovic
Copy link
Collaborator

@mpintaric55334 it makes sense to add key exists function to C++ API, I agree. Currently seems it is not possible to check if key exists but property value is null

@mpintaric55334
Copy link
Contributor Author

This PR memgraph/memgraph#1336 in memgraph needs to be merged for this to work, we've added new functionality (KeyExists) in Cpp API

Copy link
Collaborator

@antoniofilipovic antoniofilipovic left a comment

Choose a reason for hiding this comment

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

Looks good to me

@antoniofilipovic antoniofilipovic added status: ship it PR approved and removed status: ready PR is ready for review labels Oct 16, 2023
@antoniofilipovic antoniofilipovic added this to the 1.12.0 milestone Oct 17, 2023
@antoniofilipovic antoniofilipovic merged commit 84c20f5 into main Oct 24, 2023
2 checks passed
@antoniofilipovic antoniofilipovic deleted the T-map-merge-fix branch October 24, 2023 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Docs unnecessary Docs unnecessary Needs new memgraph status: ship it PR approved type: bug Something isn't working
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants