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

[pdata] Map.Remove mutates value returned by Map.Get #5476

Open
dmitryax opened this issue Jun 5, 2022 · 5 comments
Open

[pdata] Map.Remove mutates value returned by Map.Get #5476

dmitryax opened this issue Jun 5, 2022 · 5 comments
Assignees
Labels
area:pdata pdata module related issues bug Something isn't working

Comments

@dmitryax
Copy link
Member

dmitryax commented Jun 5, 2022

Map.Delete doesn't work as a clean map interface and mutates values returned by Map.Get. The following snippet

	m := pcommon.NewMap()
	m.InsertString("k1", "v1")
	m.InsertString("k2", "v2")
	v1, _ := m.Get("k1")
	fmt.Println(v1.StringVal())
	m.Remove("k1")
	fmt.Println(v1.StringVal())

prints

v1
v2

instead of expected

v1
v1
@dmitryax dmitryax added the bug Something isn't working label Jun 5, 2022
@dmitryax
Copy link
Member Author

dmitryax commented Jun 5, 2022

@bogdandrutu I believe this is a side-affect of this change #2017 . Not sure if it warrants bringing back the following Map representation

type Map struct {
	orig *[]*otlpcommon.KeyValue
}

I would say we should. WDYT?

@dmitryax dmitryax changed the title [pdata] Map.Delete mutates values that referenced through Map.Get [pdata] Map.Delete mutates values returned by Map.Get Jun 5, 2022
@dmitryax dmitryax changed the title [pdata] Map.Delete mutates values returned by Map.Get [pdata] Map.Delete mutates value returned by Map.Get Jun 5, 2022
@bogdandrutu
Copy link
Member

This is true for other slices, and I think we specifically told (was there at one point at least if I remember) in the description that instances returned by At and Get will be invalid if Remove or even add is called.

@bogdandrutu
Copy link
Member

@dmitryax without that optimization we spend lots and lots of CPU allocating 1000s of small objects :))

@tigrannajaryan
Copy link
Member

BTW, this and other optimization won't be necessary and won't cause problems like this if we move to https://github.com/splunk/exp-lazyproto :-)

@dmitryax dmitryax self-assigned this Aug 23, 2022
@bogdandrutu bogdandrutu changed the title [pdata] Map.Delete mutates value returned by Map.Get [pdata] Map.Remove mutates value returned by Map.Get Sep 13, 2022
@dmitryax
Copy link
Member Author

dmitryax commented Nov 9, 2022

Removing from 1.0 milestone since fixing it is not a breaking change for the pdata API

@mx-psi mx-psi added the area:pdata pdata module related issues label Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:pdata pdata module related issues bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants