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

Cosmos: Fixup to Regex.IsMatch translation #28139

Closed
AndriySvyryd opened this issue May 31, 2022 · 15 comments · Fixed by #28261
Closed

Cosmos: Fixup to Regex.IsMatch translation #28139

AndriySvyryd opened this issue May 31, 2022 · 15 comments · Fixed by #28261
Assignees
Labels
area-cosmos area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. good first issue This issue should be relatively straightforward to fix. type-enhancement
Milestone

Comments

@AndriySvyryd
Copy link
Member

AndriySvyryd commented May 31, 2022

Fixup to #28121

@AndriySvyryd AndriySvyryd added type-enhancement good first issue This issue should be relatively straightforward to fix. area-query area-cosmos labels May 31, 2022
@roji
Copy link
Member

roji commented May 31, 2022

Depend on #26410, right?

@roji roji added the blocked label May 31, 2022
@smitpatel
Copy link
Contributor

Not really blocked, the method translator should throw exception from itself.

@roji
Copy link
Member

roji commented May 31, 2022

Don't we avoid doing that since it would block client eval?

@smitpatel
Copy link
Contributor

Depends if we want to do client eval of certain things.

@roji
Copy link
Member

roji commented Jun 1, 2022

OK, I'll bring this up in design, possibly with a proposal for #26410.

@smitpatel
Copy link
Contributor

If you think we would need #26410 for this, we can wait till we put that one in plan.

@roji
Copy link
Member

roji commented Jun 1, 2022

Just to figure out how we see things... If we're OK with throwing in method translators, #26410 becomes much less important, maybe even closeable (note it's already in the milestone).

I still think that ideally we just do #26410, e.g. by having translator return some NotTranslatableExpression (which wraps details); and then we don't have to discuss throwing because that always allows client eval and provides nicer exceptions. But we can discuss all that.

@roji roji changed the title Cosmos: Throw a detailed exception for unsupported options when translating Regex.IsMatch Cosmos: Fixup to Regex.IsMatch translation Jun 2, 2022
@Marusyk
Copy link
Member

Marusyk commented Jun 2, 2022

I'll prepare a PR for this

@roji
Copy link
Member

roji commented Jun 2, 2022

@Marusyk sure, but we'll need to decide what we want to do with unsupported options (design discussion pending).

@roji
Copy link
Member

roji commented Jun 5, 2022

Design decision: we'll do #26410 at some point, but for now it should be fine to just return null for unsupported options, so the user will just get the generic "cannot translate" exception. @Marusyk this should unblock you for working on this issue.

@Marusyk
Copy link
Member

Marusyk commented Jun 5, 2022

So, should I return the previous version with HashSet?

"Constantize" regex options via EvaluatableFilter

I'm not sure that understand this task. Do I need to implement CosmosEvaluatableExpressionFilter?

@roji
Copy link
Member

roji commented Jun 5, 2022

So, should I return the previous version with HashSet?

Nope, I don't see any reason for doing so. The current code does the right thing (for now) - if there's any unsupported option, it just returns null.

Do I need to implement CosmosEvaluatableExpressionFilter?

Yeah, @smitpatel can probably offer some more assistance if needed.

@roji
Copy link
Member

roji commented Jun 14, 2022

@Marusyk do you intend to work on this?

@Marusyk
Copy link
Member

Marusyk commented Jun 15, 2022

Unfortunately, I don't have time to investigate EvaluatableExpressionFilter.

@roji
Copy link
Member

roji commented Jun 17, 2022

Discussed with @smitpatel and the constantization of the RegexOptions parameters would require some infra work, so we'll leave that for now.

@roji roji added closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. good first issue This issue should be relatively straightforward to fix. and removed good first issue This issue should be relatively straightforward to fix. labels Jun 17, 2022
@ghost ghost closed this as completed in #28261 Jun 17, 2022
ghost pushed a commit that referenced this issue Jun 17, 2022
@ajcvickers ajcvickers modified the milestones: 7.0.0, 7.0.0-preview6 Jun 20, 2022
@ajcvickers ajcvickers modified the milestones: 7.0.0-preview6, 7.0.0 Nov 5, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-cosmos area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. good first issue This issue should be relatively straightforward to fix. type-enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants