Skip to content

test: use supported kernel session timezone - #529

Merged
vuanhphung merged 2 commits into
mainfrom
vu-phung/fix-kernel-timezone-e2e
Sep 23, 2026
Merged

vuanhphung merged 2 commits into
mainfrom
vu-phung/fix-kernel-timezone-e2e

Conversation

@vuanhphung

@vuanhphung vuanhphung commented Sep 23, 2026

Copy link
Copy Markdown
Collaborator

Use the supported TIMEZONE SQL session parameter in the kernel E2E test and assert current_timezone() reflects the configured value. databricks-sql-kernel#303 removed the kernel-side session configuration allowlist, exposing that the previous spark.sql.session.timeZone test key is unsupported by the server.

Validation: TypeScript build check, ESLint, Prettier, and diff checks.

Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: 1 Low

Test-only change swapping an unsupported spark.sql.session.timeZone conf for the supported TIMEZONE session parameter and asserting current_timezone() reflects it — a solid improvement that actually verifies forwarding. One low note: unlike the sibling execute tests, this one doesn't wrap execute/fetch in try/finally, so a failing assertion leaks the operation/session/client.

Comment thread tests/e2e/kernel/execution-e2e.test.ts Outdated
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No issues identified by the review bot.

@vuanhphung
vuanhphung added this pull request to the merge queue Sep 23, 2026
Merged via the queue into main with commit 938730e Sep 23, 2026
25 checks passed

This branch was successfully deployed

1 active deployment
azure-prod 94abe165 Deployed Sep 23, 2026 by vuanhphung via e2e-test (26) #1604
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants