Repository navigation
Conversation
…tent regardless of compilation mode. Add --execution-graph option to preserve old behavior
| | `--full-refresh` | boolean | `false` | Forces incremental tables (unless marked `protected`) to be rebuilt from scratch instead of updated incrementally (regular tables and views are always rebuilt). | | ||
| | `--credentials` | string | `.df-credentials.json` | Path to the credentials JSON file to use. | | ||
| | `--json` | boolean | `false` | Outputs the run result as JSON. Only supported together with `--dry-run` — plain `run --json` (without `--dry-run`) is rejected. | | ||
| | `--json` | boolean | `false` | Outputs the run result as JSON instead of a human-readable summary | |
There was a problem hiding this comment.
nit: add a trailing period to the --json description, wrap false in backticks (\false`) and add the missing closing
| !executionGraph.actions.some((action) => !!action.jitCode) | ||
| ) { | ||
| printExecutionGraph(executionGraph, isJsonOutput); | ||
| if (argv.executionGraph) { |
There was a problem hiding this comment.
Lets make sure we properly document this breaking change.
There was a problem hiding this comment.
How do you suggest documenting it aside from the changes to cli/README.md?
| ) { | ||
| printExecutionGraph(executionGraph, isJsonOutput); | ||
| if (argv.executionGraph) { | ||
| printExecutionGraph(executionGraph, argv.json); |
There was a problem hiding this comment.
The removed comment explained that the early graph print was skipped for JiT actions because their SQL is only produced once the Runner triggers JiT compilation. With --execution-graph that limitation comes back: JiT actions are emitted with raw jitCode and no compiled SQL. Worth documenting in the flag description/README (or warning on stderr), and adding a test for --execution-graph --json with JiT actions.
Also, this early return silently ignores --run-tests and --dry-run — should those combinations be rejected or documented?
There was a problem hiding this comment.
@rafal-hawrylak PTAL please as you implemented JIT support
There was a problem hiding this comment.
--dry-run --json now always runs Runner and populates compiledSql for both AoT and JiT. For --execution-graph, build() only sets jitCode with empty tasks since JiT compiles inside Runner, so documenting that and rejecting --dry-run or --run-tests with --execution-graph makes sense.
There was a problem hiding this comment.
The problem with skipping for JiT was that it led to the command having completely different behavior and outputs depending on whether JiT actions are present (make a dry run and print run result if they are, print the exec graph if they aren't). Execution graph is the intermediate stage after AoT compilation but before running (and thus before JiT compilation), so IMO it makes sense to leave JiT as is here. Warning on stderr might be a good idea though.
Rejecting when pairing --execution-graph with --run-tests and --dry-run might be a good idea as well.
There was a problem hiding this comment.
Added a warning if --run-tests or --dry-run also set + a warning if jit actions are present that they will not be compiled
| const logger = new Logger(!argv.json); | ||
|
|
||
| if (isJsonOutput && !argv.dryRun) { | ||
| if (argv.executionGraph && !argv.json) { |
There was a problem hiding this comment.
Why require --json here? printExecutionGraph(executionGraph, json) already supports a non-JSON mode, so allowing plain --execution-graph would avoid this extra validation path. If the restriction is intentional, maybe a shorter message: --execution-graph requires --json.
There was a problem hiding this comment.
I required it mostly because my intention in adding the option was to preserve the old behavior in case any users need it, and in the old version there was no way to obtain the non-json summary. But I suppose we can as well use that logic and allow --execution-graph without --json.
There was a problem hiding this comment.
I removed the requirement, and extended the non-JSON path of printExecutionGraph to also display the current warehouse state (since otherwise it would be essenitally identical to compile output).
I think the name --execution-graph is somewhat confusing now though, since it does not actually print the graph without --json - do you think maybe something like --build-only would be a better name?
| }); | ||
|
|
||
| if (actionsByName.size === 0) { | ||
| if (!argv.json && actionsByName.size === 0) { |
There was a problem hiding this comment.
With --json and no actions we now fall through and create a lineage emitter + runner just to get an empty result. Consider short-circuiting here instead: print an empty RunResult (status SUCCESSFUL, actions: []) and return 0.
| const defaultLocation = compiledGraph.projectConfig?.defaultLocation ?? undefined; | ||
| if ( | ||
| !isJsonOutput && | ||
| !argv.json && |
There was a problem hiding this comment.
In JSON mode this location-mismatch warning (and the --timeout hint above, and the timed-out/cancelled messages at the end) are dropped entirely. Since stdout carries the JSON, these could go to stderr instead — same approach you used for failed unit tests.
There was a problem hiding this comment.
Good point - those warnings already wrote to stderr, so not sure why they were skipped before.
| | `--job-prefix` | string | — | Adds an additional prefix to BigQuery job IDs, in the form `dataform-${jobPrefix}-`. | | ||
| | `--job-labels` | string | — | Comma-separated `key=value` labels to attach to BigQuery jobs, e.g. `'key1=val1,key2=val2'`. Also applied to unit tests with `--run-tests`. Overrides `defaultJobLabels`, action `jobLabels` and table `labels` that use the same key. | | ||
| | `--emit-lineage` | boolean | — | Emits OpenLineage `RunEvent`s to Knowledge Catalog Lineage for each executed action. Overrides the `lineage.enabled` setting in `workflow_settings.yaml` when specified. See the [Data Lineage guide](../docs/lineage.md). | | ||
| | `--execution-graph` | boolean | false | Only builds and outputs the execution graph, without submitting the jobs to BigQuery. Only supported together with `--json` — plain `run --execution-graph` (without `--json`) is rejected. |
There was a problem hiding this comment.
Nits: missing trailing | on this row; default should be `false` for consistency; and it'd read better placed next to --json/--dry-run rather than after --emit-lineage. Also the --json row above lost its trailing period.
| }, | ||
| ); | ||
|
|
||
| test("run --json executes actions and outputs RunResult", { timeout: 120000 }, async () => { |
There was a problem hiding this comment.
This performs a real (non-dry) run that creates example_table. Please double-check uniqueDataset is cleaned up after the suite so we don't leak datasets in the integration project.
There was a problem hiding this comment.
It deletes the datasets after running tests AND removes all the stale ones that were left undeleted for whatever reason before running tests, just like the other suites, so there should be no long term leak possible
| const aotAction = executedGraph.actions.find((a: any) => a.target.name === "aot_table"); | ||
| expect(!!aotAction).to.equal(true); | ||
| expect(aotAction.tasks[0].statement).to.include("SELECT 2 as id"); | ||
| expect(aotAction.tasks[0].compiledSql).to.include("SELECT 2 as id"); |
There was a problem hiding this comment.
This assertion change (statement -> compiledSql) illustrates the output schema change for run --dry-run --json (ExecutionGraph -> RunResult) — see the breaking-change comment in run_command.ts.
There was a problem hiding this comment.
Previously, passing --actions=aot_table pruned jit_table out of executionGraph.actions, which caused --dry-run --json to hit the early ExecutionGraph exit (statement) instead of returning RunResult (compiledSql). Also now it is consistent with other assertions in this suite (mixed JiT with AoT).
Problem
The behavior of --json option in the run command is inconsistent and does not match documentation.
Today --json can only be passed together with --dry-run, but (unless jit actions are present), this doesn't actually trigger a dry run, and only prints out the execution graph. The docs do not make it clear that --json effectively overrides --dry-run.
If jit actions are present, however, the dry run will be conducted, and the json representation of the run result will be printed (which is probably the more intutive behavior for --dry-run combined with --json). This is also not made clear in the docs.
Effectively, this means that the --json flag, instead of the expected "print the result of the command in json format" semantics (which would match the description in the docs: "Outputs the run result as JSON."), actually means "If no jit actions are present, print out the execution graph without running. Otherwise, run and print out the run results. Works only for dry runs".
Solution
Make the behavior of the --json option consistent with itself and the docs description by making it simply print out the run result as json, suppressing all other outputs, and allow it to be used with or without --dry-run. In order to still allow the user to obtain the execution graph if needed, introduce the --execution-graph option, which, when combined with --json will simply print out the execution graph and terminate (regardless of --dry-run settings and the presence of jit actions)