feat(sea): wire rowLimit + statementConf + TIMESTAMP_NTZ/LTZ params#408
Closed
msrathore-db wants to merge 2 commits into
Closed
feat(sea): wire rowLimit + statementConf + TIMESTAMP_NTZ/LTZ params#408msrathore-db wants to merge 2 commits into
msrathore-db wants to merge 2 commits into
Conversation
msrathore-db
force-pushed
the
msrathore/sea-tls-connectivity
branch
from
f857d15 to
1a72cad
Compare
msrathore-db
force-pushed
the
msrathore/sea-statement-options-params
branch
from
e09869b to
7726d7f
Compare
msrathore-db
changed the title
This was referenced May 31, 2026
Closed
Closed
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
1 similar comment
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
Three statement-option / param-type additions where the kernel + napi were already ready but the node SEA layer didn't expose them: - rowLimit: new `ExecuteStatementOptions.rowLimit` → napi `rowLimit` (SEA `row_limit`). SEA-only server-side cap; Thrift has no execute-time cap. - statementConf: new `ExecuteStatementOptions.statementConf` → napi `statementConf` (SEA `statement_conf`), the Thrift `confOverlay` equivalent. Generalises the existing query_tags serialisation so a caller-supplied statementConf and queryTags merge into one conf map (queryTags already forwarded upstream). - TIMESTAMP_NTZ / TIMESTAMP_LTZ: added to `DBSQLParameterType` so callers can bind timezone-explicit timestamp params. `toSparkParameter` already honours an explicit type and `SeaPositionalParams` passes the SQL type verbatim to the kernel codec (which has the NTZ/LTZ arms). Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
The cascade commit that surfaced `statementId` on `SeaNativeStatement` and `sessionId` on `SeaNativeConnection` (matching the kernel napi binding's new getters) didn't update the blocking test fakes, breaking compilation. Add the readonly fields to FakeNativeStatement / FakeNativeConnection (execution.test.ts) and FakeNativeStatement / FakeMetadataConnection (metadata.test.ts). The async fakes already carried statementId. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
msrathore-db
force-pushed
the
msrathore/sea-statement-options-params
branch
from
68a65ce to
edd07c8
Compare
msrathore-db
added a commit
that referenced
this pull request
Jun 3, 2026
Consolidates the last net-new bit of the superseded #408: two SEA-path DBSQLParameterType variants for binding timezone-explicit timestamps. The type name flows through the existing param codec (toSparkParameter → sqlType), which the kernel accepts — validated live (SELECT ? with TIMESTAMP_NTZ/LTZ returns the bound values). On the Thrift backend they degrade to a plain TIMESTAMP bind. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
Contributor
Author
|
Superseded by the consolidated SEA stack (#412 → #413 → #414) and closing to minimise open PRs. Coverage verified before closing: rowLimit + statementConf + TIMESTAMP_NTZ/LTZ params → consolidated into #413 (buildExecuteOptions rowLimit/statementConf + DBSQLParameterType.TIMESTAMP_NTZ/LTZ, both warehouse-validated). No unique code is lost. |
msrathore-db
added a commit
that referenced
this pull request
Jun 3, 2026
Consolidates the last net-new bit of the superseded #408: two SEA-path DBSQLParameterType variants for binding timezone-explicit timestamps. The type name flows through the existing param codec (toSparkParameter → sqlType), which the kernel accepts — validated live (SELECT ? with TIMESTAMP_NTZ/LTZ returns the bound values). On the Thrift backend they degrade to a plain TIMESTAMP bind. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Three statement-option / param-type additions where the kernel + napi binding were already complete but the node SEA layer never exposed them. (Audit "Group A"; the fourth Group-A item,
queryTags, was a dropped pre-existing option and is fixed upstream in #403.)ExecuteStatementOptions.rowLimit→ napirowLimit(SEArow_limit)ExecuteStatementOptions.statementConf→ napistatementConf(ThriftconfOverlayequivalent)Datecoerced toTIMESTAMPDBSQLParameterType.TIMESTAMP_NTZ/TIMESTAMP_LTZselectableWhy these were "free"
The kernel
StatementSpec(row_limit,statement_conf, NTZ/LTZTypedValuearms) and napiExecuteOptionsalready exposed everything; the only missing piece was the nodeExecuteStatementOptionssurface + threading. No kernel/napi change.Notes
query_tagsserialisation (wired upstream in feat(sea): Thrift-parity — params, intervals, getInfo, SQL-error class, input validation + queryTags #403): a caller-suppliedstatementConfandqueryTagsmerge into one conf map.maxRowsremains the client-side per-fetch chunk size on both backends.toSparkParameteralready honours an explicittypeandSeaPositionalParamspasses the SQL type verbatim to the kernel codec — so only the enum values were needed.Testing
240 SEA unit tests pass (rowLimit/statementConf forwarding, statementConf+queryTags merge, NTZ/LTZ param mapping). Verified live against pecotesting earlier (rowLimit:7 caps a
range(0,100)result to 7 rows; NTZ param round-trips).Stacking
Stacked on #407 → #406 → #403 → #404.
This pull request and its description were written by Isaac.
Downstream fixes / reviewer note
flatbuffers, one-pass IPC duration rewrite, cancel/close local-state rollback on native RPC failure, closed fetch →OperationStateError(Closed), and Arrow duration rewriter cleanup.useCloudFetchrather than silently ignoring it, and uses nativesessionId/statementIdfor session/operation ids where available.