Skip to content

Add Tablet.SetTimestampAt with rowIndex validation#169

Merged
HTHou merged 1 commit into
apache:mainfrom
PDGGK:fix/set-timestamp-bounds
Jul 24, 2026
Merged

Add Tablet.SetTimestampAt with rowIndex validation#169
HTHou merged 1 commit into
apache:mainfrom
PDGGK:fix/set-timestamp-bounds

Conversation

@PDGGK

@PDGGK PDGGK commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Problem

Tablet.SetTimestamp writes to t.timestamps[rowIndex] without validating
rowIndex, so an out-of-range row panics with an index-out-of-range error
instead of returning one. For example, with a tablet created via
NewTablet(..., 1) (only row index 0 is valid), SetTimestamp(ts, 1) panics.

Fix

As @HTHou noted, adding an error return to SetTimestamp would break source
compatibility for consumers relying on its signature (interface satisfaction,
method values), and this module already has v2 releases. So SetTimestamp is
left unchanged, and a checked alternative is added:

func (t *Tablet) SetTimestampAt(timestamp int64, rowIndex int) error {
	if rowIndex < 0 || rowIndex >= t.maxRowNumber {
		return fmt.Errorf("illegal argument rowIndex %d", rowIndex)
	}
	t.timestamps[rowIndex] = timestamp
	return nil
}

SetTimestampAt mirrors the bounds handling of SetValueAt / GetValueAt,
returning an illegal argument rowIndex error for a negative or out-of-range
row. The PR is purely additive; SetTimestamp can be deprecated in
documentation later if callers should migrate to the checked API.

Tests

Added TestTablet_SetTimestampAtRowIndexBounds: a valid row index returns no
error; out-of-range indices (1 on a one-row tablet, and -1) return an error
instead of panicking. go build ./... and go test ./client/ pass.

Follow-up to #168.

@HTHou

HTHou commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Could we preserve the existing SetTimestamp(timestamp int64, rowIndex int) signature and introduce a checked API such as SetTimestampAt(timestamp int64, rowIndex int) error instead?

Adding an error result to the existing exported method is not fully source-compatible. Statement-form calls still compile, but consumers using an interface or method value will break, for example:

type TimestampSetter interface {
    SetTimestamp(int64, int)
}

var _ TimestampSetter = (*client.Tablet)(nil)
var setter func(int64, int) = tablet.SetTimestamp

Since this module already has v2 releases, keeping SetTimestamp unchanged would preserve compatibility, while SetTimestampAt can perform the new bounds validation and return illegal argument rowIndex errors. The existing method could be deprecated in documentation later if callers should migrate to the checked API.

Tablet.SetTimestamp writes to t.timestamps[rowIndex] without validating
rowIndex, so an out-of-range row (e.g. SetTimestamp(ts, 1) on a tablet
created with NewTablet(..., 1)) panics with an index-out-of-range error.

Per review, SetTimestamp's signature is kept unchanged for source
compatibility (it is part of a v2 public API). This adds a checked
alternative, SetTimestampAt(timestamp, rowIndex) error, which validates
rowIndex and returns an "illegal argument rowIndex" error, consistent with
SetValueAt and GetValueAt, plus a regression test.

Follow-up to apache#168.

Signed-off-by: Zihan Dai <99155080+PDGGK@users.noreply.github.com>
@PDGGK
PDGGK force-pushed the fix/set-timestamp-bounds branch from 514b827 to d3607ad Compare July 24, 2026 05:18
@PDGGK PDGGK changed the title Validate rowIndex in Tablet.SetTimestamp Add Tablet.SetTimestampAt with rowIndex validation Jul 24, 2026
@PDGGK

PDGGK commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @HTHou — good point on the interface/method-value compatibility, and you're right that a v2 public API shouldn't change SetTimestamp's signature. Adopted: SetTimestamp is left exactly as-is, and I added SetTimestampAt(timestamp int64, rowIndex int) error doing the bounds validation (returning illegal argument rowIndex, consistent with SetValueAt/GetValueAt). The PR is now purely additive. Happy to add a doc note steering callers to the checked API if you'd like.

@HTHou
HTHou merged commit 7b5a8f2 into apache:main Jul 24, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants