Skip to content

Conversation

@muir
Copy link
Collaborator

@muir muir commented May 1, 2025

Also: optimize map creation

@muir muir requested review from Copilot and mitchwadair May 1, 2025 05:11
@codecov-commenter
Copy link

codecov-commenter commented May 1, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (beb0095) to head (1aa5d80).

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #11   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            3         3           
  Lines          172       187   +15     
=========================================
+ Hits           172       187   +15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a new function CopyMapSubset for creating a subset of a map and also optimizes map pre-allocation in existing functions.

  • Optimizes the ToSet and CopyMap functions by pre-allocating map capacity.
  • Adds extensive test coverage for the new CopyMapSubset function.
  • Introduces the CopyMapSubset function with pre-allocation improvements.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
set.go Pre-allocates map capacity in the ToSet function.
map_test.go Adds tests for the new CopyMapSubset function covering various scenarios.
map.go Pre-allocates slices/maps in multiple functions and adds CopyMapSubset; contains a redundant nil check in Merge.

@muir muir force-pushed the partialCopy branch 2 times, most recently from 2b5f1d3 to 9b3979a Compare May 1, 2025 05:22
@muir muir requested a review from Copilot May 1, 2025 05:22
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request introduces a new CopyMapSubset function for creating map subsets based on specified keys while also optimizing map creation in several helper functions. Key changes include:

  • Pre-allocating map capacity in ToSet and CopyMap for improved performance.
  • Adding CopyMapSubset and corresponding tests covering various edge cases.
  • Refining EqualKeys and Merge to improve clarity and safety in map operations.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
set.go Optimized map allocation in ToSet by pre-allocating capacity using len(slice).
map_test.go Added comprehensive tests for CopyMapSubset across multiple scenarios.
map.go Added CopyMapSubset, improved CopyMap with pre-allocation, refactored EqualKeys, and updated Merge to return a copy when input map a is nil.

@muir muir requested a review from Copilot May 1, 2025 05:27
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a new function, CopyMapSubset, to create a subset of a map using specified keys while also optimizing map creation by pre-allocating map capacities.

  • Introduces CopyMapSubset in map.go along with relevant documentation.
  • Adds comprehensive tests for CopyMapSubset in map_test.go.
  • Optimizes map pre-allocation in ToSet and CopyMap functions in set.go and map.go.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
set.go Optimizes ToSet by pre-allocating the map capacity.
map_test.go Adds tests for the new CopyMapSubset function.
map.go Adds CopyMapSubset and optimizes map pre-allocation in CopyMap and Merge.


// MissingKeys returns the keys that are in a but not b
func MissingKeys[K comparable, V any](a, b map[K]V) []K {
// Pre-allocate with capacity of a since that's the maximum possible size
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why so pessimistic? :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The comment was written by Claude 3.7 Sonnet. I think it's correct though.

Copy link
Collaborator

@dncohen dncohen May 1, 2025

Choose a reason for hiding this comment

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

Right, but if a has 100 keys and b has all of them, you're building a map with capacity 100 to store nothing.

@muir muir merged commit fd2d55e into main May 1, 2025
29 checks passed
@muir muir deleted the partialCopy branch May 1, 2025 15:43
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