ISSUE-2103 Handle trailing and trailing statement comments#2108
Open
spatel11 wants to merge 45 commits into
Open
ISSUE-2103 Handle trailing and trailing statement comments#2108spatel11 wants to merge 45 commits into
spatel11 wants to merge 45 commits into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Contributor
|
/gcbrun |
kolina
requested changes
Jun 27, 2026
kolina
left a comment
kolina
left a comment
Contributor
There was a problem hiding this comment.
A bunch of tests needs fixing https://gist.github.com/kolina/62e278293d983de308b0e3d5949101a2
* Bump vm2 from 3.10.2 to 3.11.3 Bumps [vm2](https://github.com/patriksimek/vm2) from 3.10.2 to 3.11.3. - [Release notes](https://github.com/patriksimek/vm2/releases) - [Changelog](https://github.com/patriksimek/vm2/blob/main/CHANGELOG.md) - [Commits](patriksimek/vm2@v3.10.2...v3.11.3) --- updated-dependencies: - dependency-name: vm2 dependency-version: 3.11.3 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> * Fix vm2 sandbox execution errors and CallSite path stripping The upgrade of vm2 to 3.11.3 tightened sandbox security, stripping file path information from V8 CallSite objects during Error.prepareStackTrace and breaking the resolution of symlinked modules. This caused critical test failures ("Unable to find valid caller file") during Dataform compilation. This commit resolves these issues by: * Resolving `projectDir` to its absolute realpath before sandbox compilation to circumvent symlink boundaries (especially important for bazel runfiles). * Injecting `global.__dataform_current_file` into the NodeVM compiler configuration with a try-finally block to track cross-boundary macro scopes. * Adding a fallback in `core/utils.ts:getCallerFile` to read the injected global file path when the V8 stack trace path is stripped or undefined. --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Rafal Hawrylak <hawrylak@google.com>
* Support onSchemaChange for incremental tables This change introduces support for the onSchemaChange option in incremental tables for the BigQuery adapter. It adds the incrementalSchemaChangeBody() strategy to handle schema changes. End-to-end tests have been created to verify the new functionality. * fix: Private functions moved after public functions * - Split onSchemaChange queries to sub-tasks - Split sql queries to sun-functions - Fixed function names * fix: Address feedback for PR dataform-co#2101 and fix presubmit failures - Updated e2e tests - Added golden files - Consolidated merge and insert functions
…#2170) - Add `jit_code` field to the `Assertion` message in `protos/core.proto` - Fix missing protobuf imports and configuration in `protos/BUILD` - Introduce `JitAssertionResult` type alias and `jitCode` method to `Assertion` builder (`core/actions/assertion.ts`) - Add strictly typed `jitContextable` parameter to `Session.sqlxAction` in `core/session.ts` - Update assertion `compile()` to handle `jitCode` execution bypassing standard string evaluation - Parse and apply `jitContextable` safely for assertions in `core/session.ts` - Add unit test coverage in `core/main_test.ts` to verify compilation output
- extend onSchemaChange e2e test timeout (dataform-co#2175)
Adds JIT_COMPILATION_TARGET_TYPE_ASSERTION alongside the existing
TABLE / OPERATION / INCREMENTAL_TABLE target types, plus a matching
JitAssertionResult { string query } proto and a jitCompileAssertion()
dispatcher in core/jit_compiler.ts. Assertions reuse SqlActionJitContext
since AssertionContext implements IActionContext, and the result is a
single SELECT query string (matching the existing
JitAssertionResult = string alias in core/actions/assertion.ts).
… match (dataform-co#2191) vm2 3.11.3 strips file paths from V8 CallSite objects inside the sandbox, so `getCallerFile()` in @dataform/core can no longer resolve the current file from the stack. Track it via a host-side stack exposed through `__df_enter`/`__df_exit`/`__df_current` sandbox helpers and read it through a getter on `global.__dataform_current_file`. For @dataform/core <= 3.0.56 (no global fallback in getCallerFile), patch the bundle text at load time to consult the global. Gated on the installed Core version so newer bundles are never touched. Add a CLI/Core compatibility check: reject installed Core whose major differs from the CLI's, or whose minor is below the CLI's minor. Resolve the installed Core's version by running `require("@dataform/core").version` inside the index-generator vm so any install layout (package.json, workflow_settings.yaml stateless install, JiT) reports the version actually loaded. The error tells the user the exact `dataformCoreVersion:` line to set in `workflow_settings.yaml`. Skipped when the CLI version can't be determined (unbundled local dev). Mirror the same wrapper pattern in `testing/run_core.ts`. Add `compile rejects @dataform/core with incompatible version` test in `cli/index_compile_test.ts` that drives the workflow_settings.yaml stateless install path with dataformCoreVersion: "2.9.0" (latest 2.x on the registry) to exercise the real `npm i` install flow. Add `compile succeeds with @dataform/core <= 3.0.56 via caller-file shim` test that installs @dataform/core@3.0.50 and asserts the compiled action's `fileName` matches the source path, end-to-end proving the bundle-text patch and host-side file stack restore getCallerFile under vm2 3.11.3.
dataform-co#2192) Default compilation timeout was 30s. Bump to 60s so larger projects don't hit it on a single compile pass. Extends the "Compilation timed out" error to point at --timeout with concrete examples (--timeout=2m, --timeout=1h) so users can self-mitigate without reading docs.
Bumps [vm2](https://github.com/patriksimek/vm2) from 3.11.3 to 3.11.4. - [Release notes](https://github.com/patriksimek/vm2/releases) - [Changelog](https://github.com/patriksimek/vm2/blob/main/CHANGELOG.md) - [Commits](patriksimek/vm2@v3.11.3...v3.11.4) --- updated-dependencies: - dependency-name: vm2 dependency-version: 3.11.4 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…#2181) * Extract BigQueryClientProvider for injectable BQ clients * Address review: collapse BigQueryClientProvider to a function type
Bumps [protobufjs](https://github.com/protobufjs/protobuf.js) from 7.5.5 to 7.5.8. - [Release notes](https://github.com/protobufjs/protobuf.js/releases) - [Changelog](https://github.com/protobufjs/protobuf.js/blob/protobufjs-v7.5.8/CHANGELOG.md) - [Commits](protobufjs/protobuf.js@protobufjs-v7.5.5...protobufjs-v7.5.8) --- updated-dependencies: - dependency-name: protobufjs dependency-version: 7.5.8 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Migrate to bazel 7.3.2 * Use fixed ref for github action
KMS has an org-policy enabled, requiring the usage of client certificate aware endpoints when performing the decryption of credentials. Updating gcloud, so that it uses an updated compatible endpoint.
Mirrors TableConfig.metadata / ViewConfig.metadata so callers can attach key/value data that lands on Assertion.action_descriptor.metadata.extra_properties.
…orm-co#2199) The flag parser in common/flags/index.ts runs in a static initialiser at module load and parses process.argv. Once it had seen a --flag, any later token that was neither another flag nor a flag value caused it to throw "Arg neither flag name nor flag value", crashing the entire CLI before yargs ran. yargs accepts positional arguments after flags (for example `dataform run --some-flag value my_project`), so this lightweight parser was rejecting valid argument orderings. Ignore any token that is neither a flag nor a flag value instead of throwing, exactly as already happened for positional arguments before the first flag; yargs remains responsible for parsing positionals. Extract the parsing loop into an exported parseArgv function so it can be unit-tested in isolation, and add common/flags/index_test.ts covering the regression and related orderings. Fixes dataform-co#2198.
* Bump protobufjs-cli from 1.0.0 to 1.3.3 Bumps [protobufjs-cli](https://github.com/protobufjs/protobuf.js) from 1.0.0 to 1.3.3. - [Release notes](https://github.com/protobufjs/protobuf.js/releases) - [Changelog](https://github.com/protobufjs/protobuf.js/blob/master/CHANGELOG.md) - [Commits](protobufjs/protobuf.js@protobufjs-cli-v1.0.0...protobufjs-cli-v1.3.3) --- updated-dependencies: - dependency-name: protobufjs-cli dependency-version: 1.3.3 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> * fix(tools): patch generated proto typings to use ES6 import for 'long' This avoids syntax errors in older rollup-plugin-dts which does not support 'import Long = require("long")' syntax. We patch the generated ts.d.ts file to use 'import Long from "long"' instead. --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Nick Nalivaika <ikolina@google.com>
Bumps [js-yaml](https://github.com/nodeca/js-yaml) from 4.1.1 to 4.2.0. - [Changelog](https://github.com/nodeca/js-yaml/blob/master/CHANGELOG.md) - [Commits](https://github.com/nodeca/js-yaml/commits) --- updated-dependencies: - dependency-name: js-yaml dependency-version: 4.2.0 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…e descriptions pr fix (dataform-co#2164) * initial attempt at md reader * adding relevant tests and better path resolving for md * cleaner get contents path handling * escaping newlines for md * adding documentation * add check if file in rootdir for get contents and update documentation on getContents to include limits * cleaner and filesystem agnostic check if getContents target is in the root dir * making sure root dir has seperator on check if getcontents file is in dir * removing node module usage * removing redundant normalize and root variable * adding seperator when checking if file is in rootDir
Bumps [protobufjs](https://github.com/protobufjs/protobuf.js) from 7.5.8 to 7.6.3. - [Release notes](https://github.com/protobufjs/protobuf.js/releases) - [Changelog](https://github.com/protobufjs/protobuf.js/blob/protobufjs-v7.6.3/CHANGELOG.md) - [Commits](protobufjs/protobuf.js@protobufjs-v7.5.8...protobufjs-v7.6.3) --- updated-dependencies: - dependency-name: protobufjs dependency-version: 7.6.3 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…s and tests (dataform-co#2213) - Enable `use_default_shell_env` in `gcloud_secret` rule to pass host environment variables (like `CLOUDSDK_CONTEXT_AWARE_USE_CLIENT_CERTIFICATE`) to the gcloud tool. - Pass `CLOUDSDK_CONTEXT_AWARE_USE_CLIENT_CERTIFICATE` to bazel test in `scripts/publish`.
…orm-co#2189) Putting a .sqlx file under the ``filename`` field of an entry in ``definitions/actions.yaml`` used to fall through to ``nativeRequire``, which then failed with a cryptic error far from the source of the mistake. ``.sqlx`` files are loaded directly from the ``definitions/`` directory by the sqlx compiler, not referenced through ``actions.yaml``. Add an explicit validation step in ``loadActionConfigs`` that runs before each action is constructed. If the action's ``filename`` ends in ``.sqlx`` (case-insensitive), emit a ``compileError`` naming the action type, the offending filename, and pointing at the fix. The action is then skipped so we don't double-up with a downstream ``nativeRequire`` error. The check covers every action type that accepts a ``filename``: ``table``, ``view``, ``incrementalTable``, ``assertion``, ``operation``, ``declaration``, ``notebook`` and ``dataPreparation``. Data preparations are included because a ``.dp.sqlx`` file is also auto-compiled from ``definitions/`` - referencing one from ``actions.yaml`` doesn't resolve its path, ignores its ``config {}`` block and double-registers the action, so it's the same mistake. ``.dp.yaml`` data preparations are unaffected. Closes dataform-co#1785
Bumps [tmp](https://github.com/raszi/node-tmp) from 0.2.4 to 0.2.7. - [Changelog](https://github.com/raszi/node-tmp/blob/master/CHANGELOG.md) - [Commits](raszi/node-tmp@v0.2.4...v0.2.7) --- updated-dependencies: - dependency-name: tmp dependency-version: 0.2.6 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [@tootallnate/once](https://github.com/TooTallNate/once) from 2.0.0 to 2.0.1. - [Release notes](https://github.com/TooTallNate/once/releases) - [Changelog](https://github.com/TooTallNate/once/blob/v2.0.1/CHANGELOG.md) - [Commits](TooTallNate/once@2.0.0...v2.0.1) --- updated-dependencies: - dependency-name: "@tootallnate/once" dependency-version: 2.0.1 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [fast-uri](https://github.com/fastify/fast-uri) from 3.0.6 to 3.1.2. - [Release notes](https://github.com/fastify/fast-uri/releases) - [Commits](fastify/fast-uri@v3.0.6...v3.1.2) --- updated-dependencies: - dependency-name: fast-uri dependency-version: 3.1.2 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
spatel11
commented
Jul 8, 2026
Author
I believe these should be fixed. I'm running
|
Contributor
|
/gcbrun |
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
7 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.
This PR fixes an issue with trailing comments in (at least) pre_operations blocks. It adds tests for the existing task.concatenateQueries function, and some example definitions in the integration tests.
Should fix: #2103
While preserving fix for: #1231