yyanyy commented on PR #57585:
URL: https://github.com/apache/spark/pull/57585#issuecomment-5364295716
> Question: do we actually have to support state options in RelationCatalog?
I think state options today apply only to tables, meaning we can follow what we
do today for time travel and write privileges. If we detect state options,
don't attempt to use the optimized loading and just try TableCatalog.
Thanks for the review!
I think it's better to route the state options to `loadRelation` instead of
directly assuming it's a table, for 2 reasons:
1. I think this will introduce a real behavior regression. Consider the
below case
```
spark.read
.option("snapshot", "s1")
.table("catalog.namespace.some_view")
```
Despite that arguably this is not the "right way to run the query", today we
can return the existing view, ignoring `snapshot` argument. Once we make this
change and connector implementor decides to treat `snapshot` as a table state
option, Spark only treat it as a table and does not consider the existing view,
and the same query will start to fail after upgrading.
It may feel like time travel have a similar challenge/breaking behavior but
I think it's not a precedent. Before RelationCatalog was introduced, time
travel already called the table-specific loadTable(ident, version/timestamp)
overloads and never tried to resolve a view; adding loadRelation preserved that
existing table-only behavior. Connector-defined state options are different:
they are opaque options added to an otherwise ordinary read that previously
could resolve a view.
2. I think this adds additional burden to the implementor of
`RelationCatalog`. One of its benefits is that an implementer can complete the
loading behavior by only implementing the below 2 APIs:
```
loadRelation(ident)
loadRelation(ident, stateOptions) // when the catalog consumes state options
```
If we implement this proposal, a state-ware RelationCatalog implementer must
additionally implement `loadTable(ident, TableContext, stateOptions)`. We could
add a default implementation of that `loadTable` overload to delegates back to
`loadRelation(ident, stateOptions)`, but with that we have effectively
reconstructed the current design with additional indirection.
--
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]
