fix(core): UPDATE to work with bind variables for TIMESTAMP columns#6510
fix(core): UPDATE to work with bind variables for TIMESTAMP columns#6510bluestreak01 merged 8 commits intomasterfrom
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request modifies timestamp column update handling in SQL code generation and bind variable services. It qualifies a QUERY constant, updates metadata construction for timestamp columns during updates, and changes error message formatting to use numeric type codes. A new test class validates timestamp updates through PostgreSQL wire protocol APIs. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
core/src/main/java/io/questdb/griffin/SqlCodeGenerator.java (1)
5394-5469: TIMESTAMP + bind-variable update handling looks good; consider documenting invariantsThe updated block in
generateSelectVirtualWithSubQuerydoes what we want for UPDATEs:
- For update models, unresolved functions (e.g. bind vars) are now typed via
assignType(targetColumnType, ...), so a TIMESTAMP column will drive the bind-variable type instead of stayingUNDEFINED.- When the logical target type is TIMESTAMP but the underlying expression function remains
STRING/VARCHAR, the special-casecolumnType == TIMESTAMP && (function.getType() == STRING || function.getType() == VARCHAR)and metadata construction usingfunction.getType()let the metadata reflect the string-y representation the client actually sends, which should address the previous “contextType=unknown” failure for timestamp updates.Two small, non-blocking suggestions:
- The
columnIndexfromupdateTableColumnNames().indexOf(column.getAlias())is assumed to be non‑negative; that invariant already exists ingenerateSelectChoose, but anassert columnIndex >= 0(or a brief comment) here would make it explicit and future‑proof against additional non‑SET projection columns being added to update models.- The TIMESTAMP/STRING/VARCHAR branch relies on
columnTypepotentially diverging fromfunction.getType()(after the widening/implicit-cast logic). A short comment explaining that this is intentional—metadata should describe the physical bind type while the engine still treats the target as TIMESTAMP—would help avoid accidental “cleanup” of this subtle logic later.Functionally though, the change is coherent with the existing update/type-casting flow.
core/src/test/java/io/questdb/test/cutlass/pgwire/PGTimestampUpdateTest.java (1)
106-142: Consider deduplicating the SQLException‑wrapping lambdasThe helper method is well‑structured: it validates the initial row, performs the UPDATE via bind variables, drains the WAL, and re‑asserts the final state, all under
assertWithPgServer. One minor improvement would be to reduce the repeatedtry { … } catch (SQLException e) { throw new RuntimeException(e); }blocks in each test by introducing a small helper (e.g., aThrowingSqlConsumerfunctional interface plus an adapter toConsumer<PreparedStatement>), but this is purely cosmetic and can be deferred.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/io/questdb/griffin/SqlCodeGenerator.java(3 hunks)core/src/main/java/io/questdb/griffin/engine/functions/bind/BindVariableServiceImpl.java(2 hunks)core/src/test/java/io/questdb/test/cutlass/pgwire/PGTimestampUpdateTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (34)
- GitHub Check: New pull request (Hosted Running tests on windows-other-2)
- GitHub Check: New pull request (Hosted Running tests on windows-other-1)
- GitHub Check: New pull request (Hosted Running tests on windows-pgwire)
- GitHub Check: New pull request (Hosted Running tests on windows-cairo-2)
- GitHub Check: New pull request (Hosted Running tests on windows-cairo-1)
- GitHub Check: New pull request (Hosted Running tests on windows-fuzz2)
- GitHub Check: New pull request (Hosted Running tests on windows-fuzz1)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-other)
- GitHub Check: New pull request (Hosted Running tests on windows-griffin-sub)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-pgwire)
- GitHub Check: New pull request (Hosted Running tests on windows-griffin-base)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-cairo-sub)
- GitHub Check: New pull request (Hosted Running tests on mac-other)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-cairo-root)
- GitHub Check: New pull request (Hosted Running tests on mac-pgwire)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-fuzz2)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo-fuzz)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-fuzz1)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-griffin-sub)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-griffin-root)
- GitHub Check: New pull request (Hosted Running tests on mac-griffin)
- GitHub Check: New pull request (SelfHosted Other tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Other tests on linux-x86-graal)
- GitHub Check: New pull request (Rust Test and Lint on linux-jdk17)
- GitHub Check: New pull request (SelfHosted Other tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Griffin tests on linux-x86-graal)
- GitHub Check: New pull request (SelfHosted Griffin tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Griffin tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Cairo tests on linux-x86-graal)
- GitHub Check: New pull request (Trigger Enterprise CI Trigger Enterprise Pipeline)
- GitHub Check: New pull request (SelfHosted Cairo tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Cairo tests on linux-arm64)
- GitHub Check: New pull request (Check Changes Check changes)
🔇 Additional comments (6)
core/src/main/java/io/questdb/griffin/engine/functions/bind/BindVariableServiceImpl.java (2)
214-215: Error message indefinedefault branch now better reflects raw context typeSwitching to
.put(type)instead ofColumnType.nameOf(type)makes the error expose the exact encoded type value, which addresses the previous"contextType=unknown"issue and is safe since this path is already exceptional-only.
1208-1210:setLong0now reports LONG as the source type, consistent with other settersPassing
ColumnType.LONGintoreportErrorcorrectly identifies the bound value’s logical type (LONG) and aligns with the patterns insetInt0,setShort0, etc., improving diagnostic clarity without changing binding behavior.core/src/main/java/io/questdb/griffin/SqlCodeGenerator.java (1)
648-656: Explain wrapper condition remains correctUsing
QueryModel.QUERYhere keeps the explain-behaviour identical while avoiding reliance on the unqualified constant; no functional or safety concerns.core/src/test/java/io/questdb/test/cutlass/pgwire/PGTimestampUpdateTest.java (3)
36-38: Test class setup and annotations look correctExtending
BasePGTestand using theSqlNoDataSourceInspectionsuppression is consistent with other PG wire tests; class scaffolding is fine and ready to integrate with the existing test harness.
39-70: Micros TIMESTAMP tests cover all relevant binder variantsThe three
TIMESTAMPtests (setLong, setString, setTimestamp) nicely exercise the different binder paths, and the epoch/expected strings are internally consistent (1764863925s →2025-12-04T15:58:45, with the fractional parts matching the chosen microsecond values). These should reliably guard the regression around UPDATE + bind variables for microsecond timestamps.
72-104: Nanos TIMESTAMP_NS tests correctly validate nano vs micro precision behaviorThe
TIMESTAMP_NStests mirror the micros ones and explicitly check:
setLongas nanoseconds since epoch,- string parsing with 9‑digit fractional seconds, and
setTimestamponly transferring micro precision (4242ns→000004000).The expectations (
.000000042and.000004000) align with typical QuestDB semantics, and the explicit comment on micro‑only transfer makes the intent clear. This is a solid coverage matrix for the nano path.
[PR Coverage check]😍 pass : 8 / 9 (88.89%) file detail
|
2 participants
Fix the
UPDATEstatement to handleTIMESTAMPcolumns correctly when bind variables used.When attempted to update a
TIMESTAMPcolumn using bind variables, the following error was returned: