-
Notifications
You must be signed in to change notification settings - Fork 156
Switch UD reporting to 5-group test matrix, fix concurrency + verification - #4329
Switch UD reporting to 5-group test matrix, fix concurrency + verification #4329sfc-gh-fpawlowski wants to merge 36 commits into
Conversation
...ncoding The deprecated `create_temp_table` parameter was being emitted to the proto AST as a separate boolean field even though the runtime already translates it to `table_type="temporary"`. This meant the AST decoder had to handle two representations for the same thing. Fix: move the deprecation coercion before the AST block in save_as_table so `table_type` is already resolved when emitted; remove the deprecated field from both AST emission sites (WriteTable and WritePandas); update the internal cache_result mock path to pass table_type="temp" directly; mark the proto fields as deprecated. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
..._table removal Remove create_temp_table from the expected encoded AST and unparser output in the write_pandas golden test — the field is no longer emitted to the proto since the deprecation coercion now happens before the AST block, making table_type the sole carrier of this information. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
...ncoding The deprecated `create_temp_table` parameter was being emitted to the proto AST as a separate boolean field even though the runtime already translates it to `table_type="temporary"`. This meant the AST decoder had to handle two representations for the same thing. Fix: move the deprecation coercion before the AST block in save_as_table so `table_type` is already resolved when emitted; remove the deprecated field from both AST emission sites (WriteTable and WritePandas); update the internal cache_result mock path to pass table_type="temp" directly; mark the proto fields as deprecated. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
These constants were previously imported from snowflake-connector-python. The Universal Driver connector no longer owns them (it has no internal use for either symbol), so define them locally to remove the coupling. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The Universal Driver connector's constants module won't provide ENV_VAR_PARTNER either (per review discussion on snowflake-eng/universal-driver#512), so define it locally in server_connection.py alongside the other now-local backward compatibility constants (ASYNC_RETRY_PATTERN, compat.OK). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
...into analyzer_utils Removes the dependency on these private snowflake-connector-python functions (previously imported from connector.pandas_tools). Ports the implementation from universal-driver PR #518 directly into Snowpark so the connector no longer needs to expose staging helpers as part of its public surface. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
...ned pandas staging helpers black never ran on the ported code (4 blocks needed line-wrapping). The two cursor.execute(..., _force_qmark_paramstyle=True) calls also fail pyright: SnowflakeCursor.execute's two @overload stubs in snowflake-connector-python never declare _force_qmark_paramstyle, only the real implementation signature does. That gap was invisible while this code lived in the connector package; porting it into analyzer_utils.py exposes it to Snowpark's own pyright run.
...tings _stage_sql (inlined from universal-driver PR #518 in the prior commit) faithfully maps compression="gzip" to COMPRESSION=auto in the generated CREATE...STAGE...FILE_FORMAT SQL, same as it already does for the COPY INTO clause (see copy_compression in this test). The stage-creation assertion still expected the literal "gzip", unlike its COPY INTO counterpart a few lines below which already accounted for the auto mapping via copy_compression. Update it to match.
...lemetry_oob The UD's telemetry_oob.TelemetryService is a no-op stub (BD#45) that lacks batch_size, causing an AttributeError at runtime. Make LocalTestOOBTelemetryService standalone: add its own singleton, queue, batch_size, _enabled, and close(). Also fixes the pre-existing _enable typo (should have been _enabled). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The decoupling from connector.telemetry_oob ported add()/flush()/enable()/ disable() but missed size() (connector/telemetry_oob.py:542, `return self.queue.qsize()`), which several tests (tests/mock/test_oob_telemetry.py, tests/mock/test_multithreading.py) call directly on the service instance.
mock/_telemetry.py imported snowflake.connector.secret_detector.SecretDetector unconditionally -- the only reason UD PR #598 ports SecretDetector into the Universal Driver at all is to satisfy this one Snowpark import (SecretDetector is not otherwise used or maintained by UD). Per BehaviorDifferences.yaml #45, OOB telemetry is disabled on the backend anyway, so this masking is defense-in-depth on the outbound payload, not a response to active exploitation -- but export_queue_to_string() feeds a real requests.Session().post() to a live analytics endpoint, so it's not a safe no-op either. Ports only what _telemetry.py actually calls (mask_secrets -> masked string), from UD's actual _common/secret_detector.py source (PR #598) rather than the older, less complete real v4 connector's version -- verified UD's copy has 3 additional maskers (OAuth tokens, OAuth client secrets, passcodes) plus a PASSWORD_PATTERN false-positive fix and a wire-format fix to CONNECTION_TOKEN_PATTERN, all needed for correctness. Drops everything that exists solely for UD's backward-compat contract: @backward_compatibility, MaskedMessageData's 3-tuple return (only masked_text is ever read), logging.Formatter inheritance, and the classmethod-vs-staticmethod workaround. Fixes the exception-handling branch rather than porting it straight: the legacy/UD code does masked_text = str(ex) on failure, which can leak exception-reflected input. Returns a static sentinel instead -- this was already flagged as a real finding by the security bot on UD #598. After this, mock/_telemetry.py has zero remaining imports from snowflake.connector. Test coverage: ported every behavioral test from UD PR #598's test_secret_detector.py TestMaskSecrets class (27 cases via pytest count), adapted from the 3-tuple/class-method API to the plain-function API here, plus one exception-handling test adapted to assert the static sentinel instead of the leaked exception text. Dropped TestFormatter and the logging.Formatter-specific exception tests -- not applicable, since mask_secrets is a plain function here, not a logging.Formatter subclass.
Corrects the previous commit: this is now an exact copy of UD's _common/secret_detector.py (drivers#598), not a collapsed rewrite. Keeps the SecretDetector class, MaskedMessageData, the (is_masked, masked_text, err_str) 3-tuple return, classmethods, logging.Formatter inheritance, format(), create_formatting_error_log(), and masked_text = str(ex) on failure -- unchanged, even though the str(ex) behavior is a known real finding (flagged by the security bot on UD #598); fixing it is a separate call for whoever reviews this, not bundled into a move. Only removed what's physically impossible to keep: @backward_compatibility, install_backward_compatibility_getattr and its import, and the trailing install_backward_compatibility_getattr(__name__) call -- none of that mechanism exists in snowpark-python. The mask_secrets() docstring explaining why the maskers are classmethods (to survive @backward_compatibility stashing the class out of module globals) is left as-is even though that rationale no longer applies here -- not silently deleted or rewritten as part of this move. _telemetry.py's call site goes back to unpacking the 3-tuple unmodified; test_secret_detector.py is now a full copy of UD's test suite (TestMaskSecrets, TestMaskSecretsExceptionHandling, TestFormatter), not just the masking-behavior subset -- format()/logging.Formatter are kept, so their tests are too.
...ctor imports connector_version is already imported in utils.py; this one-liner exposes a boolean flag so callers can gate imports or behavior that differs between the legacy connector (v3/v4) and the Universal Driver (v5+). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
...; drop connector.options imports snowflake.connector.options is a backward-compat shim in v5 (UD) that will eventually be removed. Define MissingOptionalDependency, MissingPandas, MissingPyarrow, ModuleLikeObject, pandas, pyarrow, installed_pandas, and installed_pyarrow directly in _internal/utils.py and redirect all thirteen source-file imports there. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The v5 (Universal Driver) public snowflake.connector.telemetry shim does not always expose TelemetryData.TRUE/.FALSE (present on _internal.telemetry in some UD builds), which made every telemetry-sending Snowpark test raise AttributeError: type object 'TelemetryData' has no attribute 'FALSE'. Gate the TelemetryData import on IS_V5_DRIVER: on v5 prefer _internal.telemetry and fall back to the public shim; on v4 keep the legacy public import. Co-authored-by: Cursor <cursoragent@cursor.com>
...edefinition; re-source from connector 04c82d2 assumed connector.options had a gap and redefined MissingOptionalDependency, MissingPandas, MissingPyarrow, ModuleLikeObject, pyarrow, and installed_pyarrow locally. connector.options already provides all of these except installed_pyarrow (verified against the actually-installed v4.7.2 connector, which only exposes pyarrow itself and couples its availability to pandas's import tuple, not a standalone name). Re-import the four names that do exist there, gated on IS_V5_DRIVER so this keeps working once UD's connector._common.extras lands, and derive installed_pyarrow locally via isinstance instead of maintaining an independent, unprecedented MissingPyarrow resolution. pandas/installed_pandas (produced by the pre-existing _pandas_importer(), unrelated to 04c82d2) are intentionally left untouched -- unifying those is a separate follow-up.
...rent shape UD PR #1151 was updated since cb9f590 landed: MissingPandas is deleted outright from _common/extras.py (BehaviorDifferences.yaml #66), not kept as a deprecated re-export. Its suggested replacement, MissingOptionalDependency("pandas"), only works on v5 -- the real v4 connector's MissingOptionalDependency defines no __init__ override, so it only supports the no-arg-subclass pattern. Import MissingPandas only where it's real (v4). Add _missing_pandas() to build the sentinel with whichever construction style the active driver generation supports, centralizing the branch in _internal/utils.py rather than spreading IS_V5_DRIVER awareness to callers. Fix _pandas_importer() and mock/_options.py, both of which referenced MissingPandas directly and would otherwise NameError/ ImportError under IS_V5_DRIVER=True. installed_pyarrow also moves to a direct v5 import: #1151 fixed it to check pyarrow independently instead of mirroring installed_pandas, so it's now correct to import there instead of re-deriving locally. v4 still doesn't export it at all, so the local isinstance derivation stays there.
...n v5 The previous try/except hedged between two locations, neither of which actually has TelemetryData.TRUE/.FALSE on the current UD main: the public snowflake.connector.telemetry shim's TelemetryData has no TRUE/FALSE at all, and _internal.telemetry currently has no TelemetryData class either. The fallback branch was silently reachable and silently wrong. UD PR #1106 (open, not draft) adds TelemetryData/TelemetryField to _internal/telemetry.py specifically to match Snowpark's exact usage (PCTelemetryData(message=..., timestamp=...), .TRUE/.FALSE) -- confirmed by reading its actual diff. Import from there unconditionally on v5, no try/except: both branches now import from one definite, verified location.
...as from connector _pandas_importer() predates this whole effort and duplicated resolution the connector already does correctly on both driver generations -- including the "relative imports without dots" DataFrame workaround, now folded into UD's own _common.extras.pandas (confirmed on the not-yet-merged UD PR #1151/#1152; v4's options.py already had it). Add pandas/installed_pandas to the existing IS_V5_DRIVER-gated import block and delete the local resolution entirely. Verified the workaround isn't needed on Snowpark's side by running the exact invocation style its comment called out (pytest with tests/unit/ as cwd) -- no failure, consistent with both driver generations now handling it internally.
....extras mock/_options.py's MissingNumpy/numpy try-except was functionally identical to _common/extras.py's own numpy resolution (confirmed: pure duplicate, no fix to merge, per UD PR #1152's investigation). Import numpy from _common.extras on v5; v4 keeps its own MissingNumpy class since v4's options.py has no numpy handling to delegate to. Does not touch the pandas try/except in this file -- Local Testing deliberately never resolves pyarrow, unlike every other pandas-resolution path in this codebase (commit #1628).
Removing the function left only one blank line before class TempObjectType; black requires two before a top-level class definition.
...block - F401: pandas is imported purely for other modules to re-import from here, so it's never referenced elsewhere in this file. Split into its own import with a noqa, rather than noqa-ing a name inside a multi-line parenthesized import (which flake8 attributes to the opening line, not the name's own line). - E402: the IS_V5_DRIVER conditional-import block and _missing_pandas() ended up sitting between the top-of-file imports and two later ones (Row, VERSION). Moved those two imports up to stay contiguous.
...ield imports on IS_V5_DRIVER network.py doesn't exist in UD at all -- legacy's errors.py/network.py split is consolidated into errors.py. UD PR #1224 (open, stacked on #1133) adds ReauthenticationRequest(ProgrammingError) to errors.py and removes network.py outright, naming Snowpark's import site explicitly as the target. Gate the import in server_connection.py and its unit test mock, same pattern as every other IS_V5_DRIVER import in this stack. TelemetryClient/TelemetryField were imported unconditionally from the top-level snowflake.connector.telemetry module, which is a stub on v5 (the real implementation lives in _common.telemetry per UD PR #1106's current branch). Also fixes this same file's PCTelemetryData import, added in an earlier commit against _internal.telemetry -- that class moved to _common too on the same #1106 branch since that commit landed, so it was already stale for the identical reason. Adds test coverage for _missing_pandas() (added earlier in this stack to replace direct MissingPandas() construction), which had zero coverage after test__pandas_importer() was deleted alongside _pandas_importer() itself.
... import from snowflake.connector.options import installed_pandas is unconditional in tests/integ/test_function.py, test_cte.py, scala/test_datatype_suite.py, and scala/test_update_delete_merge_suite.py -- ModuleNotFoundError once UD deletes options.py outright (confirmed: f61156c7a, ancestor of SNOW-2912540-extras-to-common's current tip, already relied on elsewhere in this stack). Swap to snowflake.snowpark._internal.utils, which already re-exports installed_pandas correctly gated on IS_V5_DRIVER internally (from this PR's earlier _pandas_importer()-removal commit) -- no IS_V5_DRIVER awareness needed in these test files themselves.
...s.py _internal/utils.py's IS_V5_DRIVER-gated block (MissingOptionalDependency, ModuleLikeObject, pandas, pyarrow, installed_pandas, installed_pyarrow, _missing_pandas()) was a self-contained concern mirroring the connector's own options.py/_common.extras 1:1, buried in an already-large kitchen-sink file. Moved to a dedicated module, mirroring mock/_options.py's existing role as the scoped equivalent for the local-testing side. options.py computes its own IS_V5_DRIVER independently rather than importing it from utils.py, since utils.py itself needs names back from options.py (MissingOptionalDependency/ModuleLikeObject/installed_pandas, used by its modin-optional-dependency code) -- importing in both directions would be circular. One-line duplication, avoids import-order fragility entirely. Updated all 17 consumers (found types.py's multi-line import via a regex-based sweep after a naive single-line grep missed it) to import these names from _internal.options instead. mock/_options.py and event_table_telemetry.py keep their other _internal.utils imports (IS_V5_DRIVER, parse_table_name) unchanged.
The UD's SnowflakeCursor exposes the client-generated request UUID as a public `request_id` property; `_request_id` is only a backward-compat alias slated for removal. The legacy v4 connector is the opposite: it only ever sets `self._request_id` as a plain instance attribute and has no public `request_id` property. Branch on IS_V5_DRIVER in the one call site that reads this (execute_and_notify_query_listener) and its unit test mock. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Blocks pre-commit's insert-py-license hook (and therefore "Check linting", which gates the entire test matrix) for every branch stacked on top of this one.
...Error.raw_msg Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Snowflake Security Review
Security grade: A — Passed ✅
This PR was classified as LOW risk by the automated pre-screen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Snowflake Security Review
Security grade: A — Passed ✅
This PR was classified as LOW risk by the automated pre-screen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Snowflake Security Review
Security grade: A — Passed ✅
This PR was classified as LOW risk by the automated pre-screen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Snowflake Security Review
Security grade: A — Passed ✅
This PR was classified as LOW risk by the automated pre-screen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Snowflake Security Review
Security grade: A — Passed ✅
This PR was classified as LOW risk by the automated pre-screen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Snowflake Security Review
Security grade: A — Passed ✅
This PR was classified as LOW risk by the automated pre-screen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Snowflake Security Review
Security grade: A — Passed ✅
This PR was classified as LOW risk by the automated pre-screen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Snowflake Security Review
Security grade: A — Passed ✅
This PR was classified as LOW risk by the automated pre-screen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Snowflake Security Review
Security grade: A — Passed ✅
This PR was classified as LOW risk by the automated pre-screen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Snowflake Security Review
Security grade: A — Passed ✅
This PR was classified as LOW risk by the automated pre-screen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Snowflake Security Review
Security grade: A — Passed ✅
This PR was classified as LOW risk by the automated pre-screen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Snowflake Security Review
Security grade: A — Passed ✅
This PR was classified as LOW risk by the automated pre-screen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Snowflake Security Review
Security grade: A — Passed ✅
This PR was classified as LOW risk by the automated pre-screen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Snowflake Security Review
Security grade: A — Passed ✅
This PR was classified as LOW risk by the automated pre-screen.
3b135a2 to
1fddef5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Snowflake Security Review
Security grade: A — Passed ✅
This PR was classified as LOW risk by the automated pre-screen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Snowflake Security Review
Security grade: A — Passed ✅
This PR was classified as LOW risk by the automated pre-screen.
Previous 5-group attempt on this branch accumulated a lot of ad-hoc debugging hacks (checkout-target flip-flopping, dead-token additions, manual git-fetch detours) while chasing what turned out to be a red herring. Reverting those left the branch back at its original 2-group form -- losing the intended 5-group scope in the process. This rebuilds cleanly from ud-ci-workflow-auto-triggered-full-improvements (unit-integ/scala/modin/datasource/doctest), using the already-proven patch from snowpark-ud-job-testing (scripts/patch_new_workflow_for_verification.py + verify_ud_snippet.py) for the install-hardening + UD-verification logic, instead of reinventing it. repository: snowflakedb/universal-driver is left untouched from the source -- this is the repo snowpark-ud-job-testing has reliably dispatched against for weeks (see its JOBS.md history), unlike snowflakedb/drivers which had persistent, never-fully-root-caused actions/checkout fetch failures earlier this session. Only change beyond the source: dropped the concurrency block (confirmed none of odbc-reports' other UD-dispatch sibling jobs use one either -- see commit history for the concurrency-collision risk this avoids).
1fddef5 to
8a0ba9d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Snowflake Security Review
Security grade: A — Passed ✅
This PR was classified as LOW risk by the automated pre-screen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Snowflake Security Review
Security grade: A — Passed ✅
This PR was classified as LOW risk by the automated pre-screen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing quotes around ${snowflake_path} variable. This will cause word splitting and glob expansion if the path contains spaces or special characters.
# Current (broken): .tox/"$TOX"/bin/pip install --force-reinstall --no-deps ${snowflake_path}/snowflake_connector_python*.whl # Fixed: .tox/"$TOX"/bin/pip install --force-reinstall --no-deps "${snowflake_path}"/snowflake_connector_python*.whl
This is inconsistent with line 221 which correctly quotes "${ud_connector_path}".
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
0c1595c to
4475ec9
Compare
What
Replaces this branch's 2-group UD test matrix (
integration/scala, ported fromud-ci-workflow-auto-triggered-full) with the 5-group architecture (unit-integ/scala/modin/datasource/doctest, ported fromud-ci-workflow-auto-triggered-full-improvements): UD is built once via a dedicatedbuild-udjob (wheel fromsnowflakedb/universal-driver) and consumed as a downloaded artifact, instead of each test job pip-installing a git ref inline.Two deliberate deviations from
ud-ci-workflow-auto-triggered-full-improvementsConcurrency block removed. The source branch's
concurrency: group: ud-tests-${{ github.ref }}, cancel-in-progress: truekeys only on the git ref hosting the workflow file, not on the UD ref under test. odbc-reports dispatches this same ref repeatedly, concurrently, with a differentud-refper historical snapshot date — that keying would cancel one in-flight backfill run whenever another started, and a cancelled run reports as a permanenterrors: 1rather than a retriable missing entry. Confirmed none of odbc-reports' other four UD-dispatch sibling jobs (snowflake-cli, snowflake-sqlalchemy, dbt-adapters, airflow) use a concurrency block either, so this matches the existing convention rather than inventing a one-off.Fail-fast UD-install verification redesigned to poison output, not just exit. This job has
continue-on-error: trueat the job level plus a separateif: always()"Extract results" step — a bareexit 1wouldn't stop a silently-installed legacy connector's misleading pass/fail counts from being scraped and reported as real UD results. The verification step now overwritesreports/test-output.logwith1 errorson failure so the downstream extraction reflects the truth.scripts/tox_install_cmd.shneeded no changes (already hardened identically on both source branches:set -o pipefail,--no-depson the UD reinstall).tox.inicopied verbatim fromud-ci-workflow-auto-triggered-full-improvements(already passes throughud_connector_path/UD_RERUN_FLAGS/JUNIT_REPORT_DIR).Scope
No
src/ortests/changes — workflow/CI files only.Context
This is a long-lived CI-reference branch (same lifecycle as
ud-ci-workflow-auto-triggered-full-improvements), not intended to merge intomain— it's whatsnowflake-eng/odbc-reports'update-python-tests-cache.ymldispatches against to track Snowpark-on-UD pass rate over time. This PR is for reviewability of the diff.