API, Core: Add sqlFor API to views to handle basic resolution of dialect #9247
Conversation
| * @return the view representation for the given SQL dialect, or null if no representation could | ||
| * be resolved | ||
| */ | ||
| default ViewRepresentation sqlFor(String dialect) { |
There was a problem hiding this comment.
It's unfortunate, this API has to return a ViewRepresentation. It would have been better if it could have returned a SQLViewRepresentation specifically but since that's an implementation in core that's not possible. The issue is that now callers have to cast. But I guess they would've had to do that anyways?
There was a problem hiding this comment.
Should we just move SQLViewRepresentation into API? That would make sense to me.
core/src/test/java/org/apache/iceberg/view/ViewCatalogTests.java
Outdated
Show resolved
Hide resolved
amogh-jahagirdar
force-pushed
the
view-sql-resolve
branch
from
e9e854b to
848f58d
Compare
| return sqlViewRepresentation; | ||
| } | ||
|
|
||
| closest = sqlViewRepresentation; |
There was a problem hiding this comment.
Should we use the first defined representation or the last? I'm inclined to use the first as the closest.
There was a problem hiding this comment.
I don't really have a preference as long as it's consistent (always first or always last or well defined based on some other heuristic).
What's the main benefit you see for using the first? Is it just that it's easier to see in the metadata?
For context, I chose the last just because the implementation is cleaner there (just need a single loop over the representations and tracking the closest is just a matter of updating a reference until either an exact match is found or the loop completes and we return the closest which would be null in case there's no SQL representation).
amogh-jahagirdar
force-pushed
the
view-sql-resolve
branch
from
848f58d to
202dae9
Compare
There was a problem hiding this comment.
A couple minor points, but overall this looks good. Thanks for getting this done, @amogh-jahagirdar!
amogh-jahagirdar
force-pushed
the
view-sql-resolve
branch
from
202dae9 to
8115290
Compare
amogh-jahagirdar
force-pushed
the
view-sql-resolve
branch
from
8115290 to
c8cb259
Compare
|
Thanks for the review @rdblue , merging! |
| for (ViewRepresentation representation : currentVersion().representations()) { | ||
| if (representation instanceof SQLViewRepresentation) { | ||
| SQLViewRepresentation sqlViewRepresentation = (SQLViewRepresentation) representation; | ||
| if (sqlViewRepresentation.dialect().equals(dialect)) { |
There was a problem hiding this comment.
I'm curious whether equalsIgnoreCase would make sense here?
There was a problem hiding this comment.
Ah this was equalsIgnoreCase before but when I refactored back into a loop, it mistakenly became equals. Let me fix this.
There was a problem hiding this comment.
But yes I think the API should be case insensitive (in practice it won't really matter I think because engines would just read and write the view through a constant but it's helpful from an API perspective if someone wants to create the view from say a Python script which doesn't necessarily know the exact constant). The API should still return the same representation
There was a problem hiding this comment.
I should add a test for this as well
This change adds a
ViewRepresentation sqlFor(String dialect)API which will resolve a representation based on the give dialect. The current implementation just aims for an exact search with a fallback to the first added representation in case an exact match does not exist. If no SQL dialect exists, null is returned.The intention of this API is so that engine integrations don't have to implement this same logic. If an engine wants to use a different dialect than what this API returns then they can still implement that.