API: Add contextual load apis - #15895
Conversation
e597a1f to
b6d8da2
Compare
| * @return instance of {@link Table} implementation referred by {@code ident} | ||
| * @throws NoSuchTableException if the table does not exist | ||
| */ | ||
| default Table loadTable( |
There was a problem hiding this comment.
what's the reason for adding the default here? the SupportsReferencedBy decorator interface is not extended by this interface. can we just add the proper class that implements this new interface?
Maybe only RESTSessionCatalog should implement the SupportsReferencedBy decorator interface.
There was a problem hiding this comment.
we have SessionContext for this api, it can't implement SupportsReferencedBy which doesn't have sessionContext
There was a problem hiding this comment.
Why not default to calling the parent version without "referencedBy"? I guess I have similar thoughts as Steven here.
There was a problem hiding this comment.
Also noted by the agents, you commented that this would delegate to old implementations for backwards compatibility :)
There was a problem hiding this comment.
I had that earlier version : e597a1f#diff-93c6a4d20e7c7d3a8e9b90cac139da020d6a75138c2a5718941a4c4e7b9d8217R198-R210
forgot to update the description :(, wow agents caught that, thats pretty cool !
Then i went backwards as in this scenario if we let referencedBy skip then its felt a bit incorrect as i am swallowing the intent of the api, throwing unsupported exception would better prompt the user for this is what i think, please let me know you thoughts considering above ?
|
I think we need to override the delegates here as well. Once |
precisley, i was thinking of taking that as part of core changes, here is the complete e2e flow that i had in mind : #13979 please let know if you prefer the delegates too in this pr, happy to put that in the current change. |
| * @throws NoSuchTableException if the table does not exist | ||
| */ | ||
| default Table loadTable( | ||
| SessionContext context, TableIdentifier ident, List<TableIdentifier> referencedBy) { |
There was a problem hiding this comment.
Just wanted to add my concern here that we should probably be adding some sort of LoadContext as opposed to specific parameters that are targeted to this one behavior. Without introducing something that lends itself to evolution, we make future additions very difficult to plumb through and lots of dead/deprecated code paths.
There was a problem hiding this comment.
Added a typed Object with defined fields, let me know wdyt ?
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
|
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
…tualLoad Addresses review feedback from danielcweeks: - PR apache#15895: "generalize this to something more like SupportsContextualLoad and not make it so narrowly specific to this particular field" - PR apache#15895: "we should probably be adding some sort of LoadContext as opposed to specific parameters that are targeted to this one behavior" - PR apache#13979: "much prefer specific context objects" over Map<String, Object> Introduces a typed LoadContext with a builder pattern that can evolve with new fields (e.g., planId, credentials scope) without requiring new interfaces or method overloads for each addition.
bd9c08d to
dc87c4f
Compare
|
I think we probably should tie in the core changes in this PR since we are the main consumer of this API and it's really small. But i'm also ok with just merging this. @danielcweeks do you have any other feedback here ? Are you happy with the builder approach or are you looking for a more context specific type system. I was kind of considering requesting something like List where we have subclasses for the Contexts being passed (ReferencedByContext, ...) But honestly I'm not sure we will extend this all that much so the Builder seems pretty much fine to me. |
| * | ||
| * <p>Catalogs that do not need this context are not required to implement this interface. | ||
| */ | ||
| public interface SupportsContextualLoad { |
There was a problem hiding this comment.
Why do we need this interface as opposed to just adding these methods to the respective Catalog/ViewCatalog?
If the default is to fail, then we don't need a mixin interface.
There was a problem hiding this comment.
The mixin was just there to group catalog (table api) and viewCatalog (view api) for contextual load, and this is a new functionality entirely.
Other angle is spark also needs a mixin and in that case TableCatalog and ViewCatalog are in spark so we need a spark specific mixin since we can't modify them.
i can put them to catalog / viewCatalog if we want to reuse existing api and open to modifying this with default fail which we have been doing.
There was a problem hiding this comment.
ok, i thought about this a lot and i think since we have the handle of both the catalog and viewCatalog api, we can introduce this, to keep view and table api seperate than a mixin but we need the Spark interface though
…tualLoad Addresses review feedback from danielcweeks: - PR apache#15895: "generalize this to something more like SupportsContextualLoad and not make it so narrowly specific to this particular field" - PR apache#15895: "we should probably be adding some sort of LoadContext as opposed to specific parameters that are targeted to this one behavior" - PR apache#13979: "much prefer specific context objects" over Map<String, Object> Introduces a typed LoadContext with a builder pattern that can evolve with new fields (e.g., planId, credentials scope) without requiring new interfaces or method overloads for each addition.
Use Lists.newArrayList() instead of new ArrayList<>() per Iceberg checkstyle rules.
03dc259 to
70c18a2
Compare
70c18a2 to
5ba9427
Compare
Add LoadContext, a value object carrying optional per-load information, currently the ordered chain of views that reference the table or view being loaded. Add contextual load overloads as default methods on Catalog, ViewCatalog, SessionCatalog and ViewSessionCatalog. Each default ignores the context and delegates to the existing single-argument method, so a catalog opts in by overriding rather than by implementing a marker interface. The context is supplied by the client, so a catalog can only use it to narrow access, never to widen it; ignoring it is therefore safe and there is no reason for the default to fail.
5ba9427 to
2fead87
Compare
About the change
SupportsContextualLoad - a new opt-in catalog capability interface with loadTable and loadView methods
that accept an ordered list of referencing view identifiers
Default overloads on SessionCatalog.loadTable and ViewSessionCatalog.loadView that accept the
referenced-by view chain, delegating to the existing methods for backward compatibility
When a table or view is loaded as part of resolving a view, the chain of referencing views can be passed
to the catalog. This enables catalog servers to make authorization, credential scoping, and auditing
decisions based on the full view reference chain.
This is the first in a series of PRs to implement the referenced-by feature end-to-end, broken out from
#13979:
Related