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

Correct the yeild var input bug. #2789

Closed
wants to merge 8 commits into from

Conversation

Shylock-Hg
Copy link
Contributor

@Shylock-Hg Shylock-Hg commented Sep 3, 2021

Close #2495 Close #2646

ps.Maybe I think disable user write variable expression is better, the variable expression just used inner.

@Shylock-Hg Shylock-Hg added the ready-for-testing PR: ready for the CI test label Sep 3, 2021
@Shylock-Hg Shylock-Hg requested a review from a team September 3, 2021 02:05
@Shylock-Hg Shylock-Hg added the type/bug Type: something is unexpected label Sep 3, 2021
@czpmango
Copy link
Contributor

czpmango commented Sep 7, 2021

Disable user-defined variables? What does "used inner" mean?

@Shylock-Hg
Copy link
Contributor Author

Disable user-defined variables? What does "used inner" mean?

The variable generated by developer when validating.

@nevermore3 nevermore3 added the need to discuss Solution: issue or PR without a clear conclusion on whether to handle it label Sep 17, 2021
@Sophie-Xie Sophie-Xie added this to the v2.6.0 milestone Sep 22, 2021
@Shylock-Hg Shylock-Hg added the incompatible PR: incompatible with the recently released version label Sep 28, 2021
@Sophie-Xie Sophie-Xie removed the need to discuss Solution: issue or PR without a clear conclusion on whether to handle it label Sep 28, 2021
@Sophie-Xie Sophie-Xie removed this from the v2.6.0 milestone Sep 28, 2021
@@ -137,7 +137,10 @@ void DeducePropsVisitor::visit(InputPropertyExpression *expr) {

void DeducePropsVisitor::visit(VariablePropertyExpression *expr) {
exprProps_->insertVarProp(expr->sym(), expr->prop());
userDefinedVarNameList_->emplace(expr->sym());
if (expr->sym()[0] != '_') {
Copy link
Contributor

Choose a reason for hiding this comment

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

What does if (expr->sym()[0] != '_') mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Means variable from query instead of inner generated variable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a better alternative implementation.
It's very unfriendly to someone reading this code.

Comment on lines 11 to 24
Scenario: yield constant after pipe
When executing query:
"""
$var = GO FROM "Tim Duncan" OVER *; YIELD $var[0][0] as var00;
"""
Then the result should be, in any order:
| var00 |
| "Manu Ginobili" |
| "Manu Ginobili" |
| "Manu Ginobili" |
| "Manu Ginobili" |
| "Manu Ginobili" |
| "Manu Ginobili" |
| "Manu Ginobili" |
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is $var[0][0] not one row?

(czp@nebula) [nba]> $var = GO FROM "Tim Duncan" OVER *; YIELD $var[0][0] as var00;
+-----------------+
| var00           |
+-----------------+
| "Manu Ginobili" |
+-----------------+
Got 1 rows (time spent 7942/9138 us)

Mon, 25 Oct 2021 14:11:31 CST

(czp@nebula) [nba]> $var = GO FROM "Tim Duncan" OVER *; 
+-----------------+------------+---------------------+
| like._dst       | serve._dst | teammate._dst       |
+-----------------+------------+---------------------+
| "Manu Ginobili" |            |                     |
+-----------------+------------+---------------------+
| "Tony Parker"   |            |                     |
+-----------------+------------+---------------------+
|                 | "Spurs"    |                     |
+-----------------+------------+---------------------+
|                 |            | "Danny Green"       |
+-----------------+------------+---------------------+
|                 |            | "LaMarcus Aldridge" |
+-----------------+------------+---------------------+
|                 |            | "Manu Ginobili"     |
+-----------------+------------+---------------------+
|                 |            | "Tony Parker"       |
+-----------------+------------+---------------------+
Got 7 rows (time spent 7580/8871 us)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The result of project keep row count of input variable.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand this, but the inconsistency with the semantics of the statement is confusing to users.

Copy link
Contributor Author

@Shylock-Hg Shylock-Hg Oct 25, 2021

Choose a reason for hiding this comment

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

A little strange. We also could add more input variables to Project to get one row result, but this need discussion.

@Shylock-Hg Shylock-Hg requested a review from czpmango October 25, 2021 07:46
czpmango
czpmango previously approved these changes Oct 25, 2021
@Shylock-Hg
Copy link
Contributor Author

Replace by #3271

@Shylock-Hg Shylock-Hg closed this Nov 17, 2021
@Shylock-Hg Shylock-Hg deleted the correct/yield-var branch February 9, 2022 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
incompatible PR: incompatible with the recently released version ready-for-testing PR: ready for the CI test type/bug Type: something is unexpected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mistake check of yield expression in yield sentence. Mistake check of yield expression in yield sentence.
4 participants