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

fix: Pull Query Key Extraction Optimizations #8346

Merged
merged 2 commits into from
Nov 17, 2021
Merged

Conversation

hli21
Copy link
Contributor

@hli21 hli21 commented Nov 10, 2021

Description

Optimize the key constraint extraction logic.
This problem was reported in issue #6973

Testing done

Add new unit tests.

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@hli21 hli21 requested a review from a team as a code owner November 10, 2021 17:50
@hli21 hli21 changed the title Pull Query Key Extraction Optimizations #6973 Pull Query Key Extraction Optimizations Nov 10, 2021
@hli21 hli21 changed the title Pull Query Key Extraction Optimizations fix: Pull Query Key Extraction Optimizations Nov 10, 2021
@@ -577,6 +582,34 @@ private Object resolveKey(
FormatOptions.noEscape())))
.orElse(null);
}

private void getConstraint(
Copy link
Member

Choose a reason for hiding this comment

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

This method name is a bit vague and sounds like a getter. Maybe call it setMostSelectiveConstraint or something like that? It would also be good to add a comment about what this does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

assertThat(keys.size(), is(1));
assertThat(keys.get(0), instanceOf(KeyConstraint.class));
final KeyConstraint keyConstraint = (KeyConstraint) keys.get(0);
assertThat(keyConstraint.getKey(), is(GenericKey.genericKey(3)));
Copy link
Member

Choose a reason for hiding this comment

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

Want to check the operator to show that you have the equals as well? Finding a three probably indicates that the one you got, but good to be sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

assertThat(keys.size(), is(1));
assertThat(keys.get(0), instanceOf(KeyConstraint.class));
final KeyConstraint keyConstraint = (KeyConstraint) keys.get(0);
assertThat(keyConstraint.getKey(), is(GenericKey.genericKey("v2")));
Copy link
Member

Choose a reason for hiding this comment

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

Would be great to check the operator here and elsewhere too just to be sure we didn't accidentally swap them with other key values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@hli21 hli21 merged commit 6ad333e into confluentinc:master Nov 17, 2021
@hli21 hli21 deleted the fix6973 branch April 18, 2022 23:15
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.

2 participants