Implement LEAD, LAG, FIRST_VALUE, LAST_VALUE window functions#2444
Implement LEAD, LAG, FIRST_VALUE, LAST_VALUE window functions#2444apoorvdarshan wants to merge 4 commits intoAlaSQL:developfrom
Conversation
…#2409) Add support for SQL:2003 positional window functions with PARTITION BY and ORDER BY. These enable period-over-period comparisons and accessing relative row values within partitions.
|
Are you intending to complete the test plan, or should I review now? |
|
Test plan complete — all items verified, ready for your review. Re-ran the full suite locally: 2457 passing, 0 failing, plus all 16 new tests in |
…rst-last-value-window-functions
mathiasrw
left a comment
There was a problem hiding this comment.
Nice work with making the solution work. But we should be focusing on using the jison grammar so we can use caching and pre compile instead of doing function detection at runtime.
There was a problem hiding this comment.
Nice way to structure this functionality
| query.grouprownums.push({as: col.as, columnIndex: 0}); // Track which column to use for grouping | ||
| } | ||
|
|
||
| // Detect positional window functions: LEAD, LAG, FIRST_VALUE, LAST_VALUE |
There was a problem hiding this comment.
Lets move the detection into the parser so we dont pollute the areas of concern.
Please consider how we can add lead, lag, first value, last value to the grammar in alasqlparser.jison (use yarn jison to build after changing the jison)
| stdlib.LEAD = function () { | ||
| return 'undefined'; | ||
| }; | ||
| stdlib.LAG = function () { | ||
| return 'undefined'; | ||
| }; | ||
| stdlib.FIRST_VALUE = function () { | ||
| return 'undefined'; | ||
| }; | ||
| stdlib.LAST_VALUE = function () { | ||
| return 'undefined'; | ||
| }; |
There was a problem hiding this comment.
Its not great to have functions intended to not do anything other than being a flag for code to operate on. Lets move the functionality to the jison parser.
| {dept: 'IT', emp: 'Eve', salary: 2500}, | ||
| ]; | ||
|
|
||
| // --- LEAD tests --- |
There was a problem hiding this comment.
Please make sure the tests makes sense without comments
There was a problem hiding this comment.
I see now its because you are bundling across lead lag first last and so on.
I propose making a level of describe for each of these so its clear in the code and ind the output.
So instead of the comment on this line its something like
describe('Test 2409 - LEAD/LAG/FIRST_VALUE/LAST_VALUE Window Functions', function () {
describe('Test 2409 - LEAD Window Functions', function () {
test(...)
}
describe('Test 2409 - LAG Window Functions', function () {
...
}
| assert.strictEqual(res[0].next_salary, 1200); | ||
| assert.strictEqual(res[1].next_salary, 1500); | ||
| assert.strictEqual(res[4].next_salary, null); |
There was a problem hiding this comment.
Please seek to compare the output from alasql with the complete output json object so we provide the total output expected as one object.
Adds a typed yy.PositionalWindowFunc AST node produced by the FuncValue grammar rule so detection happens at parse time and can be cached/precompiled. Removes the runtime funcid string-matching from compileSelectGroup0 and the no-op stdlib stubs that only existed as flags for the runtime to detect. Restructures test/test2409.js into nested describes per function with deepStrictEqual assertions on full result objects.
|
Addressed in d39981d. Summary of changes: Detection moved into the parser (
Runtime detection block removed (
No-op stdlib stubs removed (
Tests restructured (
Full suite: 2457 passing, 0 failing. |
Summary
LEAD(),LAG(),FIRST_VALUE(),LAST_VALUE()(resolves Window Offset Functions (LEAD/LAG/FIRST_VALUE/LAST_VALUE) Not Implemented #2409)PARTITION BYandORDER BYin OVER clauses, with configurable offset and default values for LEAD/LAGTest plan
test/test2409.jspass