Avoid exponential call to rewrite dataset names when creating _FrozenDatasets

[limdauto]

Description

Fixes #951

Development notes

Checklist

• Read the contributing guidelines
• Opened this PR as a 'Draft Pull Request' if it is work-in-progress
• Updated the documentation to reflect the code changes
• Added a description of this change and added my name to the list of supporting contributions in the RELEASE.md file
• Added tests to cover my changes

Notice

• I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

• I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.

• I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorised to submit this contribution on behalf of the original creator(s) or their licensees.

• I certify that the use of this contribution as authorised by the Apache 2.0 license does not violate the intellectual property rights of anyone else.

[deepyaman]

Is this the right place to optimize? Should add_all and add_feed_dict even construct a new FrozenDatasets on each add?

add encapsulates some logic around checking whether or not dataset exists in catalog, which could be encapsulated into some _exists_in_catalog. Then, the add/update is pretty straightforward, and add_all/add_feed_dict can just update a plain dictionary as opposed to constructing FrozenDatasets each time.

[limdauto]

@deepyaman I think the main motivation (and I'm guessing) was that FrozenDatasets is completely immutable, so not sure how this would look in practice add_all/add_feed_dict can just update a plain dictionary as opposed to constructing FrozenDatasets each time.

That said, I think you are right in the sense that the approach proposed here is not the right one. I'm trying another one.

[deepyaman]

@deepyaman I think the main motivation (and I'm guessing) was that FrozenDatasets is completely immutable, so not sure how this would look in practice add_all/add_feed_dict can just update a plain dictionary as opposed to constructing FrozenDatasets each time.

I don't see this as an issue. From a user perspective, you should not be able to update FrozenDatasets, sure; from a framework perspective, if you can merge the dictionaries more smartly before constructing the FrozenDatasets that the user interacts with (under the hood), that's fine. Framework code exists to do this kind of optimization. :)

[limdauto]

@deepyaman

from a framework perspective, if you can merge the dictionaries more smartly before constructing the FrozenDatasets that the user interacts with (under the hood), that's fine

Yep, going to push something along this line. Thank you

[limdauto]

@deepyaman I have pushed a fix which avoid any extra processing when creating _FrozenDatasets from another _FrozenDatasets. The extra processing of key name only happens now for newly added datasets.

Did some benchmark for call counts to the _sub_nonword_chars method. Same catalog.

Before:

After

[deepyaman]

LGTM

[deepyaman]

FYI this isn't really necessary AFAIK, as the compiled regex gets cached anyway, and you don't have a ton of regexes here (see https://docs.python.org/3/library/re.html#re.compile)

cc @datajoely just as an FYI

[datajoely]

Ah interesting - didn't know that, I still think this is nice for readability

[lorenabalan]

Brilliant!! Thank you so much for this. 🙏

[merelcht]

Nice one! 👏