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

UpdateTargetLocator->ActionTargetLocator, refactor part 2 #262

Merged
merged 13 commits into from
Mar 15, 2023

Conversation

tatu-at-datastax
Copy link
Contributor

@tatu-at-datastax tatu-at-datastax commented Mar 14, 2023

What this PR does:

Further refactoring of existing UpdateTarget[Locator] into ActionTarget[Locator] for more efficient handling, reuse.

Specifically now decoding of target locator for UpdateOperations occurs during construction, and not evaluation: this reduces processing for "updateMany()" use case. Similarly added method (ActionTargetLocator.findValueIn()) is planned to be usable for in-memory Sorting (and later on hopefully with Projection as well).

Also pushes handling of Actions (per-field part of full Update Operation) to base class to allow centralized path-conflict checking (to solve #232 in near future)

Which issue(s) this PR fixes:
Fixes #257

Checklist

  • Changes manually tested
  • Automated Tests added/updated
  • Documentation added/updated
  • CLA Signed: DataStax CLA

@tatu-at-datastax tatu-at-datastax self-assigned this Mar 14, 2023
@tatu-at-datastax tatu-at-datastax marked this pull request as ready for review March 15, 2023 00:18
@tatu-at-datastax tatu-at-datastax requested a review from a team as a code owner March 15, 2023 00:18
Copy link
Contributor

@maheshrajamani maheshrajamani left a comment

Choose a reason for hiding this comment

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

@tatu-at-datastax Can you see if we can move the 2 class to util package. Also there is a unit test failure.

*/
public record UpdateTarget(
public record ActionTarget(
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as what is in UpdateTargetLocator.

* possibly Projection)
* </ul>
*/
public class ActionTargetLocator {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have this class named as just TargetLocator and move it to util package? That way we can use it for projection/sorting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maheshrajamani Yes, I will do that today. Was wondering about it since it's not really update (or update action) specific.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you going to do it as part of this PR?

@tatu-at-datastax
Copy link
Contributor Author

Renamed "UpdateTarget[Locator]" to "PathMatch[Locator]", moved to util. Did other related renaming, hopefully didn't miss too many references.

Copy link
Contributor

@maheshrajamani maheshrajamani left a comment

Choose a reason for hiding this comment

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

LGTM

@tatu-at-datastax tatu-at-datastax merged commit 280f5dc into main Mar 15, 2023
@tatu-at-datastax tatu-at-datastax deleted the tatu/257-refactor-actions-target-locator branch March 15, 2023 15:59
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.

Refactor UpdateTargetLocator as ActionTargetLocator for use with in-memory sorting, projection
2 participants