[vuanhphung]

What type of PR is this?

• Other (performance)

Description

databricks.sql.client and databricks.sql.result_set import pandas at module load, and connect() imports client. So the first connect() in a process spends about 165 ms importing pandas (the whole client import takes ~360 ms) while holding the import lock, and every other thread connecting at that moment waits for it. pandas is only used in ResultSet._convert_arrow_table, so this imports it there instead. Nothing else changes. The import lock still serializes the first conversion, so pandas is never used half-initialized.

The gain is connect latency on a cold start only. Total work is unchanged: a process that converts rows through pandas now pays the import at its first such conversion instead. It is complementary to py-overhead's perf/rd-py-overhead (#1048), which converts small kernel results with to_pylist() and only falls back to the base _convert_arrow_table for larger ones. With both, a process that only fetches small results never imports pandas.

How is this tested?

• Unit tests
• Manually

New tests import the connect-path modules (client, result_set, the kernel client and result set, the SEA backend) in fresh interpreters and assert that pandas isn't loaded. A further test checks that the first row conversion imports pandas and still returns the same rows. Both fail on main. The full unit suite and the realkernel set pass, and so does mypy.

Live Reyden warehouse, use_kernel=True, kernel 1.4.0, main (0c1b9e8) vs this branch, fresh process per run, interleaved:

	main	this PR
first connect() in a process, 5 runs (median)	603 ms	408 ms
200 threads connecting at once, 3 runs: connect() p50 (median)	662 ms	543 ms
same, first session open (median)	574 ms	449 ms

Time to the first fetchall() is the same within noise: the import moves there.

In a separate isolated run (4 processes × 200 sessions from a cold start, 16 cores), all sessions were open after 1.95 s instead of 2.94 s on a mock server with 20 ms per request, and after 2.07 s instead of 2.46 s live.

The saving is much larger where site-packages has no .pyc files (e.g. uv pip install without --compile-bytecode into a location the process can't write). There every process compiles pandas from source. Simulated with PYTHONDONTWRITEBYTECODE=1 and a cache prefix holding only the stdlib's bytecode, import databricks.sql.client takes 1.46–1.56 s on main and 0.57–0.71 s with this change (3 runs each).

Related Tickets & Documents

RE-17633 (connection-creation time at high concurrency).

This pull request and its description were written by Isaac.

[peco-review-bot]

Verdict: 1 Low

Looks good, with one low-severity test-robustness issue. The import is now deferred correctly: _convert_arrow_table is the only place src/ uses pandas, and no test patches a module-level pandas attribute.

[peco-review-bot]

🔵 Low — The child script uses exit code 1 to mean "pandas was imported", but Python also exits with 1 on any uncaught exception. If import {module_name} raises (an ImportError from a missing optional dependency, or a NameError introduced by a refactor), the child exits 1. The returncode not in (0, 1) skip branch then never runs, and the test fails with the misleading message importing X imported pandas.

The guard does still fail rather than falsely pass, but the skip path is effectively dead code and the failure message points at the wrong cause. Wrap the import in try/except and use distinct sentinels, for example 0 = clean, 10 = pandas loaded, 20 = import error, which you then skip on. That matches the pattern the repo adopted after PR #907.

[vuanhphung]

Fixed in 4041f33: the child wraps the import in try/except and exits with distinct sentinels (10 clean, 11 pandas loaded, 12 import failed → skip), so an import error is skipped with its traceback and any other exit fails with the child's stderr.

[peco-review-bot]

Thanks for the reply. The checked-out head doesn't contain this fix yet. tests/unit/test_lazy_pandas_import.py still runs import {module_name} with no try/except, still exits with 1 if "pandas" in sys.modules else 0, and still checks returncode not in (0, 1) before skipping. None of the 10/11/12 sentinels you described are in the file, so commit 4041f33 probably hasn't been pushed to this branch, or it was pushed somewhere else. When that commit lands on the PR head, the next review pass will check it.

[peco-review-bot]

Verdict: 1 Low

Looks good, with one low-severity test-robustness concern. Moving the import is correct: pandas is only used in ResultSet._convert_arrow_table, and nothing in src/ or tests/ relies on a module-level client.pandas or result_set.pandas attribute. The import lock also means concurrent first conversions can't see a half-initialized module.

[peco-review-bot]

Verdict: 1 Low

Looks good, with one low-severity test-coverage gap. The only pandas use left is the function-local import in _convert_arrow_table, which runs after the disable_pandas early return. No tests patch a module-level pandas attribute. The subprocess tests use distinct exit codes, so an import crash can't count as a pass.

[peco-review-bot]

Verdict: 2 Low

Looks good. pandas is still a hard dependency, _convert_arrow_table was its only use, and nothing patches client.pandas or result_set.pandas, so moving the import into the function is safe. Two low-severity test gaps are noted inline.

[peco-review-bot]

🔵 Low — With this change, a process using _disable_pandas=True should never import pandas at all. That is a new property, and nothing pins it. The new tests check the connect path and the pandas conversion path, but not the as_py branch. If someone later moves import pandas above the disable_pandas check (for example to the top of _convert_arrow_table), every test here still passes and those users quietly pay the import again.

Suggested fix: add a sibling subprocess test that sets rs.connection.disable_pandas = True, calls ResultSet._convert_arrow_table, and asserts "pandas" not in sys.modules and the expected rows.

[vuanhphung]

Added in 916638b: test_disable_pandas_never_imports_pandas. Writing it showed the existing conversion test was weaker than it looked: building the test table from Python lists makes pyarrow import pandas itself (its pandas shim), so "pandas in sys.modules" held before the conversion ran. Both tests now read the table from IPC bytes, check pandas isn't loaded beforehand, and assert the rows; both fail on main.

[peco-review-bot]

I couldn't find the 916638b changes at head. tests/unit/test_lazy_pandas_import.py still has only test_connect_path_does_not_import_pandas and test_row_conversion_imports_pandas_on_first_use. There's no test_disable_pandas_never_imports_pandas, and the conversion test still builds its table with pyarrow.table({...}) from Python lists, not from IPC bytes. You may not have pushed the commit yet, or it went to a different branch. Your point about pyarrow's pandas shim matters: if building the table imports pandas, then assert "pandas" in sys.modules passes whether or not the conversion caused the import. So the IPC-based version is worth landing. I'll check again once the commit shows up on this PR's head.

[peco-review-bot]

🔵 Low — _run calls subprocess.run with no timeout. If a child interpreter hangs during import (for example, a kernel wheel that blocks at load), the unit job stalls until CI's global timeout, with no useful failure message. Adding a bounded timeout= (e.g. 120 s) would turn a hang into a clear TimeoutExpired failure.

[vuanhphung]

Added timeout=120 to _run in 916638b.

[peco-review-bot]

Thanks for picking this up. The head checkout still doesn't have the timeout, though. _run in tests/unit/test_lazy_pandas_import.py calls subprocess.run with only capture_output=True and text=True. There's no timeout=120, so commit 916638b either wasn't pushed to this branch or doesn't include that change. Could you check that the commit is on the PR branch? Once timeout= shows up in _run, this is resolved.

[peco-review-bot]

Verdict: 1 Low

Looks good. Moving the import is correct: pandas is used only inside _convert_arrow_table, and no other module or test patch target depends on a module-level pandas attribute. The one low finding is that the new tests don't guard the default Thrift connect path.

[peco-review-bot]

✅ No issues identified by the review bot.