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

Adds support for running EVAL with different scripting engines #1497

Open
wants to merge 2 commits into
base: unstable
Choose a base branch
from

Conversation

rjd15372
Copy link
Contributor

@rjd15372 rjd15372 commented Dec 31, 2024

In this PR we re-implement the EVAL commands (EVAL, EVALSHA, SCRIPT LOAD, etc...) to use the scripting engine infrastructure introduced in 6adef8e. This allows EVAL to run scripts using different scripting engines.

The Lua scripting engine implementation code was moved into its own subdirectory src/lua.

This new implementation generalizes the module API for implementing scripting engines to work with both FUNCTION and EVAL commands.

Module API changes include:

  • Rename of callback ValkeyModuleScriptingEngineCreateFunctionsLibraryFunc to ValkeyModuleScriptingEngineCompileCodeFunc.
  • Addition of a new enum enum ValkeyModuleScriptingEngineSubsystemType to specify the scripting engine subsystem (EVAL, or FUNCTION, or both).
  • In most callbacks was added a new parameter with the type ValkeyModuleScriptingEngineSubsystemType.
  • New callback specific for EVAL ValkeyModuleScriptingEngineResetEvalEnvFunc.
  • New API function ValkeyModuleScriptingEngineExecutionState (*ValkeyModule_GetFunctionExecutionState)(ValkeyModuleScriptingEngineServerRuntimeCtx *server_ctx) that is used by scripting engines to query the server about the execution state of the script that is running.

Fixes #1261
Follow-up of #1277

Copy link

codecov bot commented Dec 31, 2024

Codecov Report

Attention: Patch coverage is 62.76978% with 414 lines in your changes missing coverage. Please review.

Project coverage is 71.03%. Comparing base (921ba19) to head (b8e97eb).
Report is 1 commits behind head on unstable.

Files with missing lines Patch % Lines
src/lua/debug_lua.c 39.27% 354 Missing ⚠️
src/lua/engine_lua.c 88.48% 19 Missing ⚠️
src/eval.c 91.83% 16 Missing ⚠️
src/lua/script_lua.c 70.00% 9 Missing ⚠️
src/server.c 25.00% 9 Missing ⚠️
src/module.c 20.00% 4 Missing ⚠️
src/scripting_engine.c 92.59% 2 Missing ⚠️
src/lua/function_lua.c 97.67% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #1497      +/-   ##
============================================
+ Coverage     70.98%   71.03%   +0.05%     
============================================
  Files           120      123       +3     
  Lines         65095    65298     +203     
============================================
+ Hits          46210    46387     +177     
- Misses        18885    18911      +26     
Files with missing lines Coverage Δ
src/defrag.c 91.98% <ø> (+2.35%) ⬆️
src/functions.c 94.31% <100.00%> (+2.30%) ⬆️
src/lazyfree.c 86.11% <100.00%> (ø)
src/server.h 100.00% <ø> (ø)
src/lua/function_lua.c 98.69% <97.67%> (ø)
src/scripting_engine.c 76.85% <92.59%> (ø)
src/module.c 9.61% <20.00%> (+<0.01%) ⬆️
src/lua/script_lua.c 89.74% <70.00%> (ø)
src/server.c 87.47% <25.00%> (-0.20%) ⬇️
src/eval.c 89.13% <91.83%> (+32.06%) ⬆️
... and 2 more

... and 9 files with indirect coverage changes

@rjd15372 rjd15372 marked this pull request as ready for review January 16, 2025 10:43
Signed-off-by: Ricardo Dias <ricardo.dias@percona.com>
Signed-off-by: Ricardo Dias <ricardo.dias@percona.com>
@rjd15372
Copy link
Contributor Author

@zuiderkwast this is ready for review.

Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

It's a huge change.

The module API changes look good to me.

It's impossible to proof-read the logic but I guess we have pretty good test coverage. Breaking up eval.c into smaller units is good idea, just hard to review. Can you summarize which code was just moved unchanged and which code was written, so it's possible to tell what to review carefully and what not?

@@ -0,0 +1,992 @@
#include "debug_lua.h"
Copy link
Contributor

@zuiderkwast zuiderkwast Jan 23, 2025

Choose a reason for hiding this comment

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

We should have some copyright/license comment at the top. I believe 3 lines are enough:

# Copyright xxxxxxxxxxxx
# All rights reserved.
# SPDX-License-Identifier: BSD-3-Clause

We use Copyright 2025- Valkey contributors for new files, but for moved code we should probably keep the old names and years too.

@@ -61,6 +61,7 @@
#include "hdr_histogram.h"
#include "crc16_slottable.h"
#include "valkeymodule.h"
#include "module.h"
#include "io_threads.h"
#include "module.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

module.h is included twice here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

[NEW] Add scripting languages (for EVAL, etc.) using module API
2 participants