Skip to content

Commit 2cd0562

Browse files
feat(volume): enable native SQL file transfers
Signed-off-by: Aakash Saravanan <aakash.saravanan@databricks.com>
1 parent 4394c11 commit 2cd0562

5 files changed

Lines changed: 122 additions & 157 deletions

File tree

‎KERNEL_REV‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
6f4b6d607a61bdde61dcda2cf82bc8e80fd32cb9
1+
d41cc3657748966d8b69a802bc40e8d019022ceb

‎src/databricks/sql/backend/kernel/_errors.py‎

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@
3939
from __future__ import annotations
4040

4141
import logging
42+
from typing import Union
4243

4344
from databricks.sql.exc import (
4445
DatabaseError,
@@ -115,7 +116,9 @@
115116
}
116117

117118

118-
def reraise_kernel_error(exc: "_kernel.KernelError") -> "Error":
119+
def reraise_kernel_error(
120+
exc: "_kernel.KernelError",
121+
) -> "Union[Error, FileNotFoundError]":
119122
"""Convert a ``databricks_sql_kernel.KernelError`` to a PEP 249
120123
exception with the kernel's structured attributes forwarded onto
121124
the new instance.
@@ -126,6 +129,9 @@ def reraise_kernel_error(exc: "_kernel.KernelError") -> "Error":
126129
"""
127130
code = getattr(exc, "code", "Unknown")
128131
cls = _CODE_TO_EXCEPTION.get(code, DatabaseError)
132+
file_transfer = getattr(exc, "error_code", None) == "FILE_TRANSFER_ERROR"
133+
if file_transfer:
134+
cls = ProgrammingError if code == "PermissionDenied" else OperationalError
129135

130136
# For ServerOperationError, reproduce the Thrift backend's
131137
# ``context`` dict so callers that read
@@ -141,7 +147,15 @@ def reraise_kernel_error(exc: "_kernel.KernelError") -> "Error":
141147
"operation-id": getattr(exc, "query_id", None),
142148
"diagnostic-info": getattr(exc, "diagnostic_info", None),
143149
}
144-
new = cls(getattr(exc, "message", str(exc)), context)
150+
message = getattr(exc, "message", str(exc))
151+
if (
152+
file_transfer
153+
and code == "NotFound"
154+
and getattr(exc, "http_status", None) is None
155+
):
156+
new = FileNotFoundError(message)
157+
else:
158+
new = cls(message, context)
145159

146160
for attr in (
147161
"code",
@@ -162,7 +176,9 @@ def reraise_kernel_error(exc: "_kernel.KernelError") -> "Error":
162176
return new
163177

164178

165-
def wrap_kernel_exception(what: str, exc: BaseException) -> "Error":
179+
def wrap_kernel_exception(
180+
what: str, exc: BaseException
181+
) -> "Union[Error, FileNotFoundError]":
166182
"""Map any exception from a PyO3 call site to a PEP 249 exception.
167183
168184
- ``KernelError`` → mapped class with structured attrs forwarded.

‎src/databricks/sql/backend/kernel/client.py‎

Lines changed: 4 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,6 @@
1616
1717
- ``query_tags`` on execute is not supported (kernel exposes
1818
``statement_conf`` but PyO3 doesn't surface it).
19-
- Volume PUT/GET (staging operations): kernel has no Volume API
20-
yet. Users on Thrift-only paths.
2119
"""
2220

2321
from __future__ import annotations
@@ -74,41 +72,6 @@
7472
# per-request skip-and-warn.
7573
_KERNEL_MANAGED_HEADERS = frozenset({"authorization", "x-databricks-org-id"})
7674

77-
# Leading verbs of SQL volume/staging statements. Detected by the
78-
# leading token (case-insensitive) so the kernel backend can fail loud
79-
# on staging ops it can't service — see ``execute_command``.
80-
_STAGING_VERBS = ("PUT", "GET", "REMOVE")
81-
82-
83-
def _strip_leading_sql_comments(sql: str) -> str:
84-
"""Strip leading whitespace and SQL comments (``-- …`` line and
85-
``/* … */`` block, possibly several) from ``sql``, returning the
86-
remainder.
87-
88-
Needed so staging detection sees the real leading verb: a
89-
comment-prefixed staging op (``-- upload\\nPUT …`` or
90-
``/* c */ PUT …``, common in ETL scripts) must still be classified
91-
as staging, or it would slip past the guard into the silent-no-op
92-
bug. Block comments do not nest in Databricks SQL, so a simple
93-
scan-to-``*/`` is correct.
94-
"""
95-
i = 0
96-
n = len(sql)
97-
while i < n:
98-
if sql[i].isspace():
99-
i += 1
100-
elif sql.startswith("--", i):
101-
# Line comment: skip to end of line (or string).
102-
nl = sql.find("\n", i)
103-
i = n if nl == -1 else nl + 1
104-
elif sql.startswith("/*", i):
105-
# Block comment: skip to closing */ (or end if unterminated).
106-
close = sql.find("*/", i + 2)
107-
i = n if close == -1 else close + 2
108-
else:
109-
break
110-
return sql[i:]
111-
11275

11376
def _is_not_found(exc: BaseException) -> bool:
11477
"""True iff ``exc`` is a kernel ``NotFound`` error (HTTP 404 /
@@ -145,21 +108,6 @@ def _catalog_or_none(value: Optional[str]) -> Optional[str]:
145108
return value
146109

147110

148-
def _is_staging_statement(operation: str) -> bool:
149-
"""True iff ``operation`` is a volume/staging statement (PUT / GET /
150-
REMOVE).
151-
152-
Strips leading whitespace + SQL comments first (so a comment-
153-
prefixed staging op is still caught), then matches the leading token
154-
only — so a normal query that merely *contains* the word (e.g.
155-
``SELECT 'GET' AS x``) isn't misflagged.
156-
"""
157-
stripped = _strip_leading_sql_comments(operation)
158-
# First whitespace-delimited token, uppercased.
159-
verb = stripped.split(None, 1)[0].upper() if stripped.strip() else ""
160-
return verb in _STAGING_VERBS
161-
162-
163111
def _kernel_session_accepts_kwarg(name: str) -> bool:
164112
"""True iff the installed ``databricks_sql_kernel.Session`` constructor
165113
declares keyword ``name``.
@@ -575,28 +523,14 @@ def execute_command(
575523
# ``_async_statements`` and closed by ``close_command``); the sync
576524
# path drops it in finally. ``close_stmt`` is the post-success
577525
# decision flag — it stays True on sync, flips to False on async.
578-
# Volume/staging (PUT/GET/REMOVE) is not supported on the kernel
579-
# path: the kernel returns the staging control row as a normal
580-
# result set (``KernelResultSet.is_staging_operation`` is always
581-
# False), so the connector's ``_handle_staging_operation`` never
582-
# fires and NO file is transferred. Rather than silently no-op
583-
# (the Thrift path performs the presigned-URL upload/download),
584-
# fail loud at the call site so ETL scripts don't ingest
585-
# stale/missing data. Detected by the leading SQL verb — the
586-
# only signal available pre-execute, since the kernel exposes no
587-
# staging marker today.
588-
if _is_staging_statement(operation):
589-
raise NotSupportedError(
590-
"Volume / staging operations (PUT / GET / REMOVE) are not "
591-
"supported on the kernel backend (use_kernel=True); the file "
592-
"transfer would silently not happen. Use the Thrift backend "
593-
"for staging operations."
594-
)
595-
596526
close_stmt = True
597527
try:
598528
try:
599529
stmt.set_sql(operation)
530+
allowed_paths = cursor.connection.staging_allowed_local_path
531+
if isinstance(allowed_paths, str):
532+
allowed_paths = [allowed_paths]
533+
stmt.set_staging_allowed_local_paths(allowed_paths or [])
600534
stmt.set_row_limit(row_limit)
601535
if query_tags:
602536
# Per-statement query tags. The kernel serialises the

‎tests/e2e/test_kernel_backend.py‎

Lines changed: 51 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
DatabaseError,
3333
NotSupportedError,
3434
OperationalError,
35+
ProgrammingError,
3536
ServerOperationError,
3637
)
3738

@@ -366,9 +367,7 @@ def test_schemas_with_empty_string_filter_matches_nothing(conn):
366367
assert cur.fetchall() == []
367368

368369

369-
@pytest.mark.parametrize(
370-
"empty_filter", ["schema_name", "table_name", "column_name"]
371-
)
370+
@pytest.mark.parametrize("empty_filter", ["schema_name", "table_name", "column_name"])
372371
def test_columns_with_empty_string_filter_matches_nothing(conn, empty_filter):
373372
filters = {
374373
"catalog_name": "system",
@@ -777,28 +776,55 @@ def test_scientific_notation_decimal_parameter_binds(conn):
777776
assert int(cur.fetchall()[0][0]) == 100
778777

779778

780-
# ── Staging / volume — fail loud, not silent no-op ────────────────
781-
782-
783-
def test_staging_put_raises_not_supported(conn):
784-
"""A PUT (volume/staging) statement fails loud on the kernel path
785-
rather than silently no-opping (which would make ETL ingest
786-
stale/missing data). (CUJ-gap audit)"""
787-
with conn.cursor() as cur:
788-
with pytest.raises(NotSupportedError, match="staging"):
789-
cur.execute("PUT '/tmp/x.csv' INTO '/Volumes/main/default/v/x.csv'")
790-
791-
792-
def test_comment_prefixed_staging_put_raises_not_supported(conn):
793-
"""A comment-prefixed staging op (common in ETL scripts) must also
794-
fail loud — the leading-verb detection strips SQL comments first, so
795-
it can't slip past into the silent-no-op bug (PR #825 review #1)."""
796-
with conn.cursor() as cur:
797-
with pytest.raises(NotSupportedError, match="staging"):
798-
cur.execute(
799-
"-- upload the daily extract\n"
800-
"PUT '/tmp/x.csv' INTO '/Volumes/main/default/v/x.csv'"
801-
)
779+
@pytest.mark.parametrize("async_op", [False, True])
780+
def test_volume_file_lifecycle(kernel_conn_params, tmp_path, catalog, async_op):
781+
allowed = tmp_path / "allowed"
782+
allowed.mkdir()
783+
source = allowed / "input.bin"
784+
destination = allowed / "output.bin"
785+
payload = bytes(range(256)) * 4096
786+
source.write_bytes(payload)
787+
denied = tmp_path / "denied.bin"
788+
denied.write_bytes(payload)
789+
catalog = catalog or "main"
790+
namespace = f"volume_{uuid4().hex}"
791+
qualified = f"`{catalog.replace('`', '``')}`.`{namespace}`"
792+
remote = f"/Volumes/{catalog}/{namespace}/files/input.bin"
793+
with sql.connect(
794+
**kernel_conn_params, staging_allowed_local_path=str(allowed)
795+
) as connection:
796+
with connection.cursor() as cur:
797+
cur.execute(f"CREATE SCHEMA {qualified}")
798+
try:
799+
cur.execute(f"CREATE VOLUME {qualified}.files")
800+
801+
def execute(query):
802+
if async_op:
803+
cur.execute_async(query)
804+
cur.get_async_execution_result()
805+
else:
806+
cur.execute(query)
807+
return cur.fetchall()
808+
809+
assert (
810+
execute(f"-- upload\nPUT '{source}' INTO '{remote}' OVERWRITE")
811+
== []
812+
)
813+
assert execute(f"GET '{remote}' TO '{destination}'") == []
814+
assert destination.read_bytes() == payload
815+
with pytest.raises(ProgrammingError):
816+
execute(f"PUT '{denied}' INTO '{remote}' OVERWRITE")
817+
with pytest.raises(FileNotFoundError):
818+
execute(
819+
f"PUT '{allowed / 'missing.bin'}' INTO '{remote}' OVERWRITE"
820+
)
821+
assert execute(f"REMOVE '{remote}'") == []
822+
assert execute(f"REMOVE '{remote}'") == []
823+
with pytest.raises(OperationalError, match="404"):
824+
execute(f"GET '{remote}' TO '{allowed / 'deleted.bin'}'")
825+
assert execute("SELECT 1")[0][0] == 1
826+
finally:
827+
cur.execute(f"DROP SCHEMA {qualified} CASCADE")
802828

803829

804830
# ── Error fidelity — diagnostic-info reaches .context ─────────────

‎tests/unit/test_kernel_client.py‎

Lines changed: 47 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -393,9 +393,7 @@ def fake_session(**kw):
393393

394394

395395
@pytest.mark.parametrize("enabled", [True, False])
396-
def test_open_session_passes_geospatial_representation_to_kernel(
397-
monkeypatch, enabled
398-
):
396+
def test_open_session_passes_geospatial_representation_to_kernel(monkeypatch, enabled):
399397
captured = {}
400398

401399
def fake_session(*, enable_geospatial_support=True, **kw):
@@ -807,70 +805,43 @@ def test_execute_command_forwards_row_limit(row_limit):
807805
stmt.set_row_limit.assert_called_once_with(row_limit)
808806

809807

810-
# ---------------------------------------------------------------------------
811-
# Staging / volume operations — fail loud (not silently no-op)
812-
# ---------------------------------------------------------------------------
813-
814-
815808
@pytest.mark.parametrize(
816-
"operation",
809+
"allowed, expected",
817810
[
818-
"PUT '/local/f.csv' INTO '/Volumes/c/s/v/f.csv'",
819-
" put '/local/f' into '/Volumes/...'", # leading ws + lowercase
820-
"GET '/Volumes/c/s/v/f' TO '/local/f'",
821-
"REMOVE '/Volumes/c/s/v/f'",
811+
(None, []),
812+
("/tmp/files", ["/tmp/files"]),
813+
(["/tmp/a", "/tmp/b"], ["/tmp/a", "/tmp/b"]),
822814
],
823815
)
824-
def test_staging_operation_raises_not_supported(operation):
825-
"""Volume/staging PUT/GET/REMOVE must FAIL LOUD on the kernel path
826-
(the kernel can't perform the presigned-URL transfer; silently
827-
no-opping would make ETL ingest stale/missing data)."""
816+
@pytest.mark.parametrize("async_op", [False, True])
817+
def test_execute_command_forwards_allowed_local_paths(allowed, expected, async_op):
828818
c = _make_client()
829819
c._kernel_session = MagicMock()
830820
cursor = MagicMock()
821+
cursor.connection.staging_allowed_local_path = allowed
831822
cursor.arraysize = 100
832823
cursor.buffer_size_bytes = 1024
833-
834-
with pytest.raises(NotSupportedError, match="staging"):
835-
c.execute_command(
836-
operation=operation,
837-
session_id=MagicMock(),
838-
max_rows=1,
839-
max_bytes=1,
840-
lz4_compression=False,
841-
cursor=cursor,
842-
use_cloud_fetch=False,
843-
parameters=[],
844-
async_op=False,
845-
enforce_embedded_schema_correctness=False,
846-
)
847-
848-
849-
@pytest.mark.parametrize(
850-
"operation, is_staging",
851-
[
852-
("PUT '/f' INTO '/v'", True),
853-
("get '/v' to '/f'", True),
854-
("REMOVE '/v'", True),
855-
# Comment-prefixed staging ops MUST still be caught — otherwise
856-
# they slip into the silent-no-op bug this guard exists to close
857-
# (regression: review #1 on PR #825). ETL scripts commonly
858-
# prefix statements with comments.
859-
("-- upload the file\nPUT '/f' INTO '/v'", True),
860-
("/* staging */ PUT '/f' INTO '/v'", True),
861-
("/* c1 */\n -- c2\n get '/v' to '/f'", True), # mixed, multiple
862-
(" \n\t PUT '/f' INTO '/v'", True), # leading whitespace only
863-
("SELECT 'GET' AS x", False), # word appears but not leading verb
864-
("SELECT * FROM puts", False),
865-
("-- PUT in a comment\nSELECT 1", False), # verb only in comment
866-
("/* PUT */ SELECT 1", False),
867-
("INSERT INTO t VALUES (1)", False),
868-
("", False),
869-
("-- just a comment", False), # comment only, no statement
870-
],
871-
)
872-
def test_is_staging_statement(operation, is_staging):
873-
assert kernel_client._is_staging_statement(operation) is is_staging
824+
stmt = c._kernel_session.statement.return_value
825+
executed = MagicMock(
826+
statement_id="00000000-0000-0000-0000-000000000001",
827+
arrow_schema=MagicMock(return_value=pa.schema([("x", pa.int64())])),
828+
)
829+
stmt.execute.return_value = executed
830+
stmt.submit.return_value = executed
831+
c.execute_command(
832+
operation="PUT '/tmp/files/input' INTO '/Volumes/c/s/v/input'",
833+
session_id=MagicMock(),
834+
max_rows=1,
835+
max_bytes=1,
836+
lz4_compression=False,
837+
cursor=cursor,
838+
use_cloud_fetch=False,
839+
parameters=[],
840+
async_op=async_op,
841+
enforce_embedded_schema_correctness=False,
842+
)
843+
stmt.set_staging_allowed_local_paths.assert_called_once_with(expected)
844+
(stmt.submit if async_op else stmt.execute).assert_called_once()
874845

875846

876847
# ---------------------------------------------------------------------------
@@ -2582,3 +2553,21 @@ def test_only_reserved_headers_omits_kwarg(self, monkeypatch):
25822553
monkeypatch, [("Authorization", "Bearer x"), ("x-databricks-org-id", "1")]
25832554
)
25842555
assert "http_headers" not in captured
2556+
2557+
2558+
@pytest.mark.parametrize(
2559+
"code, http_status, expected",
2560+
[
2561+
("PermissionDenied", None, ProgrammingError),
2562+
("NotFound", None, FileNotFoundError),
2563+
("NotFound", 404, OperationalError),
2564+
("NetworkError", None, OperationalError),
2565+
],
2566+
)
2567+
def test_file_transfer_error_matches_thrift_exception(code, http_status, expected):
2568+
error = _FakeKernelError(code=code, message="File transfer failed")
2569+
error.error_code = "FILE_TRANSFER_ERROR"
2570+
error.http_status = http_status
2571+
result = kernel_client._reraise_kernel_error(error)
2572+
assert type(result) is expected
2573+
assert result.error_code == "FILE_TRANSFER_ERROR"

0 commit comments

Comments
 (0)