Skip to content

Conversation

@AviAvni
Copy link
Contributor

@AviAvni AviAvni commented Oct 7, 2021

No description provided.

@jeffreylovitz
Copy link
Contributor

The TCK test:

MATCH (a)-[r]->(b:X) WITH a, r, b MATCH (a)-[r]->(b) RETURN r AS rel ORDER BY rel.id

fails due to #1883.

Given this optimization, records are now passed down immediately rather than being enqueued and popped, so the order they are received is reversed. This test previously passed accidentally.

@jeffreylovitz jeffreylovitz requested a review from swilly22 October 7, 2021 21:05
@codecov
Copy link

codecov bot commented Oct 8, 2021

Codecov Report

Merging #1976 (6f48202) into master (019bda3) will increase coverage by 0.76%.
The diff coverage is 94.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1976      +/-   ##
==========================================
+ Coverage   91.90%   92.66%   +0.76%     
==========================================
  Files         239      239              
  Lines       20372    20512     +140     
==========================================
+ Hits        18723    19008     +285     
+ Misses       1649     1504     -145     
Impacted Files Coverage Δ
src/ast/cypher_whitelist.c 87.50% <ø> (ø)
src/graph/graph.c 99.06% <ø> (ø)
src/commands/execution_ctx.c 95.16% <75.00%> (-1.45%) ⬇️
src/execution_plan/ops/op_expand_into.c 91.89% <89.01%> (-1.45%) ⬇️
src/arithmetic/string_funcs/string_funcs.c 99.14% <95.23%> (+10.13%) ⬆️
...hmetic/algebraic_expression/algebraic_expression.c 84.89% <100.00%> (+0.22%) ⬆️
src/arithmetic/arithmetic_expression_construct.c 92.25% <100.00%> (+0.81%) ⬆️
src/arithmetic/entity_funcs/entity_funcs.c 98.71% <100.00%> (+0.30%) ⬆️
src/commands/cmd_explain.c 84.21% <100.00%> (+0.42%) ⬆️
src/commands/cmd_query.c 96.62% <100.00%> (ø)
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 637a5ac...6f48202. Read the comment docs.

@swilly22 swilly22 force-pushed the expand-into-optimization branch from 9d0443d to 58021fb Compare October 9, 2021 19:19
if(r == NULL) break;

// set the matrix on first invocation
if(op->M == NULL && op->single_operand) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

why this is not on the init function? is optimize could make not operand to operand?

Record_PersistScalars(childRecord);
op->records[op->record_count] = childRecord;
// store received record
// TODO: not sure if necessary when we're streaming records
Copy link
Contributor Author

Choose a reason for hiding this comment

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

how to check if this necessary

@swilly22 swilly22 merged commit 474527a into master Oct 10, 2021
@swilly22 swilly22 deleted the expand-into-optimization branch October 10, 2021 12:36
pnxguide pushed a commit to CMU-SPEED/RedisGraph that referenced this pull request Mar 22, 2023
* use operand matrix when available

* Add ExpandIntoSimpleConsume routine

* refactor expand_into op

* resolve expand-into optimization at init time

* disable only param test

Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com>
Co-authored-by: swilly22 <roi@redislabs.com>
Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>
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.

4 participants