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.

Sign up for GitHub

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

Jump to bottom

Deep sort hashes #52

Closed
wants to merge 2 commits into from
Closed

Deep sort hashes #52

wants to merge 2 commits into from

Conversation

tomas321
Copy link

@tomas321 tomas321 commented Apr 16, 2019 •
edited
Loading

Added new dependency 'deepsort' gem from 'https://github.com/mcrossen/deepsort'.
This PR is a prerequisite for #50. If it's merged, I will rebase #50 to master and make the necessary changes.

@Teebor-Choka
Copy link

The code looks good. Any ETA on this being merged? This functionality is needed for multiple other PRs.

@ypid-geberit
Copy link

ypid-geberit commented Jun 25, 2020 •
edited
Loading

Hi @tomas321, like your other PR #50, this one also looks like a duplicate. There is already ongoing discussion about this in #41. Nothing wrong with alternative implementations, but you should at least reference them and declare why you chose to do it differently.

@tomas321
Copy link
Author

Hi @ypid-geberit, sorry for for the confusion. I needed this feature with combination of #50 and this change and even though #41 fixes the deep sort problem, I tried to be the least invasive to the current code. Although your solution is independent of external libraries, which is better in the long term, it presents possible duplicate to the tested and existent library mcrossen/deepsort.

To compare, both solutions deepsorts the event objects, #41 still proposes breaking changes with the serialize function, #52 has no breaking changes and backward compatibility is ensured as in #41 with the deep_sort variable defaulting to False.

@ypid-geberit
Copy link

ypid-geberit commented Jun 26, 2020 •
edited
Loading

Thanks very much! I guess when external dependencies are ok, your solution is cleaner, has no breaking changes by default (which was also my intention with #41, which was still waiting on further info) and should thus be preferred.

@ypid-geberit ypid-geberit mentioned this pull request Jun 26, 2020
Closed
@kares
Copy link
Contributor

kares commented Jul 2, 2020

Thanks for the PR, I've looked into deepsort gem and I am not sure I like bringing it in as a dependency.
First of its monkey-patching core and has way more functionality than we need here (one deep_sort method).
Even if that concern is negligible there's "weird" stuff such as the re-defined map ordering.
This does not seem very fortunate, I think the plugin should have an explicit path on what we're doing for non-comparables.

So before doing a review here it would be worth re-thinking the "deep-sort" method in house.

@jsvd jsvd mentioned this pull request Aug 27, 2020
Merged
jsvd added a commit that referenced this pull request Aug 27, 2020
@jsvd
deeply sort hashes to ensure consistent fingerprints (#55)
7292935 
Fingerprinting is done by either fetching the .to_hash representation
or by fetching fields with .get(field)

The content of these often contain hashes, which we don't guarantee
order during insertion, so this commit recursively sorts hashes.

This change affects all modes:
* concatenate_all_sources => true
* concatenate_fields => true
* normal single field fingerprint (that may contain a nested Hash)

Fixes #39
Replaces #41, #52
@ypid-geberit
Copy link

Nice to see this bug finally got fixed by #55. This alternative PR can probably be closed.

@roaksoax
Copy link

Closing this PR as per the above comment.

@roaksoax roaksoax closed this Aug 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tomas321 @Teebor-Choka @ypid-geberit @kares @roaksoax