Skip to content

Cardinality of filter compared to zero#13867

Closed
kaikalur wants to merge 1 commit intoprestodb:masterfrom
kaikalur:cardinality_of_filter_compared_to_zero
Closed

Cardinality of filter compared to zero#13867
kaikalur wants to merge 1 commit intoprestodb:masterfrom
kaikalur:cardinality_of_filter_compared_to_zero

Conversation

@kaikalur
Copy link
Copy Markdown
Contributor

@kaikalur kaikalur commented Dec 14, 2019

We rewrite expressions of the CARDINALITY(FILTER(array, lambda)) =/> 0 to none_match/anymatch(array, lambda) for preformance.

== NO RELEASE NOTE ==

@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla bot commented Dec 14, 2019

CLA Check
The committers are authorized under a signed CLA.

  • ✅ Sreeni Viswanadha (9aff6af49ac32ad34d404b77dc34747274d6b0e0)

@kaikalur kaikalur force-pushed the cardinality_of_filter_compared_to_zero branch from c2440c8 to 9aff6af Compare December 14, 2019 00:03
@kaikalur kaikalur force-pushed the cardinality_of_filter_compared_to_zero branch from 9aff6af to c9d4bbe Compare December 14, 2019 00:07
@kaikalur
Copy link
Copy Markdown
Contributor Author

OK so the previous PR was contentious :) so I removed all the complex stuffs and just added a very simple rewrite that uses any_match/none_match for comparing cardinality(filter) to 0. Please check it out.

For reference, here is the previous PR:

#13721

Sorry I couldn't update that PR properly so starting a fresh one. Thanks!

@kaikalur kaikalur requested a review from rongrong December 14, 2019 00:11
@kaikalur kaikalur force-pushed the cardinality_of_filter_compared_to_zero branch 7 times, most recently from 919b66a to f9fc221 Compare December 15, 2019 03:49
@kaikalur kaikalur force-pushed the cardinality_of_filter_compared_to_zero branch from f9fc221 to 80bca3d Compare December 16, 2019 03:34
@wenleix wenleix self-assigned this Dec 16, 2019
return expression instanceof Literal;
}

private static boolean isZero(Expression expression)
Copy link
Copy Markdown
Contributor

@wenleix wenleix Dec 27, 2019

Choose a reason for hiding this comment

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

cc @highker, @hellium01 : Is this the recommended way to see whether a expression will be evaluated into certain constant?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Not really; we need to use interpreter and evaluate the expression. The return result of the eval should be a const with 0 as its value.

@wenleix wenleix requested review from hellium01 and highker December 27, 2019 00:55
{
if (expression instanceof FunctionCall) {
FunctionCall functionCall = (FunctionCall) expression;
return functionCall.getName().toString().equals("cardinality") &&
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We used to use resolveFunction to get the function handle. But given now it requires transactions, maybe @rongrong can be a better PoC to comment on.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

resolveFunction now only takes an optional transaction ID. Since this is a static function you can just pass in Optional.empty().

@stale
Copy link
Copy Markdown

stale bot commented Aug 5, 2020

This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the task, make sure you've addressed reviewer comments, and rebase on the latest master. Thank you for your contributions!

@stale stale bot added the stale label Aug 5, 2020
@stale stale bot closed this Aug 14, 2020
@kaikalur kaikalur deleted the cardinality_of_filter_compared_to_zero branch August 14, 2020 23:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants