Fix `LIMIT` clause execution order #300

alancai98 · 2020-10-01T21:30:25Z

Fixes #281

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

codecov-commenter · 2020-10-01T21:39:07Z

Codecov Report

Merging #300 into master will increase coverage by 0.00%.
The diff coverage is 78.26%.

@@            Coverage Diff            @@
##             master     #300   +/-   ##
=========================================
  Coverage     82.49%   82.50%           
- Complexity     1218     1219    +1     
=========================================
  Files           157      157           
  Lines          9264     9263    -1     
  Branches       1513     1512    -1     
=========================================
  Hits           7642     7642           
+ Misses         1170     1169    -1     
  Partials        452      452

Flag	Coverage Δ	Complexity Δ
#CLI	`18.11% <ø> (ø)`	`19.00 <ø> (ø)`
#EXAMPLES	`76.01% <ø> (ø)`	`27.00 <ø> (ø)`
#LANG	`85.20% <78.26%> (+0.01%)`	`1016.00 <3.00> (+1.00)`
#PTS	`100.00% <ø> (ø)`	`0.00 <ø> (ø)`
#TEST_SCRIPT	`79.68% <ø> (ø)`	`157.00 <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
...ng/src/org/partiql/lang/eval/EvaluatingCompiler.kt	`83.57% <78.26%> (-0.03%)`	`151.00 <3.00> (+1.00)`	⬇️
...ang/src/org/partiql/lang/ast/AstDeserialization.kt	`85.89% <0.00%> (+0.18%)`	`0.00% <0.00%> (ø%)`

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 6f206a9...ad2aa3e. Read the comment docs.

lang/src/org/partiql/lang/eval/EvaluatingCompiler.kt

dlurton · 2020-10-05T21:25:25Z

lang/src/org/partiql/lang/eval/EvaluatingCompiler.kt

                                // wrap the ExprValue to use ExprValue.equals as the equality
                                SetQuantifier.DISTINCT -> projectedRows.filter(createUniqueExprValueFilter())
                                SetQuantifier.ALL -> projectedRows
                            }

+                            if (limit != null) {
+                                quantifiedRows = quantifiedRows.take(compileLimit(limit, env))


Since this is inside a closure that's passed to thunkFactory.thunkEnv (line 968), the call to compileLimit is being invoked at evaluation-time, which means the limit clause is being re-compiled once for every row--I think. At least this is being recompiled every time the query is executed. Either is a problem. Compilation should always happen once and only once.

To fix this move the compileLimit call outside of the closure, i.e. to 967 or even higher.

This is an important aspect of how EvaluatingCompiler works: a significant portion of the the code in it executes at evaluation-time and not-compile time because it resides in closures (thunks) such as this one. It is important to avoid re-compiling parts of expressions within the body of a thunk.

Good point. In the new commit, I separated out the compilation (L955) from the evaluation (L892) so the compilation isn't done for every row.

dlurton

Need to avoid re-compiling the LIMIT clause by moving it's thunk creations outside of the body of all of its thunks.

dlurton · 2020-10-05T21:27:29Z

lang/src/org/partiql/lang/eval/EvaluatingCompiler.kt

                                // wrap the ExprValue to use ExprValue.equals as the equality
                                SetQuantifier.DISTINCT -> projectedRows.filter(createUniqueExprValueFilter())
                                SetQuantifier.ALL -> projectedRows
                            }

+                            if (limit != null) {
+                                quantifiedRows = quantifiedRows.take(compileLimit(limit, env))


This is an important aspect of how EvaluatingCompiler works: a significant portion of the the code in it executes at evaluation-time and not-compile time because it resides in closures (thunks) such as this one. It is important to avoid re-compiling parts of expressions within the body of a thunk.

dlurton · 2020-10-05T21:31:43Z

lang/src/org/partiql/lang/eval/EvaluatingCompiler.kt

                                    val registers = createRegisterBank()

                                    // note: the group key can be anything here because we only ever have a single
                                    // group when aggregates are used without GROUP BY expression
                                    val syntheticGroup = Group(valueFactory.nullValue, registers)

+                                    if (limit != null) {
+                                        fromProductions = fromProductions.take(compileLimit(limit, env))


Move compileLimit outside of thunk body.

In the new commit, I separated out the compilation (L955) from the evaluation (L892) so the compilation isn't done from within the thunk evaluation.

lang/src/org/partiql/lang/eval/EvaluatingCompiler.kt

codecov-io · 2020-10-07T02:49:18Z

Codecov Report

❗ No coverage uploaded for pull request base (master@36fe55b). Click here to learn what that means.
The diff coverage is 78.26%.

@@            Coverage Diff            @@
##             master     #300   +/-   ##
=========================================
  Coverage          ?   82.39%           
  Complexity        ?     1228           
=========================================
  Files             ?      157           
  Lines             ?     9359           
  Branches          ?     1524           
=========================================
  Hits              ?     7711           
  Misses            ?     1190           
  Partials          ?      458

Flag	Coverage Δ	Complexity Δ
#CLI	`18.11% <ø> (?)`	`19.00 <ø> (?)`
#EXAMPLES	`76.01% <ø> (?)`	`27.00 <ø> (?)`
#LANG	`85.04% <78.26%> (?)`	`1025.00 <3.00> (?)`
#PTS	`100.00% <ø> (?)`	`0.00 <ø> (?)`
#TEST_SCRIPT	`79.68% <ø> (?)`	`157.00 <ø> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
...ng/src/org/partiql/lang/eval/EvaluatingCompiler.kt	`81.21% <78.26%> (ø)`	`151.00 <3.00> (?)`

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 36fe55b...279d733. Read the comment docs.

alancai98 added 2 commits October 1, 2020 17:24

Fix incorrect execution order for LIMIT clause

c9b49ae

Merge remote-tracking branch 'origin/master' into limit-execution-order

abeaac1

alancai98 self-assigned this Oct 1, 2020

alancai98 requested review from dlurton, abhikuhikar and therapon October 1, 2020 21:30

alancai98 commented Oct 2, 2020

View reviewed changes

lang/src/org/partiql/lang/eval/EvaluatingCompiler.kt Outdated Show resolved Hide resolved

therapon previously approved these changes Oct 5, 2020

View reviewed changes

abhikuhikar suggested changes Oct 5, 2020

View reviewed changes

lang/src/org/partiql/lang/eval/EvaluatingCompiler.kt Show resolved Hide resolved

lang/src/org/partiql/lang/eval/EvaluatingCompiler.kt Outdated Show resolved Hide resolved

lang/src/org/partiql/lang/eval/EvaluatingCompiler.kt Outdated Show resolved Hide resolved

dlurton reviewed Oct 5, 2020

View reviewed changes

dlurton suggested changes Oct 5, 2020

View reviewed changes

Separated compiling and evaluation for LIMIT. Avoided use of var

ad2aa3e

alancai98 dismissed therapon’s stale review via ad2aa3e October 5, 2020 23:54

alancai98 requested review from dlurton, abhikuhikar and therapon October 6, 2020 00:02

abhikuhikar previously approved these changes Oct 6, 2020

View reviewed changes

therapon previously approved these changes Oct 6, 2020

View reviewed changes

Merge branch 'master' into limit-execution-order

279d733

alancai98 dismissed stale reviews from therapon and abhikuhikar via 279d733 October 7, 2020 02:29

therapon approved these changes Oct 8, 2020

View reviewed changes

therapon approved these changes Oct 14, 2020

View reviewed changes

dlurton approved these changes Oct 19, 2020

View reviewed changes

alancai98 merged commit c1dedfc into master Oct 19, 2020

alancai98 deleted the limit-execution-order branch October 19, 2020 22:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix `LIMIT` clause execution order #300

Fix `LIMIT` clause execution order #300

Uh oh!

alancai98 commented Oct 1, 2020

Uh oh!

codecov-commenter commented Oct 1, 2020 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dlurton Oct 5, 2020 •

edited

Loading

Uh oh!

dlurton Oct 5, 2020

Uh oh!

alancai98 Oct 5, 2020 •

edited

Loading

Uh oh!

dlurton left a comment

Uh oh!

dlurton Oct 5, 2020

Uh oh!

dlurton Oct 5, 2020

Uh oh!

alancai98 Oct 6, 2020

Uh oh!

Uh oh!

codecov-io commented Oct 7, 2020 •

edited

Loading

Uh oh!

Uh oh!

Fix LIMIT clause execution order #300

Fix LIMIT clause execution order #300

Uh oh!

Conversation

alancai98 commented Oct 1, 2020

Uh oh!

codecov-commenter commented Oct 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dlurton Oct 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlurton Oct 5, 2020

Choose a reason for hiding this comment

Uh oh!

alancai98 Oct 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlurton left a comment

Choose a reason for hiding this comment

Uh oh!

dlurton Oct 5, 2020

Choose a reason for hiding this comment

Uh oh!

dlurton Oct 5, 2020

Choose a reason for hiding this comment

Uh oh!

alancai98 Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-io commented Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Fix `LIMIT` clause execution order #300

Fix `LIMIT` clause execution order #300

codecov-commenter commented Oct 1, 2020 •

edited

Loading

dlurton Oct 5, 2020 •

edited

Loading

alancai98 Oct 5, 2020 •

edited

Loading

codecov-io commented Oct 7, 2020 •

edited

Loading