fix: pass a copy of RecordArray's internal fields in HL API

[Saransh-cpp]

Fixes #1648

The tests pass locally by running -

└─$ python3 -m pytest -vv -rs tests/v2/

[codecov]

Codecov Report

Merging #1650 (4696cbc) into main (9e17f29) will increase coverage by 0.46%.
The diff coverage is 68.67%.

Additional details and impacted files

Impacted Files	Coverage Δ
src/awkward/_v2/_connect/cuda/__init__.py	0.00% <0.00%> (ø)
src/awkward/_v2/_connect/numexpr.py	88.40% <0.00%> (ø)
src/awkward/_v2/_connect/pyarrow.py	88.46% <0.00%> (ø)
...awkward/_v2/_connect/rdataframe/from_rdataframe.py	0.00% <0.00%> (ø)
src/awkward/_v2/_lookup.py	98.68% <ø> (+1.17%)	⬆️
src/awkward/_v2/numba.py	93.47% <0.00%> (ø)
src/awkward/_v2/operations/ak_from_avro_file.py	66.66% <0.00%> (ø)
src/awkward/_v2/operations/ak_local_index.py	100.00% <ø> (ø)
src/awkward/_v2/operations/ak_max.py	87.09% <ø> (ø)
src/awkward/_v2/operations/ak_min.py	87.09% <ø> (ø)
... and 63 more

[jpivarski]

Perfect! This solves it.

[jpivarski]

@agoose77, I think we should leave fields as a list (following up on #1648 (comment)) and merge this, taking it as the first step in establishing a policy that high-level functions (ak.*, any public methods on ak.Array/ak.Record, including __getitem__, ufuncs) ensure that the data they return are not mutable references inside the Awkward Array objects. However, low-level interfaces (e.g. Content methods) do not make such a guarantee.

Do you agree?

[jpivarski]

Either way, @all-contributors please add @Saransh-cpp for code

[allcontributors]

@jpivarski

I've put up a pull request to add @Saransh-cpp! 🎉

[agoose77]

I'd suggest using .copy rather than copy.copy here, because we know that we have a list.

[Saransh-cpp]

Done!

[jpivarski]

Oh, I forgot that—I had been (wrongly) thinking that it could be a list or None, but no: leaf-nodes (NumpyForm and EmptyForm) return an empty list instead of None.

awkward/src/awkward/_v2/forms/numpyform.py

Lines 198 to 200 in 1a0858e

	@property
	def fields(self):
	return []

awkward/src/awkward/_v2/forms/emptyform.py

Lines 89 to 91 in 1a0858e

	@property
	def fields(self):
	return []

Well, the same is true of parameters: we know that we have a dict:

awkward/src/awkward/_v2/forms/form.py

Lines 259 to 263 in 1a0858e

	@property
	def parameters(self):
	if self._parameters is None:
	self._parameters = {}
	return self._parameters

And a dict also has a .copy(). I'll put that in.

[agoose77]

Taking a .copy() of a dict would be a shallow copy w.r.t the values. Is there any chance that we want a deep copy? Are there any uses of parameters that involve setting a mutable object e.g. dict, list etc.

[jpivarski]

(So would copy.copy: also shallow.)

In principle, parameters can be any JSON-able thing. In practice, they've always been strings. The way it looks like things are going, they'll always be strings. If we have some non-string lists and dicts, then in principle we'd have to protect them with deepcopy.

>>> parameters = {
...     "__record__": "SomeLongName",
...     "__categorical__": False,
...     "units": "cm",
...     "__doc__": "This is some thing. Wow."
... }
>>> def copy1(what, how_many):
...     for _ in range(how_many):
...         tmp = what.copy()
... 
>>> def copy2(what, how_many):
...     for _ in range(how_many):
...         tmp = copy.copy(what)
... 
>>> def copy3(what, how_many):
...     for _ in range(how_many):
...         tmp = copy.deepcopy(what)
... 
>>> def copy4(what, how_many):
...     for _ in range(how_many):
...         tmp = json.loads(json.dumps(what))
... 
>>> start = time.time(); copy1(parameters, 1000000); time.time() - start
0.05509829521179199
>>> start = time.time(); copy2(parameters, 1000000); time.time() - start
0.18173909187316895
>>> start = time.time(); copy3(parameters, 1000000); time.time() - start
2.2748003005981445
>>> start = time.time(); copy4(parameters, 1000000); time.time() - start
3.071228504180908

I don't think the unlikely possibility of weird types justifies the expense. Or wait, maybe

>>> def copy5(what, how_many):
...     for _ in range(how_many):
...         if all(isinstance(x, (str, bool, numbers.Integral, numbers.Real)) for x in what.values()):
...             tmp = what.copy()
...         else:
...             tmp = copy.deepcopy(what)
... 
>>> start = time.time(); copy5(parameters, 1000000); time.time() - start
0.5682976245880127

is only a factor-of-10 cost, which goes into the expensive branch on weird cases (none of ours).

[agoose77]

Oops, I misread this as copy.deepcopy.

[jpivarski]

(Working through the web editor...)