Repository navigation
Honor remove_duplicates when searching nested data - #299
codewithfourtix wants to merge 2 commits into
Conversation
Signed-off-by: Ali Zulfiqar <[email protected]>
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Scalar matches still fail on the default remove_duplicates=True path. After this change the recursive calls correctly defer deduplication, but the outer call still passes the combined result to remove_duplicate_dicts(), which assumes every value is a dict and calls .items(). For example, find_data_in_nested_structure({'event': 'Alice'}, 'event') still raises AttributeError. Since the return type is Sequence[Any] and this PR explicitly adds scalar handling, could deduplication support non-dict/hashable values as well, with the scalar regression exercised for the default path?
remove_duplicate_dicts() assumes every value is a dict and calls .items(),
so the default remove_duplicates=True path still raised AttributeError for
scalar matches, e.g. find_data_in_nested_structure({'event': 'Alice'}, 'event').
Add remove_duplicate_values(), which builds a hashable key for any value
(dicts, lists, sets, scalars, and dicts containing unhashable values) and
use it for the top-level deduplication. remove_duplicate_dicts() is left
unchanged for existing callers.
Adds tests for scalar matches on the default path, duplicate scalars,
mixed value types, and dicts with unhashable values.
|
@sylvesterkaczmarek Test examples is failing on main itself since e071b30 (Social Deception example) , same failure on 3e30a20. This PR doesn't touch anything the examples import (find_data_in_nested_structure has no callers in examples/). Happy to rebase once main is green. |
|
Rechecked 9b654d2. The default deduplication path now accepts scalar and mixed nested values instead of routing everything through dict-only .items() handling. The exact top-level scalar case I raised is covered, along with duplicate scalars and mixed/unhashable container values. My concern is resolved. |
-- 8f21e61 by Ali Zulfiqar <[email protected]>: Honor remove_duplicates when searching nested data Signed-off-by: Ali Zulfiqar <[email protected]> -- 9b654d2 by Ali Zulfiqar <[email protected]>: Deduplicate non-dict values in find_data_in_nested_structure remove_duplicate_dicts() assumes every value is a dict and calls .items(), so the default remove_duplicates=True path still raised AttributeError for scalar matches, e.g. find_data_in_nested_structure({'event': 'Alice'}, 'event'). Add remove_duplicate_values(), which builds a hashable key for any value (dicts, lists, sets, scalars, and dicts containing unhashable values) and use it for the top-level deduplication. remove_duplicate_dicts() is left unchanged for existing callers. Adds tests for scalar matches on the default path, duplicate scalars, mixed value types, and dicts with unhashable values. COPYBARA_INTEGRATE_REVIEW=#299 from codewithfourtix:fix-nested-data-duplicate-option 9b654d2 PiperOrigin-RevId: 986701079 Change-Id: Id9d19cfd4409b76cab0654bf970f04902405a362
|
Thanks for this contribution, @codewithfourtix! This change has already been merged — it was imported into Google's internal monorepo and submitted on 2026-09-23, then synced back out to Our import tooling normally closes the PR automatically once the change lands, but that step silently failed here, so this PR was left open even though your code shipped. We've filed b/565369855 against the import tooling to get this fixed properly. Closing it now — apologies for the confusion, and thank you for the fix! |
|
Thanks for the clarification and for getting the fix landed internally. Glad the nested/scalar deduplication case made it into main. |
Superseded by remove_duplicate_values + _make_hashable from the google-deepmind#299 rework. find_data_in_nested_structure no longer calls it, no other callers remain, and its frozenset(d.items()) implementation raises AttributeError on the non-dict values the new path supports.
-- 8add909 by ajitashwath <[email protected]>: Remove dead legacy remove_duplicate_dicts helper Superseded by remove_duplicate_values + _make_hashable from the #299 rework. find_data_in_nested_structure no longer calls it, no other callers remain, and its frozenset(d.items()) implementation raises AttributeError on the non-dict values the new path supports. COPYBARA_INTEGRATE_REVIEW=#398 from ajitashwath:remove-legacy-dedup-helper 8add909 PiperOrigin-RevId: 998621490 Change-Id: Iaa02e4ba1cac84bd77c4c983d2712c928e0c95d0
Recursive calls reset
remove_duplicatesto its default, so passingFalsestill removes nested matches and can crash on scalar values. Collect matches without deduplicating during recursion, then apply the requested behavior once at the top.Adds tests for nested duplicates, scalar values, and the default deduplication behavior.