[WIP] API/Core: View support #4657
Conversation
|
PR support spark sql? |
|
Yes, will follow up with PR to support Spark ViewCatalog once the commit for ViewCatalog interfaces in apache/spark#35636 is merged. |
jzhuge
force-pushed
the
view-support
branch
4 times, most recently
from
343c899 to
eda62c2
Compare
Co-authored-by: anjalinorwood@gmail.com
Co-authored-by: anjalinorwood@gmail.com
| * @return this catalog's name | ||
| */ | ||
| default String name() { | ||
| return toString(); |
There was a problem hiding this comment.
I think this should not be default, otherwise it will show the Java object string by default that is unique for each instance.
| * @return this catalog's name | ||
| */ | ||
| default String name() { | ||
| return toString(); |
There was a problem hiding this comment.
I think this should not be default, otherwise it will show the Java object string by default that is unique for each instance. We can just leave this to implementation
| * @return instance of {@link View} implementation referred by {@code tableIdentifier} | ||
| * @throws NoSuchViewException if the view does not exist | ||
| */ | ||
| View loadView(TableIdentifier identifier); |
There was a problem hiding this comment.
thould throws NoSuchViewException be a part of API declaration?
| * @return instance of {@link View} implementation referred by {@code tableIdentifier} | ||
| * @throws NoSuchViewException if the view does not exist | ||
| */ | ||
| View loadView(TableIdentifier identifier); |
There was a problem hiding this comment.
thould throws NoSuchViewException be a part of API declaration?
| */ | ||
| View createView( | ||
| TableIdentifier identifier, | ||
| List<ViewRepresentation> representations, |
| */ | ||
| View createView( | ||
| TableIdentifier identifier, | ||
| List<ViewRepresentation> representations, |
| } | ||
|
|
||
| /** | ||
| * Drop a view and delete all data and metadata files. |
There was a problem hiding this comment.
I thought view is only for logical view. Are we going to make this also materialized view?
| } | ||
|
|
||
| /** | ||
| * Drop a view and delete all data and metadata files. |
There was a problem hiding this comment.
I thought view is only for logical view. Are we going to make this also materialized view?
| void renameView(TableIdentifier from, TableIdentifier to); | ||
|
|
||
| /** | ||
| * Invalidate cached view metadata from current catalog. |
There was a problem hiding this comment.
it is unclear to me why this is a part of the API, it feels like an engine-side cache, how is data actually cached?
| void renameView(TableIdentifier from, TableIdentifier to); | ||
|
|
||
| /** | ||
| * Invalidate cached view metadata from current catalog. |
There was a problem hiding this comment.
it is unclear to me why this is a part of the API, it feels like an engine-side cache, how is data actually cached?
| * @param definition a view definition | ||
| * @param properties a string map of view properties | ||
| */ | ||
| default View createView( |
There was a problem hiding this comment.
Is it worth using a builder style like createTable?
| * @param definition a view definition | ||
| * @param properties a string map of view properties | ||
| */ | ||
| default View createView( |
There was a problem hiding this comment.
Is it worth using a builder style like createTable?
| import java.util.List; | ||
| import org.apache.iceberg.Schema; | ||
|
|
||
| public interface ViewDefinition extends ViewRepresentation { |
There was a problem hiding this comment.
why do we want this extension? I thought we just want different view representation implementations
| import java.util.List; | ||
| import org.apache.iceberg.Schema; | ||
|
|
||
| public interface ViewDefinition extends ViewRepresentation { |
There was a problem hiding this comment.
why do we want this extension? I thought we just want different view representation implementations
| * When committing, these changes will be applied to the current view metadata. | ||
| * Commit conflicts will be resolved by applying the pending changes to the new view metadata. | ||
| */ | ||
| public interface ViewUpdateProperties extends PendingUpdate<Map<String, String>> { |
| * When committing, these changes will be applied to the current view metadata. | ||
| * Commit conflicts will be resolved by applying the pending changes to the new view metadata. | ||
| */ | ||
| public interface ViewUpdateProperties extends PendingUpdate<Map<String, String>> { |
| * @param key a String key | ||
| * @param value a String value | ||
| * @return this for method chaining | ||
| * @throws NullPointerException If either the key or value is null |
There was a problem hiding this comment.
I think the standard is generally IllegalArgumentException if either the key or value is null.
| * | ||
| * @return a long ID for this version's parent, or null if it has no parent | ||
| */ | ||
| Integer parentId(); |
There was a problem hiding this comment.
Should this be Long?
| * | ||
| * @return a long ID | ||
| */ | ||
| int versionId(); |
There was a problem hiding this comment.
Should this be a long?
| } | ||
|
|
||
| public Builder sql(String value) { | ||
| sql = value; |
There was a problem hiding this comment.
Just thinking out loud: If I'm not mistaken the spec basically lets the SQL be free form and with a dialect, an engine can do whatever parsing/analysis it needs to translate to its desires. However, certain aspects such as a view cannot reference itself (either directly or indirectly) seem universal across engines and it seems like we would want some layer of analysis within Iceberg to prevent that. In other words some semantic analysis should be baked into the spec.
But I haven't fully thought this through, maybe there's something I'm missing.
There was a problem hiding this comment.
Never mind on this
|
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. |
Support view.
The view spec has been added in #3188.
TODO:
Follow-ups: