I think it’s still worthwhile to include the snapshot and timestamp refs for completeness sake.
Also, Jan brought up interesting use case with BI tool using the MV without SQL representation. The BI tool can get all table and view dependencies if the lineage is complete.
Thanks
On May 17, 2024, at 1:35 PM, Walaa Eldin Moustafa <wa.moust...@gmail.com> wrote:
Sounds good. I am assuming we agree it is not required for either snapshot or timestamp?
Thanks, Walaa.
I like Jack's suggestions to capture the ref type and value! When the ref type is branch, the snapshot id is dynamic and so the engine using the MV can validate that the latest snapshot on a branch matches the branch snapshot at the time of materialization.
I think if we do this then we don't need to precisely identify the same table (at different snapshots) in the MV's query tree. So, we don't need to capture any additional properties like alias, parent view, path to root, sequence number, etc.
Thanks Benny
Thanks Jack, and welcome back!
Taking a step back, I understand the initial concern was that a table name (e.g., t1 in your example) would be referenced multiple times in the view definition and each reference is associated with a different snapshot ID, hence UUID is not sufficient to capture each occurrence/reference. I proposed:
* The solution to track unique occurrences is to use something along the lines of the SQL alias (e.g., "t1" for the first occurrence and "t2" from "as t2" in your example) to uniquely identify each occurrence -- we can tweak the representation and explore how to handle this in case of nested queries, etc, but alias is the main concept to track uniqueness. * However, since this leads to a series of open ended problems, I have also suggested avoiding this complexity altogether and not supporting time travel in MVs for now.
However, thinking again, are not time travel queries in MVs self-containing the exact snapshot ID that we are trying to track in the properties? Looks like this information is already encoded in the query and there is no need to capture it externally.
For example, if the MV definition consists of table references where all of the references are bound to specific snapshot IDs or timestamps, then the storage table is always fresh no matter if the underlying tables change. Tracking snapshot IDs in the storage table is only required for table references that are not pinned to a specific snapshot ID/timestamp in the view definition, for which UUID is sufficient.
Thanks, Walaa.
Hi everyone, just want to say I am back from the leave, and currently catching up with the threads, I will make more comments later after knowing more details of what has been going on. Looks like we've made great progress!
Just my 2 cents on the current properties vs metadata field discussion. The proposed properties are: - in view:
1. a boolean flag to indicate a view is a MV 2. a pointer to the storage table - in storage table:
3. view version that is materialized
4. a prefix-based map to describe the snapshot version of the base tables that are materialized
5. a prefix-based map to describe the version of child views that are materialized
For 1, 2, and 3, these are all pretty simple and can be just properties. I guess 4 and 5 are the main ones that seem complex and can be more formalized as metadata fields. I think the time travel cases Bunny brought up might be good ones to look into more details:
For direct version travel, I think the base table version serves as the default. If you have a MV query like
SELECT * FROM t1, t1 FOR SYSTEM_VERSION AS OF 987654 as t2 WHERE t1.c1 = t2.c1
and in the storage table it says t1 maps to snapshot id 123456, then the query is still not ambiguous, it should be interpreted as
SELECT * FROM t1 FOR SYSTEM_VERSION AS OF 123456, t1 FOR SYSTEM_VERSION AS OF 987654 as t2 WHERE t1.c1 = t2.c1
For ref travel, the specific ref version needs to be fixed at MV creation time:
SELECT * FROM t1, t1 FOR SYSTEM_VERSION AS OF '2024-Q1' as t2 WHERE t1.c1 = t2.c1
Just storing table UUID is not sufficient. In a property-based approach, we need something like base.table.<table>.ref.<ref-name>=<snapshot-id>.
Time travel is similar to ref travel:
SELECT * FROM t1, t1 FOR SYSTEM_TIME AS OF timestamp '2024-01-01' as t2 WHERE t1.c1 = t2.c1
In a property-based approach, we need something like base.table.<table>.time.<timestamp>=<snapshot-id>.
Technically this is indeed getting increasingly complex, so I can get why many of us say this property-based approach is quite brittle. However, it seems like it can still work as we extend the property structure. Personally speaking I am leaning more towards the property-based approach just for its simplicity, but I need to think more about other use cases as well.
Best, Jack Ye
I think this is orthogonal to the property vs metadata field since instead of representing the property as `base.table.[UUID]` it would be something like `base.table.[alias]` where `alias` is the specific occurrence of the table in the query according to its alias (and SELECT scope possibly, which kind of opens the door to further complexities, but for the sake of argument -- there is a mapping to properties too).
Another question: assuming we go with the top level metadata model, will we still expose this metadata on the engine side as properties? What would the property names be?
Thanks, Walaa.
Sounds good.
Another benefit of the struct model is that it's more extensible in the future when we need to disambiguate the same table that appears multiple times in the MV query tree. This could happen with time travel queries or branching. We may end up adding additional properties like a sequence number, parent view or path to root.
Thanks
Hi Benny, I have responded to the comment.
I would suggest that we use this thread to evaluate properties model vs top level metadata model (to avoid discussion drift).
If we have feedback on the actual properties used in the properties model as defined in the PR, we can have the discussion there.
THanks, Walaa.
Hi Walaa
Hi Jan,
Thanks Benny
On Wed, May 15, 2024 at 12:06 AM Jan Kaul <jank...@mailbox.org.invalid> wrote:
I agree with Szehon and Benny that storing the lineage
information as multiple table properties is too brittle,
especially for many source tables (base tables). I would prefer to
have the whole lineage information in one entry as it is more
concise. This is also how Trino has been doing it, as you can see
here.
As we've discussed in the google
doc: it is helpful to also store the table identifiers of
the source tables to enable clients to determine the freshness of
the MV that don't understand the SQL dialect of the MV definition,
like other query engines, BI tools and Dataframe libraries. This
is also how Trino is doing it. That's why we chose the design in
the google doc.
Storing the storage table identifier as a property might work.
Thanks, Jan
On 15.05.24 02:38, Walaa Eldin Moustafa
wrote:
Thanks Benny. My specific thoughts about the spec
and the properties are captured in the spec PR https://github.com/apache/iceberg/pull/10280.
The spec is also implemented in the Spark implementation PR https://github.com/apache/iceberg/pull/9830,
and I believe this follows the same nature of how the
information was captured in Netflix's implementation with Spark,
and Trino implementation (prior to formalizing through that
spec), both of which have been used reliably for years. I think
it also aligns with Ryan's feedback here https://github.com/apache/iceberg/issues/6420#issuecomment-1369280546 which
indicated the usage of properties.
The reasons for choosing properties:
* Not every table is a storage table and not every view is
a materialized view. I feel exposing the info as top level
metadata is an overkill for the original object type.
* The properties are simple. They contain either single
snapshot ID each, or single view version each, or lastly, the
storage table identifier. Engines can use them without issues
(also as shown in the implementation).
* To be meaningful, the metadata fields should be captured
in the engine API as well, which is an effort that has to be
pursued outside the Iceberg community. Until engine APIs
evolve, we would have to define a mapping between Iceberg
metadata fields and engine properties (only current place in
engine side to capture this info). This requires an additional
spec on its own, and it will introduce complexities. Hence it
is always cleaner to map Iceberg properties to engine
properties and Iceberg metadata to designated engine APIs.
Mixing and matching (e.g., Iceberg metadata fields as engine
properties) just for the lack of other cleaner options does
not sound like a good idea in both short and long term.
Let me know your thoughts.
Thanks,
Walaa.
I agree with Szheon here. I think storing
the materialization lineage as a bunch of properties is
brittle. This lineage information is needed by engines to
validate the staleness of a materialization and also to
perform full or incremental refreshes. There’s a lot to
capture here.
Maybe we should drill down into the use cases
first - such as incremental refresh with aggregates?
(Pick a harder one first 😀)
I left a few comments about this in the doc.
I wonder what are your thoughts here Walaa?
Thanks
Thanks John. The current metadata does
not sound complex. We need to track the underlying
table snapshot IDs as well as the view version ID. I
agree as long as it is simple and before this feature
fully matures, we should track it in properties.
One important factor for me (apart from the API
effort, especially on the engine side), is that not
each table is an MV storage table. Surfacing
MV-specific storage table properties as first class
table metadata sounds to impose this metadata on
every table, when it is not required for normal
table operation (yes, they can be optional, but it
does not sound like the use case warrants exposing
them as metadata fields yet).
Similarly, since not every view is a materialized
view, it sounds reasonable to keep MV-specific data
in properties.
UUID (for views), on the other hand, is common to
all views, hence it made sense to add it as a top
level field.
Thanks,
Walaa.
Hi Szheon,
While I fully share your concern of abusing
table properties, we took the approach of option
1 and run it in production for several years:
- the feature was still evolving
- quick and simple implementation
- table properties are simple enough and not
confusing
- haven't seen an urgent need to convert the
properties to metadata fields and add API
- do not wish on-disk changes (requiring
lengthy tedious migration)
That said, I am open to codifying the mv
metadata into api and spec, with the following
considerations
- mv metadata has reached maturity and
consensus (could be just a core portion)
- when mv metadata becomes too complex
- wish to support use cases that are quicker
to adopt API changes (than engines), e.g.,
using Iceberg library to manipulate MVs, or
parsing metadata files directly
- Spark view catalog API can evolve
separately from Iceberg API and spec changes
Thanks all for the great discussion!
Hi Szheon,
Thanks for the follow-up. It is
possible some of the concerns were
referring to the backend catalogs, but it
is all connected. My main personal concern
is from the engine connector APIs point of
view, but I share the concern about the
catalogs.
I think everyone's concern is not about
the complexity per backend
catalog/engine catalog API (in which case
adding new metadata to existing objects
could require less code), but rather about
the number of APIs and
implementations that need to change (in
which case both new metadata to existing
objects and new objects altogether
introduce equal complexity).
Thanks,
Walaa.
Hi Walaa
OK thanks for confirming. I am
still not 100% in agreement, my
understanding of the rationale for
separate Table/View objects in the
comment that you linked:
I
think the biggest problem with
this is that we would need to
modify every catalog to support
this combination and that would be
really difficult.
is about JavaCatalogs /REST Catalog
needing to to support creating ,
persisting, and loading a
MaterializedView object, which is much
more complex. See HiveView PR for
example : https://github.com/apache/iceberg/pull/9852
We would have to do the same exercise
for persisting MV.
In our case though, there's not
much complexity regardless of approach
('properties' or new metadata fields),
in terms of Java Catalog/REST
Catalog. It's mostly pass-through to
storage. Looks like you are referring
to Spark's View model in terms of
complexity, which may be a different
story, but not sure if it is a
good rationale to make Iceberg to use
'properties' .
'properties' is for read/write
configurations, not to save
metadatas. To me, its also brittle to
save important metadata, as it's not
in the defined schema.
A
string to string map of table
properties. This is used to
control settings that affect
reading and writing and is not
intended to be used for arbitrary
metadata. For
example, commit.retry.num-retries is
used to control the number of
commit retries.
On the other hand, the Draft Spec
suggests to save `lineage` as a
modeled field on the Storage Table's
snapshot metadata. This allows you
to 'time travel', 'branch', and have
this metadata life cycle integrated
via normal snapshots lifecycle
operations.
So that's my rationale. Not sure
if we can come to an agreement
over email though, and may need
others to chime in as well.
Thanks
Szehon
Hi Szehon,
Yes, you are reading the PR
correctly, and interpreting the
meaning of properties correctly. I
think the reply you pasted from
Ryan refers to the same concept as
well.
For the initial Google doc
and the issue (by the way it is
an issue, not a PR), yes both
are proposing new metadata
fields.
The references I made to the
modeling doc [1, 2] are reasons
why new APIs are not desired.
The cons/concerns applicable to
new MV metadata apply by
extension to new table and view
metadata fields.
The reason why new metadata
adds complexity is that this new
metadata needs to be propagated
to the engine API. For example,
here is the ViewInfo [3] class
in the Spark catalog, which is
used in view methods like
createView. Its
fields correspond with the
Iceberg metadata. Adding new
Iceberg fields should be
accompanied with new fields in
the engine catalog/connector
APIs, which was a major reason
for rejecting the combined MV
object model as well.
Thanks,
Walaa.
Hi Walaa
As there may be
confusion in the word
'properties', I want to
double check if we are
talking about the same
thing here.
| optional |
optional |
properties |
A string to string map of
table
properties. This
is used to
control settings
that affect
reading and
writing and is
not intended to
be used for
arbitrary
metadata. For
example, commit.retry.num-retries is
used to control
the number of
commit retries. |
and adding Storage Table
pointer as key/value pair in
the View's 'properties'
field: https://github.com/apache/iceberg/blob/main/format/view-spec.md?plain=1#L65
| optional |
properties |
A string to string map of
view properties
[2]
|
Is that correct?
On the other hand, I was talking about adding
this metadata as
actual fields, as is
described in the Draft
Spec of the Design
Doc https://docs.google.com/document/d/1UnhldHhe3Grz8JBngwXPA6ZZord1xMedY5ukEhZYF-A and
first PR https://github.com/apache/iceberg/issues/6420 .
Do you mean, the vote
means we cannot model
new fields like
'materialization' and
'lineage' as was
proposed there ? If
that is the
interpretation, I am not
sure I agree. I dont
fully see how new fields
adds more catalog
implementation
complexity over new
key/value properties?
To me, the vote seemed
to just rule out using a
combined catalog object
(MaterializedView) in
favor of re-using the
Table and View metadata
models, not to prevent
change to the Table and
View model.
Thanks
Szehon
Hi
Szehon,
I think
choosing
separate
view + table
objects
precludes us
from adding
new metadata
to table and
view metadata.
Here is one
relevant
comment [1]
from Ryan on
the modeling
doc, where his
point is that
we want to
avoid
introducing
new APIs since
it requires
updating every
catalog, and
(quoting) even
now, we have
few
implementations
that support
views because
of the
problems
updating back
ends.
Therefore, one
of the major
reasons to
avoid a new
model with new
metadata is to
avoid adding
new metadata,
which
introduces
this
complexity.
Here is
another
similar
comment from
Renjie [2] on
the cons
listed for the
combined
object
approach.
Even Ryan's
point on the
MV issue that
you referenced
reads to me as
he is
supportive of
the property
model. Here
are some
quotes:
> We
would still
want some MV
metadata in
table
*properties*.
> I
recommend
instead
reusing the
existing
snapshot
metadata
structure to
store what you
need as
snapshot
*properties*.
> First,
I think we
want to avoid
keeping much
state
information in
complex table
*properties*.
Again,
here, he is
supportive of
table
properties,
but wants to
make sure that
the
information is
simple.
> We may
want
additional
metadata as
well, like a
UUID to ensure
we have the
right view. I
don't think we
have a UUID in
the view spec
yet, but we
could add one.
Here, he is
very specific
when it comes
to new
metadata
fields, and
explicitly
calls it out.
That is the
only new
metadata field
in that reply
and by now it
is already
supported. It
is also not
MV-specific.
Hope this
addresses your
question on
the property
vs new
metadata
model.
Thanks,
Walaa.
Hi
Walaa,
I agree,
I definitely
do not want
yet another
pr/doc where
discussion
happens. as
its already
quite spread
out :) But
did not want
to clarify
some points
before we get
started on the
discussion on
your PR.
With
reusing the
table and view
objects, we
are not
changing the
existing
metadata of
either table
or view spec
but rather
introduce new
properties and
formalize them
to express
materialized
views
On this point,
I am not 100%
sure
that choosing
to represent a
MaterializedView as a separate View + Table object precludes us from
adding to
metadata of
Table or View
as the Draft
Spec
suggested,
though. I
think this
point was
discussed in
Jan's initial
PR with a good
point from
Ryan: https://github.com/apache/iceberg/issues/6420#issuecomment-1369280546 that
using Table
Properties to
track lineage
is fairly
brittle, and
having it
formalized in
the Iceberg
metadata is
cleaner, and
that was thus
the direction
of the Draft
Spec in the
design doc.
What do people
think?
Thanks
Szehon
Thanks
Szehon.
The
reason for the
difference is
that the
proposal in
the Google doc
is based on a
new MV model,
hence, new
metadata
fields and a
new metadata
model were
being
introduced
(with types,
optionality,
etc). With
reusing the
table and view
objects, we
are not
changing the
existing
metadata of
either table
or view spec
but rather
introduce new
properties and
formalize them
to express
materialized
views. This
would be the
answer to most
of the
questions you
posted on the
PR (besides
some naming
questions,
which I think
should be
straightforward).
With that
fundamental
difference, we
cannot lift
and shift what
is in the doc
to any PR.
Further,
having
consensus on
separate table
and view
objects
contradicts
with the point
being made on
having
consensus on
the doc. We
might have had
agreements on
some elements,
but definitely
not on the
whole doc,
proven by the
follow ups
(also as a
community, not
individuals).
Therefore:
we need a new
space to
discuss the
separate table
and view
properties.
Is the
question
whether to:
1- Create
a new doc
2- Create
a new PR?
I feel a
PR is the most
effective way,
especially
given the fact
that we
discussed the
topic a lot by
now. If we
agree, we can
continue the
discussion on
the PR, else,
we can create
a doc.
Thanks,
Walaa.
Thanks
Walaa for
driving it
forward,
looking
forward to
thinking about
implementation
of
Materialized
Views.
I see Jan's
point, the PR
spec change is
similar but
does not seem
to be
completely aligned
with the Draft
Spec in the
design doc: https://docs.google.com/document/d/1UnhldHhe3Grz8JBngwXPA6ZZord1xMedY5ukEhZYF-A/
. I left my
comments on PR
of those
sections with
the links to
the
difference. I
think most of
those Draft
Spec
proposal is
still
applicable
after the
decision to
have separate
Table and View
objects It
will be
interesting to
at least see
drill a bit
further why we
did not choose
the approach
in the Draft
Spec and chose
another way.
Thanks
Szehon
Well,
everybody that
actively
contributed to
the discussion
on the
original
google doc was
in consensus.
That's why I
brought up the
topic at the
Community Sync
on the
2024-02-14 (https://youtu.be/uAQVGd5zV4I?t=890)
to raise the
awareness of
the broader
community.
After which
the discussion
about the
storage model
started. I
don't think
that the
discussion
about a single
aspect of a
proposal
should
invalidate all
other aspects
of the
proposal.
Regardless,
the state of
the proposal
from the
original
google doc
contains a lot
of valuable
contributions
from Micah,
Szehon, Jack,
Dan, yourself
and others and
it should at
least provide
the basis for
any further
discussion. I
don't think
it's effective
to start with
a completely
different
design because
we are bound
to have the
same
discussions
all over
again.
Thanks, Jan
On
08.05.24
12:11, Walaa
Eldin Moustafa
wrote:
The
only consensus
the community
had was on the
object model
through the
most recent
voting thread
[1]. This kind
of consensus
was not
present during
the doc
discussions,
and this
should be
evident from
the fact the
last doc state
listed 5
alternatives with
no particular
conclusion. I
am not quite
sure what type
of consensus
we are
referring to
here given all
the follow up
discussions,
alternatives,
etc.
Due to
the
separate object
model, the PR
is
fundamentally
different from
the doc in the
sense it does
not propose a
new metadata
model but
rather
formalizes
some new table
and view
properties
related to
MVs. That is
also one
reason there
are no
repeated
discussions.
That said, if
you feel there
is a repeated
discussion
(which I do
not see so
far), it would
be best to
link the
relevant
discussion
from the doc
in a comment.
Happy to
move the
discussion elsewhere
if there is
sufficient support
for this idea,
but as things
stand, I do
not see this
as an
efficient way
to make
progress. It
sounds we have
been
re-emphasizing
the same
points in the
last two
replies, so I
will let
others chime
in at this
point.
Thanks,
Walaa.
The original google doc discussed
multiple
aspects of the
Materialized
View spec. One
was the
storage model
while others
were related
to the
metadata.
After we
(Micah,
Szehon, you,
me) reached
consensus in
the google
doc, Jack
raised his
concern about
the storage
model and the
long
discussion
about the
storage model
started. Now
we truly
reached
consensus
about the
storage model,
which is now
also reflected
in the google
doc. All other
aspects from
the google doc
about the
metadata
weren't
questioned and
still
represent the
consensus.
I would like
to avoid
repeating the
discussions
in your PR
that we
already had in
the google
doc.
Especially
since we
reached
consensus
which took a
considerable
amount of
time.
Thanks, Jan
On
08.05.24
10:21, Walaa
Eldin Moustafa
wrote:
Thanks
Jan. I think
we moved on to
more alignment
steps beyond
that doc a
while ago.
After that
doc, we have
discussed the
topic further
in 2 dev list
threads and one more doc (with strictly
two options
for the
storage model
to consider).
Moreover, the
original doc
grew to 14
pages long
with
one section
comparing 5
design
alternatives,
which
made things
harder to
reach
consensus. The
lack of
consensus is
what partly
led up to the
subsequent
discussions
and call for a
more focused
approach to
reach
consensus. If
we already
have a
consensus on
the storage
model
(separate
tables and
views), I
think we
should take
things further
and have
continued
focused
discussions on
the specific
metadata in
the form of a
PR. I have
included all
previous
discussions
including the
original doc
and issue as
references in
the PR
description.
Please let me
know if this
works. Happy
to hear
others'
thoughts on
the best way
to move
forward.
Thanks,
Walaa.
Thanks
Walaa for
trying to move
things along.
However I
don't think
it's a good
idea to start
a separate
discussion
about the
metadata for
materialized
views because
we already had
this
discussion and
reached
consensus in
this google
doc:
https://docs.google.com/document/d/1UnhldHhe3Grz8JBngwXPA6ZZord1xMedY5ukEhZYF-A/edit?usp=sharing
Once the
draft is
finalized we
can adopt the
PR to reflect
the consensus
from the
google doc.
Best wishes,
Jan
On
07.05.24
19:11, Walaa
Eldin Moustafa
wrote:
Thanks
Steven. I feel
it is needed
so the MV spec
is not
scattered
across the
table and view
spec pages. We
may add a
reference in
each
respective
properties section.
Walaa,
thanks for
initiating the
next step.
With
the agreed model
of separate
view
and storage
table, I am
wondering if a
separate
materialized
view spec page
is needed.
E.g., the new
view metadata
(view-materialized and view-storage-table) is probably good to be added
to the view
page directly
to avoid
information
scattering.
The same can
be said about
the storage
table
metadata.
We may
keep the
separate
materialized
view page to
document
motivation,
freshness
semantics,
etc..
Hi
Everyone,
Thanks
again for
participating
in the
modeling
discussion
[1]. Since the
outcome of
this
discussion was
to model
materialized
views as
separate
objects, an
Iceberg view
and a table, I
think the next
step should be
discussing the
metadata
details for
each object. I
have created a
PR https://github.com/apache/iceberg/pull/10280
with an
initial spec
improvement.
Please feel
free to review
it and leave
feedback
there.
Thanks,
Walaa.
--
|
