Skip to content

Navigation Menu

Sign in
Sign up

SNOW-2912540: translate create_temp_table=True to table_type in AST encoding (rebased onto main) - #4307

Open
sfc-gh-fpawlowski wants to merge 7 commits into
main from
create-temp-table-ast-fix-on-main
Open

SNOW-2912540: translate create_temp_table=True to table_type in AST encoding (rebased onto main) #4307
sfc-gh-fpawlowski wants to merge 7 commits into
main from
create-temp-table-ast-fix-on-main

Conversation

@sfc-gh-fpawlowski

@sfc-gh-fpawlowski sfc-gh-fpawlowski commented Aug 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Summary

Rebases the fix from #4296 onto main. #4296's head branch (worktree-create-temp-table-ast-fix) is based on the stale ud-local-test-scripts branch, which is 49 commits behind main — so it can't land there cleanly. This PR cherry-picks #4296's two commits (unchanged, same authorship) directly onto current main, where they apply without conflicts.

Same fix as #4296: stops emitting the deprecated create_temp_table parameter into the encoded AST sent to the server, without changing Snowpark's own public API — create_temp_table stays in the function signatures, still fires its deprecation warning, and still works exactly as before for callers. Only table_type (already resolved from create_temp_table when needed) is now recorded in the AST.

Changes:

  • dataframe_writer.py: Move create_temp_table deprecation coercion to before the AST emission block in save_as_table, so table_type is already resolved when WriteTable is encoded. Remove expr.create_temp_table emission.
  • session.py: Remove ast.create_temp_table = create_temp_table from the write_pandas AST block — the coercion already fires before AST emission there, so ast.table_type carries the correct value.
  • dataframe.py: Replace create_temp_table=True with table_type="temp" in the internal cache_result mock path, matching the real code path and avoiding a spurious deprecation warning from internal code.
  • ast.proto: Mark both create_temp_table fields as // Deprecated: use table_type instead. (fields retained for wire compatibility).
  • tests/ast/data/session_write_pandas.test: Remove create_temp_table: true from expected encoded AST and create_temp_table=True from expected unparser output.

Test plan

  • tests/ast/test_ast_driver.py::test_ast[session_write_pandas.test] passes
  • tests/ast/test_ast_driver.py::test_ast[DataFrame.write.test] passes
  • Manually verified on a local-testing session: save_as_table(..., create_temp_table=True) still logs the deprecation warning and creates a temp table
  • Manually verified cache_result() no longer emits the spurious deprecation warning

Supersedes #4296 for the purpose of landing on main.

Checklist

  • If adding any arguments to public Snowpark APIs or creating new public Snowpark APIs, I acknowledge that I have ensured my changes include AST support.
  • I acknowledge that I have ensured my changes to be thread-safe

Stack (via Graphite)

🤖 Generated with Claude Code

sfc-gh-fpawlowski marked this pull request as ready for review August 6, 2026 09:01

codecov-commenter commented Aug 6, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.94%. Comparing base (09fe78f) to head (9d2f1f5).

Additional details and impacted files 
@@ Coverage Diff @@
## main #4307 +/- ##
===========================================
- Coverage 95.25% 82.94% -12.31% 
===========================================
 Files 171 171 
 Lines 44771 44769 -2 
 Branches 7687 7687 
===========================================
- Hits 42645 37133 -5512 
- Misses 1339 5680 +4341 
- Partials 787 1956 +1169

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

sfc-gh-fpawlowski and others added 2 commits September 2, 2026 06:43 
...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>
cursor Bot force-pushed the create-temp-table-ast-fix-on-main branch from 4d80196 to 9d96c9c Compare September 2, 2026 06:45
cursoragent and others added 3 commits September 2, 2026 18:51 
...or test
seq1()/seq2() restart per parallel generator worker, so ordered results
repeat values (assert 0 < 0 on the timelimit case). Assert non-decreasing
order instead.
Co-authored-by: Filip Pawłowski <sfc-gh-fpawlowski@users.noreply.github.com>

sfc-gh-fpawlowski commented Sep 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Merge activity

  • Sep 3, 6:32 AM UTC: Graphite couldn't merge this PR because it failed for an unknown reason (You're not authorized to push to this branch).
  • Sep 3, 8:07 AM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Sep 3, 8:08 AM UTC: Graphite couldn't merge this PR because it failed for an unknown reason (You're not authorized to push to this branch).
  • Sep 3, 8:09 AM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Sep 3, 8:10 AM UTC: Graphite couldn't merge this PR because it failed for an unknown reason (You're not authorized to push to this branch).
  • Sep 4, 5:41 AM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Sep 4, 5:41 AM UTC: Graphite couldn't merge this PR because it failed for an unknown reason (You're not authorized to push to this branch).
  • Sep 7, 6:17 AM UTC: Graphite couldn't merge this PR because it failed for an unknown reason (You're not authorized to push to this branch).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sfc-gh-aling sfc-gh-aling sfc-gh-aling approved these changes
@sfc-gh-mayliu sfc-gh-mayliu Awaiting requested review from sfc-gh-mayliu sfc-gh-mayliu is a code owner automatically assigned from snowflakedb/snowpark-python-api-reviewers
@sfc-gh-jzeng sfc-gh-jzeng Awaiting requested review from sfc-gh-jzeng sfc-gh-jzeng is a code owner automatically assigned from snowflakedb/snowpark-python-api-reviewers

Assignees

No one assigned

Labels

NO-CHANGELOG-UPDATES This pull request does not need to update CHANGELOG.md

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /