[WIP] Implement LET clause similar to OrientDB#2535
Conversation
|
@copilot continue |
Addressed in |
|
@copilot the inital comment indicates that some tasks are not completed. Can you please confirm? |
The original checklist is stale. The regression test, SELECT LET support, targeted validation, secret scan, and final review/security checks were completed in |
| try { | ||
| if (query.wherefn(scope, query.params, alasql)) { | ||
| // If there is a GROUP BY then pipe to grouping function | ||
| if (query.groupfn) { | ||
| query.groupfn(scope, query.params, alasql); | ||
| } else { | ||
| query.data.push(query.selectfn(scope, query.params, alasql)); | ||
| } | ||
| } | ||
| } finally { | ||
| query.params = originalParams; |
There was a problem hiding this comment.
Are we expecting the commands to error? What is the concern? There is a performance hit on this BUT worse, it seems like we dont trust our data structure of our logic.
There was a problem hiding this comment.
Updated in ee4e5e7: LET now restores only the touched variables after evaluation instead of cloning/swapping the whole params object.
| .map(function (assignment) { | ||
| return ( | ||
| assignment.method + assignment.variable + ' = ' + assignment.expression.toString() | ||
| ); | ||
| }) |
There was a problem hiding this comment.
Is this the most efficient way?
There was a problem hiding this comment.
Tightened in ee4e5e7 to a small shared formatter helper.
|
|
||
| compileLet(query) { | ||
| if (!this.let || this.let.length === 0) return; | ||
|
|
||
| const lets = this.let | ||
| .map(assignment => { | ||
| const target = | ||
| assignment.method === '@' | ||
| ? `alasql.vars[${JSON.stringify(assignment.variable)}]` | ||
| : `params[${JSON.stringify(assignment.variable)}]`; | ||
| return `${target}=${assignment.expression.toJS('p', query.defaultTableid, query.defcols)};`; | ||
| }) | ||
| .join(''); | ||
|
|
||
| query.letfn = new Function( | ||
| 'p,params,alasql', | ||
| `var y; | ||
| if(Array.isArray(params)) { | ||
| params = Object.assign([], params); | ||
| } else if(params && typeof params === 'object') { | ||
| params = Object.assign({}, params); | ||
| } else { | ||
| params = {}; | ||
| } | ||
| ${lets} | ||
| return params;` | ||
| ); | ||
| } | ||
| }; |
There was a problem hiding this comment.
do we normally move things out of the main loop? Also, seems like a lot of code for such a change.
There was a problem hiding this comment.
Reduced in ee4e5e7: LET now precompiles only the value expressions and applies/restores touched variables at evaluation time.
There was a problem hiding this comment.
We need many more exampled and edgecases covered.
There was a problem hiding this comment.
Expanded in ee4e5e7 with coverage for filtering/selection, restoring existing named params, GROUP BY/HAVING, and cleanup of temporary @ variables.
Co-authored-by: mathiasrw <1063454+mathiasrw@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.