Open
Conversation
rafal-hawrylak
force-pushed
the
cli_jit_integration
branch
from
fdccca4 to
371b080
Compare
rafal-hawrylak
requested review from
andrzej-grudzien,
apilaskowski,
ikholopov-omni and
kolina
rafal-hawrylak
force-pushed
the
cli_jit_integration
branch
from
371b080 to
18042dd
Compare
kolina
requested changes
Mar 11, 2026
Contributor
kolina
left a comment
kolina
left a comment
There was a problem hiding this comment.
Let's test compatibility with different Dataform core versions.
Also this change seems worth it to bump a minor version of Dataform core
cli/api/commands/run.ts
Outdated
| private readonly dbadapter: dbadapters.IDbAdapter, | ||
| private readonly graph: dataform.IExecutionGraph, | ||
| private readonly executionOptions: IExecutionOptions = {}, | ||
| private readonly projectDir: string, |
Contributor
There was a problem hiding this comment.
if it's part of IExecutionOptions, do you need to pass it separately?
| type: "table", | ||
| tableType: "incremental", | ||
| jitCode: `async (jctx) => { | ||
| return jctx.incremental() ? "SELECT 'inc' as t" : "SELECT 'full' as t"; |
Contributor
There was a problem hiding this comment.
add a test for incremental mode?
cli/api/commands/compile.ts
Outdated
| this.childProcess = childProcess; | ||
| export class CompileChildProcess extends BaseWorker<string, string | Error> { | ||
| constructor() { | ||
| super("../../vm/compile_loader"); |
Contributor
There was a problem hiding this comment.
Do I understand correctly that this will change the order of fork script resolution? I'd prefer to keep the original order
Comment on lines
+14
to
+16
| // AoT compile() in cli/vm/compile.ts returns a JSON string of CompiledGraph. | ||
| // We need to wrap it in a CoreExecutionResponse and Base64 encode it | ||
| // to match what cli/api/commands/compile.ts expects. |
Contributor
There was a problem hiding this comment.
Is it true? From my reading of code:
compileincli/vm/compile.tscallsmainfunction of Dataform core and returns its result here, in the CLI we always pass base-64 core execution request here- The
mainfunction in the Dataform core in this case returns base64-encoded serialized proto response here (comment about older version of the Dataform CLI is a bit confusing)
| reserved 1, 3, 7, 13; | ||
| bool disabled = 15; | ||
|
|
||
| reserved 1, 3, 7; |
Contributor
There was a problem hiding this comment.
why do we have less reserved field numbers here?
tests/api/api.spec.ts
Outdated
| await callback(mockDbAdapterInstance); | ||
|
|
||
| const runner = new Runner(mockDbAdapterInstance, RUN_TEST_GRAPH); | ||
| const runner = new Runner(mockDbAdapterInstance, RUN_TEST_GRAPH, "."); |
Contributor
There was a problem hiding this comment.
If someone uses the Dataform CLI as a library, will they need to update their code in the same manner?
ikholopov-omni
requested changes
Mar 11, 2026
Collaborator
ikholopov-omni
left a comment
ikholopov-omni
left a comment
There was a problem hiding this comment.
Blocked on review of https://github.com/dataform-co/dataform/pull/2109/changes
* feat: add worker process management for JiT compilation - Introduce base worker and JiT-specific child process logic - Implement RPC bridge for database access during JiT - Add VM scripts and loader for isolated execution - Add unit tests for the RPC mechanism
- Enhance error coercion in common/errors/errors.ts - Sync protos/execution.proto and protos/jit.proto with new fields
* feat: add worker process management for JiT compilation - Introduce base worker and JiT-specific child process logic - Implement RPC bridge for database access during JiT - Add VM scripts and loader for isolated execution - Add unit tests for the RPC mechanism
* feat: add action descriptor support to JiT compilation results - Implement action and column descriptor logic in core/jit_compiler.ts - Support overrides for descriptions, labels, and tags in JiT results - Enhance error coercion in common/errors/errors.ts - Add comprehensive JiT metadata tests in core/jit_compiler_test.ts - Sync protos/execution.proto and protos/jit.proto with new result fields
rafal-hawrylak
force-pushed
the
cli_jit_integration
branch
7 times, most recently
from
7dc9046 to
9627902
Compare
* feat: enable JiT compilation in CLI run, build and compile commands - Connect main CLI execution flow to the JiT worker process - Add comprehensive JiT test suite - Finalize AoT build logic to preserve JiT-specific fields - Update CLI console and entry points for integrated JiT support
rafal-hawrylak
force-pushed
the
cli_jit_integration
branch
from
9627902 to
6f6cae6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.