vbhanuchander-lang commented on issue #15699:
URL: https://github.com/apache/iceberg/issues/15699#issuecomment-5251204292
Before the syntax question, there is a structural obstacle that I think
decides the shape of this: **Iceberg cannot add `VERSION BETWEEN ... AND ...`
to a `SELECT` on its own.** Three points in the code, on Spark 4.1:
**1. `VERSION AS OF` is Spark's syntax, not Iceberg's.** Spark parses it and
calls the catalog's time-travel overloads; Iceberg only receives the
already-parsed value:
```java
// SparkCatalog
public Table loadTable(Identifier ident, String version) { return
load(ident, TimeTravel.version(version)); }
public Table loadTable(Identifier ident, long timestampMicros) { ... }
```
So the existing feature is not evidence that Iceberg can add sibling syntax
— it is evidence that Spark owns this grammar and Iceberg implements a callback.
**2. Iceberg's extension parser is whole-statement, all-or-nothing.** It
cannot hook a sub-rule of Spark's `SELECT`:
```scala
// IcebergSparkSqlExtensionsParser
private def parsePlanWithDelegate(sqlText: String)(delegateParse: String =>
LogicalPlan) = {
if (isIcebergCommand(sqlTextAfterSubstitution)) {
parse(...) { parser => astBuilder.visit(parser.singleStatement()) }
} else {
RewriteViewCommands(...).apply(delegateParse(sqlText)) // entire text
to Spark
}
}
```
**3. And `isIcebergCommand` only ever matches `ALTER TABLE`:**
```scala
normalized.startsWith("alter table") && (normalized.contains("add partition
field") || ... )
```
A `SELECT` therefore goes wholesale to Spark's parser, which does not know
`VERSION BETWEEN`. Correspondingly the `statement` rule in
`IcebergSqlExtensions.g4` contains only `ALTER TABLE` forms — no relation or
`FROM`-clause syntax at all.
So implementing the proposal as written means an upstream Spark grammar
change, not an Iceberg one. Worth knowing before the syntax is bikeshedded
here, because the venue for that decision would be SPARK, not this repo.
---
On @smaheshwar-pltr's semantics question, @ted-jenks's answer checks out
against the code. The DataFrame path builds an `IncrementalAppendScan` with
**start exclusive, end inclusive**:
```java
// SparkScanBuilder.buildIcebergIncrementalAppendScan
.fromSnapshotExclusive(startSnapshotId)
...
scan = scan.toSnapshot(endSnapshotId);
```
so "match the existing `start-snapshot-id` / `end-snapshot-id` semantics" is
a well-defined answer: appended rows only, deletes and overwrites invisible.
The ambiguity @smaheshwar-pltr raised is real, but it is resolved by picking
`IncrementalAppendScan`, and that choice already has a precedent users rely on.
One further wrinkle in the proposal worth noting if it moves upstream:
`VERSION BETWEEN 'tag-a' AND 'tag-b'` and `VERSION BETWEEN 1 AND 5` are
different types in one clause. `TimeTravel.AsOfVersion` already carries this
tension and resolves it by probing (`Longs.tryParse(version) != null` decides
snapshot-id versus ref name), so a range form would inherit the same ambiguity
twice over — `BETWEEN 1 AND 5` is a snapshot range, unless a branch happens to
be named `1`.
---
Given the above, the options that stay inside Iceberg are the ones that need
no new `FROM`-clause syntax:
- a table-valued function, e.g. `SELECT * FROM iceberg_incremental('db.tbl',
1, 5)`
- read options on the existing metadata-table style access
- keeping `create_changelog_view` and documenting the incremental-append
equivalent
None is as pleasant as the proposed syntax, but all are implementable here
rather than in Spark. Happy to prototype whichever direction the maintainers
prefer — I have not written any code for this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
