Reject timestampAsString override on kernel sessions - #935
Conversation
e84aa7d to
9d63ab7
Compare
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — the kernel _check_session_configuration faithfully mirrors Thrift's validation (same Error class and message), the check correctly runs against the stringified session_conf, and the new test asserts both rejection and that no Session is constructed. One low-severity divergence: unlike Thrift, the kernel path does not force-inject timestampAsString="false" when the config is absent.
jay-xiao446
added
the
engineer-bot
GitHub Actions
Failure
9d63ab7 to
e8a375a
Compare
Signed-off-by: Jay Xiao <jay.xiao@databricks.com>
e8a375a to
84c36e6
Compare
Addresses: - #3874674214 at src/databricks/sql/backend/kernel/client.py:331 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
| return sql[i:] | ||
|
|
||
|
|
||
| def _check_session_configuration(session_configuration: Dict[str, str]) -> None: |
There was a problem hiding this comment.
Why do we need this?
Thrift pass this in ExecuteStatement, I am not even sure if session can be opened if this is passed through. Does dbsql support this param at all?
[CONFIG_NOT_AVAILABLE.WITHOUT_SUGGESTION] Configuration spark.thriftserver.arrowbasedrowset.timestampasstring is not available. SQLSTATE: 42K0I
There was a problem hiding this comment.
The _check_session_configuration guard in kernel/client.py was carried over from the Thrift backend (thrift_backend.py:563) purely as a fail-fast: it rejects a user-supplied timestampAsString != false with a clear connector-side error instead of a confusing server error. Unlike Thrift, the kernel path deliberately does NOT inject timestampAsString=false (see the comment at client.py:344-353) because the kernel owns type handling natively.
Your point is valid and I can't confirm it from here: session_conf is forwarded to the kernel verbatim (client.py:328-329), so if SEA rejects this Thrift-only param entirely (the CONFIG_NOT_AVAILABLE / SQLSTATE 42K0I you cite), then even a false value would break session open, which makes the guard pointless and argues for stripping the key (or dropping the guard) rather than validating it. Confirming whether SEA accepts the param at all requires a live warehouse — the unit-only follow-up job here has no E2E credentials, so I can't verify the server behavior in this PR. Can you confirm SEA's behavior for this key? If it's rejected outright, the right fix is to strip TIMESTAMP_AS_STRING_CONFIG from session_conf before it reaches the kernel and remove the guard; I'll make that change once we've settled the intended behavior.
There was a problem hiding this comment.
The Thrift one come in through ExecuteStatement and it is added by the python connector itself, but here you are guarding against OpenSession.
For kernel/SEA that flag should not matter and we should not need to guard anything about it since nobody gonna pass it on purpose.
There was a problem hiding this comment.
Done. The reviewer's point had converged (the timestampAsString guard is meaningless on the kernel/SEA path — nobody passes that Thrift-only flag on purpose), so I actioned it as a code change rather than continuing the discussion:
src/databricks/sql/backend/kernel/client.py
- Deleted
_check_session_configurationand its call site inopen_session - Removed the now-unused
TIMESTAMP_AS_STRING_CONFIGconstant andErrorimport - Left a comment noting
session_confis forwarded to the kernel verbatim, with no injection or validation of the Thrift-only flag
tests/unit/test_kernel_client.py
- Removed the three guard tests, added one asserting the conf is forwarded verbatim, dropped the unused
Errorimport
All 127 tests in tests/unit/test_kernel_client.py pass (run wi
Pushed acbe571.
There was a problem hiding this comment.
good catch, thanks
There was a problem hiding this comment.
⛔ NEEDS HUMAN DECISION — the bots can't resolve this thread; a maintainer's input is required.
Thread has converged: the requested change was already applied and pushed in commit acbe571 (removed _check_session_configuration, its call site, and the unused TIMESTAMP_AS_STRING_CONFIG constant — confirmed absent from client.py just now), and the reviewer's last message ("good catch, thanks") is a closing acknowledgment. Nothing further is actionable in this PR; no code change or reply is warranted.
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
Addresses: - #3876101905 at src/databricks/sql/backend/kernel/client.py:105 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 High
Blocking mismatch: the PR title, description, and changelog all claim the kernel path now rejects a non-false timestampAsString "matching the Thrift backend's validation," but the diff (comment + new test) implements and asserts the opposite — verbatim forwarding with no rejection. The code itself is internally consistent; the documentation/PR metadata is wrong. Align title/description/changelog with the actual behavior (or implement the rejection the title promises). See F1.
Addresses: - #3876213902 at CHANGELOG.md:4 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
|
There is no meaningful code changes in this PR I think we can just close it @jay-xiao446 |
3 participants
Summary
Tests