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

Enhancement/push filter down AppendVertices #5490

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

czpmango
Copy link
Contributor

@czpmango czpmango commented Apr 13, 2023

What type of PR is this?

  • bug
  • feature
  • enhancement

What problem(s) does this PR solve?

Issue(s) number:

Fix #5492
Fix https://github.com/vesoft-inc/nebula-ent/issues/2604

Description:

Just move the optimizer rule from ent version.

Special notes for your reviewer, ex. impact of this fix, design document, etc:

Checklist:

Tests:

  • Unit test(positive and negative cases)
  • Function test
  • Performance test
  • N/A

Affects:

  • Documentation affected (Please add the label if documentation needs to be modified.)
  • Incompatibility (If it breaks the compatibility, please describe it and add the label.)
  • If it's needed to cherry-pick (If cherry-pick to some branches is required, please label the destination version(s).)
  • Performance impacted: Consumes more CPU/Memory

@czpmango czpmango added the ready-for-testing PR: ready for the CI test label Apr 13, 2023
@czpmango czpmango force-pushed the enhancement/PushFilterDownAppendVertices branch from 39a1885 to 8bf34ae Compare April 13, 2023 03:27
@czpmango czpmango added the do not review PR: not ready for the code review yet label Apr 13, 2023
@czpmango czpmango marked this pull request as ready for review April 13, 2023 03:49
@czpmango czpmango removed the do not review PR: not ready for the code review yet label Apr 13, 2023
@czpmango czpmango force-pushed the enhancement/PushFilterDownAppendVertices branch 5 times, most recently from e0d12ae to 6b3083e Compare April 14, 2023 08:53
@czpmango czpmango added ready for review auto-sync ready-for-testing PR: ready for the CI test and removed ready-for-testing PR: ready for the CI test labels Apr 14, 2023
@czpmango czpmango force-pushed the enhancement/PushFilterDownAppendVertices branch from 36f543f to 2069a2d Compare April 14, 2023 10:09
@czpmango czpmango requested review from yixinglu, jievince and xtcyclist and removed request for yixinglu April 17, 2023 02:12
Add tck

small change

fix tck

fix tck

Fix tck

flush vscode ...

small fix

Fix tck
@czpmango czpmango force-pushed the enhancement/PushFilterDownAppendVertices branch from 3a0bc58 to e22e6c7 Compare April 17, 2023 02:18
jievince
jievince previously approved these changes Apr 17, 2023
auto pool = qctx->objPool();
auto condition = graph::ExpressionUtils::rewriteVertexPropertyFilter(
pool, appendVertices->colNames().back(), filter->condition()->clone());

Copy link
Contributor

Choose a reason for hiding this comment

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

Need to port the latest version of this file from the ent repo. I introduced a bug fix here at line:48.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which pr?

| 1 | PassThrough | 3 | |
| 3 | Start | | |

# FIXME(czp): Disable this for now. The ngdata test set lacks validation mechanisms
Copy link
Contributor

@xtcyclist xtcyclist Apr 17, 2023

Choose a reason for hiding this comment

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

Why disable this test? At least we need to check the code, if these queries produced different results. Did you encounter a different result here? Being different itself is a potential problem.

We need to have complex datasets like ngdata in the CI. It's necessary. Validation could be improved later.

# | id | name | dependencies | operator info |
# | 9 | Aggregate | 11 | |
# | 11 | Project | 10 | |
# | 10 | Filter | 6 | |
Copy link
Contributor

Choose a reason for hiding this comment

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

This filter was gone in the ent version, after pushing the filter down into appendvertices, hence there is such a tck case here.

@czpmango czpmango marked this pull request as draft April 17, 2023 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

match with regular expression return error
4 participants