feat: Carry FOR VERSION AS OF from the parser into the plan - #1827
feat: Carry FOR VERSION AS OF from the parser into the plan#1827PingLiuPing wants to merge 2 commits into
Conversation
|
@mbasmanova Could you help take a look at this PR? Thank you. |
mbasmanova
left a comment
There was a problem hiding this comment.
Thank you for picking this up — a version that parses and is then silently dropped is the worst of the options.
Could this land as parse-and-reject only? No connector overrides supportsTableTimeTravel(), so FOR VERSION AS OF 8 never reaches PlanBuilder::tableScan, in a test or in a deployment. The snapshot on SchemaTableName, its place in hashing and identity, the serde field, the PlanBuilder parameter and the connector hook are all unexercised.
Unexercised plumbing does not stay right while it waits. The findView call that follows findTable in RelationPlanner::processTable gets the snapshot-bearing name, and findView is a views_.find(tableName), so the first connector to opt in would see "table not found" for a versioned view rather than a message about views.
The rejection is the valuable half of this change and stands on its own. When a connector is ready to honor a snapshot, the shape can be settled with a real caller in hand — and Presto's is worth a look first: it keeps SchemaTableName a name and passes ConnectorTableVersion beside it into getTableHandle, with the resolved version living in the table handle. That type also carries a version operator for AS OF versus BEFORE and a typed value rather than an integer, since FOR TIMESTAMP AS OF is a timestamp — so an optional<int64_t> on the name would have to be replaced rather than extended once either rejected form is implemented.
Also, rejectsUnsupportedTableVersionForms names the expectation rather than what it covers. tableVersion reads like its neighbours, tablesample and everything.
SELECT ... FROM t FOR VERSION AS OF 8parsed and then read the current snapshot. The grammar accepts theFOR ... AS OF/BEFOREclause and the AST carries a slot for it, butvisitTableNamebuilt the table without ever readingtableVersionExpression, so the version was dropped and the query silently answered from the wrong snapshot.With this patch, snapshot id now travels end to end. SchemaTableName adds an optional snapshotId that is part of table identity (hashing, toString, plan serialization), so two references differing only by snapshot are distinct tables and the optimizer caches them apart. PlanBuilder::tableScan accepts it and the parser fills it from the AST.
Connectors opt in via ConnectorMetadata::supportsTableTimeTravel(), defaulting to false. A versioned reference to a connector that would ignore it now fails with a clear message instead of reading the current version.
FOR TIMESTAMP AS OFandFOR VERSION BEFOREparse but are rejected as not yet supported.