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

Remove static count(), delete() and deleteAll() methods from YqlStatement #104

Merged
merged 1 commit into from
Dec 4, 2024

Conversation

lavrukov
Copy link
Contributor

@lavrukov lavrukov commented Dec 4, 2024

No description provided.

@lavrukov lavrukov force-pushed the table-refactoring-rem-count-del branch from 05b48e8 to aae8add Compare December 4, 2024 10:50
@nvamelichev nvamelichev changed the title Remove static count(), delete() and deleteAll() methods from YqlState… Remove static count(), delete() and deleteAll() methods from YqlStatement Dec 4, 2024
Copy link
Collaborator

@nvamelichev nvamelichev left a comment

Choose a reason for hiding this comment

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

👍 LGTM overall, have one comment on improving code generation in YqlPredicate.from(...)

@@ -76,6 +76,16 @@ public static void setUseLegacyRel(boolean value) {
YqlPredicate.useLegacyRel.set(value);
}

public static YqlPredicate from(Collection<? extends YqlStatementPart<?>> parts) {
YqlPredicate predicate = YqlPredicate.alwaysTrue();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This method will generate a bit better YQL code if you use YqlPredicate.and(Collection<YqlPredicate>) on a collection of parts that are instanceof YqlPredicate, instead instead of turning the YqlStatement.predicateFrom()'s Stream::reduce into a for loop. (The YqlPredicate.and() method also returns YqlPredicate.alwaysTrue() if the predicate collection is empty, so all the invariants of previous implementations would be satisfied, I think.)

@lavrukov lavrukov force-pushed the table-refactoring-rem-find-in branch from b11a0fa to b21b4bd Compare December 4, 2024 20:51
Base automatically changed from table-refactoring-rem-find-in to main December 4, 2024 20:55
@lavrukov lavrukov force-pushed the table-refactoring-rem-count-del branch from aae8add to 466bcf7 Compare December 4, 2024 20:57
@lavrukov lavrukov force-pushed the table-refactoring-rem-count-del branch from 466bcf7 to 50518bb Compare December 4, 2024 20:58
@lavrukov lavrukov merged commit 844386f into main Dec 4, 2024
1 check passed
@lavrukov lavrukov deleted the table-refactoring-rem-count-del branch December 4, 2024 21:01
nvamelichev pushed a commit that referenced this pull request Dec 6, 2024
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