Reviewed 17 of 17 files at r1, 8 of 8 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nihalpednekar and @otan)

pkg/ccl/logictestccl/testdata/logic_test/as_of, line 126 at r2 (raw file):

does not yet work

Bounded staleness queries in explicit transactions are not something I imagine we will support because the semantics around which timestamp to use are strange and not particularly useful. Consider being more direct here and saying that this is unsupported.

pkg/sql/sem/tree/as_of.go, line 84 at r2 (raw file):

	// Timestamp is the HLC timestamp evaluated from the AS OF SYSTEM TIME clause.
	Timestamp hlc.Timestamp
	// BoundedStaleness is true if the AS OF SYSTEM TIME clause allows bounded

Is "allows" the right word here? That makes it sound like this flag can be ignored. But it can't, right? It changes the semantics of the Timestamp field, from the exact timestamp to evaluate the query to a minimum staleness bound used to dynamically determine the timestamp to evaluate the query.

pkg/sql/sem/tree/as_of.go, line 110 at r2 (raw file):

	opts ...EvalAsOfTimestampOption,
) (AsOfSystemTime, error) {
	o := evalAsOfTimestampOptions{}

This options struct is forced to allocate and escape to the heap, right? I'm trying to remember how many heap allocations this options pattern causes. The static allocation of EvalAsOfTimestampOptionAllowBoundedStaleness avoids one, and then the varargs shouldn't cause any (but worth confirming, they may if not inlined), so maybe this is the only one.

Consider replacing type EvalAsOfTimestampOption func(o *evalAsOfTimestampOptions) with type EvalAsOfTimestampOption func(o evalAsOfTimestampOptions) evalAsOfTimestampOptions to avoid the heap allocation.

pkg/sql/sem/tree/as_of.go, line 119 at r2 (raw file):

		if o.allowBoundedStaleness {
			optFuncs = fmt.Sprintf(
				", %s, %s",

nit: comma at the end of string

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nihalpednekar and @nvanbenschoten)

pkg/sql/sem/tree/as_of.go, line 119 at r2 (raw file):

oh no, a believer of the oxford comma D:

Done

Reviewed 4 of 4 files at r3.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @nihalpednekar)

pkg/sql/sem/tree/as_of.go, line 84 at r2 (raw file):

This comment or the one above should probably also discuss the difference in the meaning of Timestamp when BoundedStaleness is set to true versus when it is set to false.