Core: Common metadata for TableMetadata and ViewMetadata #9682
Conversation
|
cc: @szehon-ho , @nastra , @pvary |
nk1506
changed the title
|
Sorry for the delay. I have some feedback and I'll take a closer look at this (and the Hive + View support PR) once 1.5.0 is out |
|
|
||
| @Value.Parameter(order = 1002) | ||
| int formatVersion(); | ||
|
|
There was a problem hiding this comment.
Question, can we put Override and Value.parameter here, and remove it from TableMetadata? It will be more clear.
There was a problem hiding this comment.
Could you please add more context? i couldn't understand this part. Do you mean something like all the Members of BaseMetadata should be with ViewMetadata too with Override? In that case I dont think we need
@Value.Parameter(order=?)
There was a problem hiding this comment.
Yea exactly, is it possible to remove @Value.Parameter in the BaseMetadata, and keep the annotations all in the ViewMetadata.
There was a problem hiding this comment.
@nk1506 let me know if the question makes more sense.
There was a problem hiding this comment.
@szehon-ho , Since we are planning to keep common members with ViewMetadata too with @Override. Do we have a need of keeping @Value.Parameter in ViewMetadata. I think only override should works.
WDYT ? I have updated the patch with only @Override and removed @Value.Parameter from everywhere.
|
|
||
| /** | ||
| * A base class for {@link TableMetadata} and {@link org.apache.iceberg.view.ViewMetadata} It | ||
| * abstracts out all the common metadata for table ane view. |
There was a problem hiding this comment.
typo for ane. I would actually may be renmove the first sentence, it looks like it does not add that much more info than the first sentence. So just
A base class for {@link TableMetadata} and {@link org.apache.iceberg.view.ViewMetadata}.
| * A base class for {@link TableMetadata} and {@link org.apache.iceberg.view.ViewMetadata} It | ||
| * abstracts out all the common metadata for table ane view. | ||
| */ | ||
| public interface BaseMetadata extends Serializable { |
There was a problem hiding this comment.
Just one question. Is this refactoring enough?
If we Introduce BaseMetadata, I expect the TableOperations and ViewOperations to have a base class called BaseOperations which uses this BaseMetadata for interfaces like
void commit(BaseMetadata base, BaseMetadata metadata);
Without that I don't see much value in this refactoring.
There was a problem hiding this comment.
may be we can have an end to end refactoring DRAFT PR and take a decision based on that.
There was a problem hiding this comment.
I agree, But i think those changes we can do later once this interface pattern gets approved. Or may be with this patch itself.
WDYT ?
There was a problem hiding this comment.
we can have a separate draft patch. I think doing that refactoring can help us know whether the current interfaces in BaseMetadata is enough or do we need more changes here.
There was a problem hiding this comment.
Yea I think the main refactor is over at #8907 (including baseOperations and everything). I do agree that it is a bit hard to read, I have to search over there to find what is needed and not, and so may miss something :).
There was a problem hiding this comment.
This looks ok to me as a refactor. I wonder if we need more input from @ajantha-bhat or @nastra , else Ill merge it in a day or two. It does need https://github.com/nk1506/iceberg/pull/10/files for context.
|
|
||
| Map<String, String> properties(); | ||
|
|
||
| @Nullable |
There was a problem hiding this comment.
Slightly prefer not to have it, as most Iceberg classes don't have it. But understand its from the old class.
|
let's hold off on this one until 1.5.0 is out and the Hive view PR is reviewed so that we can better decide in which direction we'd want to go |
|
Can we please continue this effort , it has been stalled 2 weeks? Let's have a PR that is the end to end version (please not on your own github, but against Iceberg github). |
|
Thanks @szehon-ho for looking into this again. I will raise another combined PR with CommonMetadata, CommonHiveOperation and Hive-View |
Hi @szehon-ho , I have created a new PR here #9852 . |
|
I reviewed #9852 and also took a look at the metadata abstraction but I'm not convinced it actually makes stuff easier. Most of the time certain Hive methods only require 1-2 params (properties, schema, ...) from table/view metadata, so passing those directly makes more sense. Additionally, this abstraction can actually hide errors, where we check for certain table properties by providing |
|
Thanks, i commented on the other thread in #9852 (comment) the original reason (saw that one first) Can you clarify this concern?
This is just a class hiearchy. The view now gets methods like String property(String property, String defaultValue), which is just shorthand for viewMetadata.getProperties().getOrDefault(property, defaultValue). I believe its not strictly necessary that these methods to be added here (it seems just for convenience), all we really need to be able to share View/TableCatalog code is properties(). |
|
Common code abstraction on Hive-Table/View has been parked for later. BaseMetadata for Table and View might not be required. We can fulfil the same using different strategy. |
Ref Issue #9514
TableMetadata and ViewMetadata have some common methods , Which have been abstracted out to IcebergMetadata now. It should help to have common code for Iceberg Table and View implementations with different catalog.