fix: parse timestamp-without-time-zone partition bounds#56
Conversation
📝 WalkthroughWalkthroughChangesPartition bounds
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/partition/partition_test.go (1)
154-163: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the exact session commands.
Substring checks would allow incorrect values such as
ISO, MDYto pass. Assert the complete SQL strings instead.Proposed assertion fix
- assert.Contains(t, db.execSQL[0], "DateStyle") - assert.Contains(t, db.execSQL[0], "ISO") - assert.Contains(t, db.execSQL[1], "TimeZone") - assert.Contains(t, db.execSQL[1], "UTC") + assert.Equal(t, "SET DateStyle = 'ISO, YMD'", db.execSQL[0]) + assert.Equal(t, "SET TimeZone = 'UTC'", db.execSQL[1])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/partition/partition_test.go` around lines 154 - 163, Update TestPinSessionForBounds to assert the complete SQL strings in db.execSQL[0] and db.execSQL[1], replacing the individual assert.Contains checks. Preserve the existing command ordering while validating the exact DateStyle and TimeZone session commands and values.cmd/partitioner/main.go (1)
69-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the duplicate local session-pinning helper.
This path now calls
partition.PinSessionForBounds, butcmd/partitioner/main.go:108-116still contains another implementation. Delete the duplicate so future SQL or error-handling changes cannot diverge.As per coding guidelines: “Follow DRY, extracting shared logic only when there are at least two real callers.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/partitioner/main.go` around lines 69 - 71, Remove the duplicate local session-pinning helper in main.go around the referenced helper definition, retaining partition.PinSessionForBounds as the sole implementation used by the partitioner path. Delete only the redundant function and any now-unused imports or references required for a clean build.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/partitioner/main.go`:
- Around line 69-71: Remove the duplicate local session-pinning helper in
main.go around the referenced helper definition, retaining
partition.PinSessionForBounds as the sole implementation used by the partitioner
path. Delete only the redundant function and any now-unused imports or
references required for a clean build.
In `@internal/partition/partition_test.go`:
- Around line 154-163: Update TestPinSessionForBounds to assert the complete SQL
strings in db.execSQL[0] and db.execSQL[1], replacing the individual
assert.Contains checks. Preserve the existing command ordering while validating
the exact DateStyle and TimeZone session commands and values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 27d51c59-a904-48db-b005-11100816b2b8
📒 Files selected for processing (4)
cmd/archiver/main.gocmd/partitioner/main.gointernal/partition/partition.gointernal/partition/partition_test.go
582e8b9 to
a71bea5
Compare
|
@coderabbitai recheck |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== PR diff ==='
git diff --find-renames origin/main...HEAD -- \
cmd/archiver/main.go \
cmd/partitioner/main.go \
internal/partition/partition.go \
internal/partition/partition_test.go
echo '=== Partition package outline ==='
ast-grep outline internal/partition/partition.go --items all --view expanded
echo '=== Startup call contexts ==='
rg -n -C 10 'PinSessionForBounds|func setupConnection|func main' \
cmd/archiver/main.go cmd/partitioner/main.go
echo '=== Relevant parser and DB interfaces ==='
rg -n -C 12 'func parseTimestamp|func parseBoundExpr|func PinSessionForBounds|type DBTX|pg_get_expr|relpartbound' \
internal/partitionLength of output: 31642 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== pgx connection call sites ==='
rg -n -C 5 '\bpgx\.(Connect|ConnectConfig|ParseConfig)\s*\(' --type go .
echo '=== Command import blocks and connection lifecycle ==='
sed -n '1,135p' cmd/archiver/main.go
printf '\n--- partitioner ---\n'
sed -n '1,105p' cmd/partitioner/main.go
echo '=== TimeBoundary parser delegation ==='
ast-grep outline internal/partition/boundary.go --items all --view expanded
rg -n -C 8 'func \(TimeBoundary\) (Literal|Parse)|parseTimestamp' internal/partition/boundary.go
echo '=== Declared pgx dependency ==='
rg -n -C 2 'jackc/pgx' go.mod go.sumLength of output: 15930
The archiver and partitioner now consistently use the pinned connection configuration, and the parser covers the reported timezone-less timestamp format (including fractional seconds). Static verification only; tests were not executed in this sandbox. ✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/partition/partition_test.go`:
- Around line 154-159: Add a unit test alongside TestBoundConnConfig that
directly exercises the exported Connect function with a malformed DSN and
asserts the returned error. Keep the test focused on the error path; leave
successful connection coverage to integration tests if required.
In `@internal/partition/partition.go`:
- Around line 83-89: Update the parseTimestamp comment’s session-pinning
reference to describe the current Connect/boundConnConfig connection path
instead of naming PinSessionForBounds. Keep the existing explanation of
supported PostgreSQL timestamp renderings unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bed3a896-2358-4dd6-a064-2d96a785bf72
📒 Files selected for processing (4)
cmd/archiver/main.gocmd/partitioner/main.gointernal/partition/partition.gointernal/partition/partition_test.go
| func TestBoundConnConfig(t *testing.T) { | ||
| cfg, err := boundConnConfig("postgres://u@localhost:5432/db") | ||
| require.NoError(t, err) | ||
| assert.Equal(t, "ISO, YMD", cfg.RuntimeParams["datestyle"]) | ||
| assert.Equal(t, "UTC", cfg.RuntimeParams["timezone"]) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a unit test for exported Connect.
TestBoundConnConfig covers only the private helper. The exported Connect function in internal/partition/partition.go, Lines 121-126, has no direct unit test in the supplied changes. Add at least an error-path unit test for malformed DSNs, with any successful connection behavior covered by an integration test if needed.
As per coding guidelines, every public Go function should have a unit test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/partition/partition_test.go` around lines 154 - 159, Add a unit test
alongside TestBoundConnConfig that directly exercises the exported Connect
function with a malformed DSN and asserts the returned error. Keep the test
focused on the error path; leave successful connection coverage to integration
tests if required.
Source: Coding guidelines
| // parseTimestamp parses a pg_get_expr bound value (the quoted string inside | ||
| // FOR VALUES FROM/TO) into a UTC time. PostgreSQL renders bounds ISO-style but | ||
| // not as strict ISO 8601 (space separator, bare +00 offset), so time.RFC3339 | ||
| // does not apply. The layouts are the stdlib time.DateTime (timestamp without | ||
| // time zone) and time.DateOnly (date), plus a numeric-offset layout for | ||
| // timestamptz; time.Parse consumes any trailing fractional second on its own. | ||
| // Callers pin the session (PinSessionForBounds) so the render stays in this set. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the stale session-pinning reference.
Line 89 names PinSessionForBounds, while the current implementation and call sites use Connect/boundConnConfig. Update the comment so it describes the actual current connection path.
Proposed fix
-// Callers pin the session (PinSessionForBounds) so the render stays in this set.
+// Connections created by Connect pin the session so the render stays in this set.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // parseTimestamp parses a pg_get_expr bound value (the quoted string inside | |
| // FOR VALUES FROM/TO) into a UTC time. PostgreSQL renders bounds ISO-style but | |
| // not as strict ISO 8601 (space separator, bare +00 offset), so time.RFC3339 | |
| // does not apply. The layouts are the stdlib time.DateTime (timestamp without | |
| // time zone) and time.DateOnly (date), plus a numeric-offset layout for | |
| // timestamptz; time.Parse consumes any trailing fractional second on its own. | |
| // Callers pin the session (PinSessionForBounds) so the render stays in this set. | |
| // parseTimestamp parses a pg_get_expr bound value (the quoted string inside | |
| // FOR VALUES FROM/TO) into a UTC time. PostgreSQL renders bounds ISO-style but | |
| // not as strict ISO 8601 (space separator, bare +00 offset), so time.RFC3339 | |
| // does not apply. The layouts are the stdlib time.DateTime (timestamp without | |
| // time zone) and time.DateOnly (date), plus a numeric-offset layout for | |
| // timestamptz; time.Parse consumes any trailing fractional second on its own. | |
| // Connections created by Connect pin the session so the render stays in this set. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/partition/partition.go` around lines 83 - 89, Update the
parseTimestamp comment’s session-pinning reference to describe the current
Connect/boundConnConfig connection path instead of naming PinSessionForBounds.
Keep the existing explanation of supported PostgreSQL timestamp renderings
unchanged.
Source: Coding guidelines
1 participant
The archiver failed on
timestamp without time zonepartition columns:pg_get_exprrenders their bounds with no timezone offset, whichparseTimestamprejected. The archiver and partitioner now pin the session toDateStyle='ISO, YMD'andTimeZone='UTC'so bounds render deterministically, andparseTimestampparses them via stdlib layouts.Closes #49.