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

[1/3][Asset Selection] Remove key_substring + support wildcards in key: #27720

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

salazarm
Copy link
Contributor

@salazarm salazarm commented Feb 10, 2025

Summary & Motivation

  1. Remove key_substring
  2. Support wildcards in key, eg: key:_prefix_*_middlefix_*_suffix_

How I Tested These Changes

pytest

Comment on lines +1188 to +1189
class KeyWildCardAssetSelection(AssetSelection):
selected_key_wildcard: str
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really happy with the naming here 🙃

@salazarm salazarm changed the title [Asset Selection] Remove key_substring + support wild cards in key: [Asset Selection] Remove key_substring + support wildcards in key: Feb 10, 2025
@salazarm salazarm changed the title [Asset Selection] Remove key_substring + support wildcards in key: [1/2][Asset Selection] Remove key_substring + support wildcards in key: Feb 10, 2025
@salazarm salazarm changed the title [1/2][Asset Selection] Remove key_substring + support wildcards in key: [1/3][Asset Selection] Remove key_substring + support wildcards in key: Feb 10, 2025
@schrockn
Copy link
Member

Similar concerns here about about what we are promising going forward in terms of indexes and searchability. I just want to come into this eyes wide open. cc: @alangenfeld @prha

value = self.visit(ctx.value())
return AssetSelection.key_substring(value)
value = self.visit(ctx.keyValue())
return KeyWildCardAssetSelection(selected_key_wildcard=value)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One question for reviewers, should we attach this to the AssetSelection class (eg. AssetSelection.assetsByWildCardString(value))?

@salazarm salazarm requested review from alangenfeld and removed request for smackesey February 11, 2025 20:56
@alangenfeld
Copy link
Member

Trying to do it against the db directly has to confront that the keys are stored as a json encoded list of string components, so its a more complex translation than just switching * for like %.

Any search backend that has documents for each asset should easily handle it.

The test each thing in memory approach can be done over an iterator thats paginating if we want to avoid having them all in memory at once with I would guess marginal latency trade-off.

I don't think we can guarantee its fast in all deployment setups but it should be doable. Nothing meaningfully worse than this initial implementation of regex testing all of them in memory.

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.

3 participants