Hi Walaa, I personally don't see a semantic issue with putting the table identifiers in the refresh state. The purpose of the refresh state is to basically take a snapshot of the table and view versions at the time of materialization. Directly using table identifiers seems pretty natural to me. So, I'm +1 for:
no lineage + refresh-state key = identifier Thanks Benny On Mon, Aug 19, 2024 at 9:19 PM Walaa Eldin Moustafa <wa.moust...@gmail.com> wrote: > Hi Micah, it is mostly about the typical results of denormalization such > as data consistency, management complexity, integrity, etc. However, as > mentioned earlier, the main reason would be the semantic gap around using > catalog table identifiers as a concept in the table (more specifically > snapshot summary) spec. Denormalization in this case is a minor issue. > > Thanks, > Walaa. > > > On Mon, Aug 19, 2024 at 4:44 PM Micah Kornfield <emkornfi...@gmail.com> > wrote: > >> Thanks Micah, for the latter, I meant the type of denormalization of >>> repeating a 3-part name as opposed to using an ID. >> >> >> Is the concern here just metadata size or something else? For size I >> think if this is really anticipated to be a problem that it is likely for >> the state map in general, and we could investigate some more sophisticated >> encodings for the State map even without the overlap. I think maybe this >> could be handled if it proves to be a problem but hopefully engines are >> placing a reasonable cap on view depth + number of tables per view which >> puts an upper bound on overall size. >> >> Thanks, >> Micah >> >> On Fri, Aug 16, 2024 at 4:56 PM Walaa Eldin Moustafa < >> wa.moust...@gmail.com> wrote: >> >>> Thanks Micah, for the latter, I meant the type of denormalization of >>> repeating a 3-part name as opposed to using an ID. >>> >>> On Fri, Aug 16, 2024 at 4:52 PM Micah Kornfield <emkornfi...@gmail.com> >>> wrote: >>> >>>> However, this still does not address the semantic issue which is more >>>>> fundamental in my opinion. The Iceberg table spec is not aware of catalog >>>>> table identifiers and this use will be the first break of this >>>>> abstraction. >>>> >>>> >>>> IIUC, based on Jan's comments, we are not going to modify the table >>>> specification. I thought the state map was effectively opaque metadata >>>> from the table specification perspective? If this is the case I feel like >>>> that is OK and not a blocker, I think by their nature as already discussed >>>> MVs need catalog information to function properly and the choice to put >>>> catalog information into the table metadata is pragmatic and preserves >>>> other desirable properties. It might be a more important point if we want >>>> to update the table specification (IMO, I still think it would probably be >>>> OK). >>>> >>>> On a side note, it does not address the denormalization issue either if >>>>> we ever want to introduce the lineage in the view as a nice-to-have. >>>> >>>> >>>> I think if lineage is introduced to the View metadata, it should only >>>> hold direct dependencies for the reasons already discussed. IMO, I think >>>> the potential overlap is OK as they serve two different purposes. >>>> >>>> Cheers, >>>> Micah >>>> >>>> >>>> >>>> >>>> >>>> On Fri, Aug 16, 2024 at 4:27 PM Walaa Eldin Moustafa < >>>> wa.moust...@gmail.com> wrote: >>>> >>>>> That is right. I agree that in the case of using catalog identifiers >>>>> in state information, using them in lineage information would be a >>>>> nice-to-have and not a requirement. >>>>> >>>>> However, this still does not address the semantic issue which is more >>>>> fundamental in my opinion. The Iceberg table spec is not aware of catalog >>>>> table identifiers and this use will be the first break of this >>>>> abstraction. >>>>> >>>>> On a side note, it does not address the denormalization issue either >>>>> if we ever want to introduce the lineage in the view as a nice-to-have. >>>>> >>>>> Thanks, >>>>> Walaa. >>>>> >>>>> >>>>> On Fri, Aug 16, 2024 at 10:09 AM Jan Kaul <jank...@mailbox.org.invalid> >>>>> wrote: >>>>> >>>>>> Hi Walaa, >>>>>> >>>>>> I would argue that for the refresh operation the query engine has to >>>>>> parse the query and then somehow execute it. For a full refresh it will >>>>>> directly execute the query and for a incremental refresh it will execute >>>>>> a >>>>>> modified version. Therefore it has to fully expand the query tree. >>>>>> >>>>>> Best wishes, >>>>>> >>>>>> Jan >>>>>> >>>>>> Am 16.08.2024 18:13 schrieb Walaa Eldin Moustafa < >>>>>> wa.moust...@gmail.com>: >>>>>> >>>>>> Thanks Jan for the summary. >>>>>> >>>>>> For this point: >>>>>> >>>>>> > For a refresh operation the query engine has to parse the SQL and >>>>>> fully expand the lineage with it's children anyway. So the lineage is >>>>>> not >>>>>> strictly required. >>>>>> >>>>>> If the lineage is provided at creation time by the respective engine, >>>>>> the refresh operation does not need to parse the SQL, correct? >>>>>> >>>>>> Thanks, >>>>>> Walaa. >>>>>> >>>>>> On Fri, Aug 16, 2024 at 12:24 AM Jan Kaul <jank...@mailbox.org.invalid> >>>>>> wrote: >>>>>> >>>>>> As the table I created is not properly shown in the mailing list I'll >>>>>> reformat the summary of the different drawbacks again: >>>>>> >>>>>> Drawbacks of (no lineage, refresh-state key = identifier): >>>>>> >>>>>> - introduces catalog identifiers into table metadata (#4) >>>>>> - query engine has to expand lineage at refresh time (happens anyway) >>>>>> >>>>>> Drawbacks of (normalized lineage, refresh-state key = uuid): >>>>>> >>>>>> - recursive calls to catalog to expand lineage at read time (#2) >>>>>> - fragile by requiring child views to have lineage field >>>>>> >>>>>> Drawbacks of (denormalized lineage, refresh-state key = uuid): >>>>>> >>>>>> - update of materialized view version required if child view is >>>>>> updated (#5) >>>>>> On 16.08.24 09:17, Jan Kaul wrote: >>>>>> >>>>>> Hi, >>>>>> >>>>>> Thanks Micah for clearly stating the requirements. I think this gives >>>>>> better clarity for the discussion. >>>>>> >>>>>> It seems like we don't have a solution that satisfies all >>>>>> requirements at once. So we would need to choose which has the fewest >>>>>> drawbacks. >>>>>> >>>>>> I would like to summarize the different drawbacks that came up in the >>>>>> discussion. >>>>>> no lineage >>>>>> + refresh-state key = identifier >>>>>> normalized lineage >>>>>> + refresh-state key = uuid >>>>>> denormalized lineage >>>>>> + refresh-state key = uuid >>>>>> - introduces catalog identifiers into table metadata (#4) >>>>>> - query engine has to expand lineage at refresh time (happens anyway) >>>>>> - recursive calls to catalog to expand lineage at read time (#2) >>>>>> - fragile by requiring child views to have lineage field >>>>>> - update of materialized view version required if child view is >>>>>> updated (#5) >>>>>> >>>>>> With identifiers as the refresh-state keys, the lineage is not >>>>>> strictly required and becomes an orthogonal proposal. That's why I left >>>>>> it >>>>>> out if the comparison. >>>>>> >>>>>> In my opinion introducing catalog identifiers into the table metadata >>>>>> (requirement #4) is the least significant drawback as it is not a >>>>>> technical >>>>>> reason but more about semantics. Especially as the identifiers are not >>>>>> introduced into the table spec but are rather stored in the snapshot >>>>>> summary. That's why I'm in favor of using the catalog identifiers as the >>>>>> refresh-state keys. >>>>>> >>>>>> Regarding your last point Walaa: >>>>>> >>>>>> The option of using catalog identifiers in the state map still >>>>>> requires keeping lineage information in the view because REFRESH MV needs >>>>>> the latest fully expanded children (which could have changed from the set >>>>>> of children currently in the state map), without reparsing the view tree. >>>>>> >>>>>> For a refresh operation the query engine has to parse the SQL and >>>>>> fully expand the lineage with it's children anyway. So the lineage is >>>>>> not >>>>>> strictly required. >>>>>> >>>>>> If I understand correctly, most of you are also in favor of using >>>>>> catalog identifiers + ref as the refresh-state keys and postponing the >>>>>> lineage proposal. >>>>>> >>>>>> I hope that we can move the discussion forward. >>>>>> >>>>>> Jan >>>>>> On 16.08.24 08:07, Walaa Eldin Moustafa wrote: >>>>>> >>>>>> The option of using catalog identifiers in the state map still >>>>>> requires keeping lineage information in the view because REFRESH MV needs >>>>>> the latest fully expanded children (which could have changed from the set >>>>>> of children currently in the state map), without reparsing the view tree. >>>>>> Therefore, catalog identifiers in the state map, does not eliminate the >>>>>> need for tracking children in the form of catalog identifiers in the >>>>>> lineage side (but in this case lineage will be a set instead of just a >>>>>> map). >>>>>> >>>>>> Hence, my concerns with using catalog identifiers (as opposed to >>>>>> UUIDs) are: >>>>>> * The fundamental issue where the table spec depends on/refers to the >>>>>> view spec (because such catalog identifiers are not defined in the table >>>>>> spec and the only place they have a meaning is in the view spec lineage >>>>>> information). >>>>>> * (less fundamental) The denormalization introduced by this >>>>>> arrangement, where each identifier is 3-parts and all of them repeat in >>>>>> both lineage info and state map. >>>>>> >>>>>> I am not very concerned with recursive expansion (through multiple >>>>>> calls), as it is always the case with views. >>>>>> >>>>>> On a positive note, looks like we agree to move past sequence numbers >>>>>> :) >>>>>> >>>>>> Thanks, >>>>>> Walaa. >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> On Thu, Aug 15, 2024 at 4:07 PM Micah Kornfield < >>>>>> emkornfi...@gmail.com> wrote: >>>>>> >>>>>> I think given the constraint that catalog lookup has to be by >>>>>> identifier and not UUID, I'd prefer using identifier in the refresh >>>>>> state. >>>>>> If we use identifiers, we can directly parallelize the catalog calls to >>>>>> fetch the latest state. If we use UUID, the engine has to go back to the >>>>>> MV and possibly additional views to reconstruct the lineage map. It's >>>>>> just >>>>>> a lot slower and more work for the engine when there is a MV that >>>>>> references a lot of views (and those views reference additional views). >>>>>> >>>>>> >>>>>> I'm +1 on using catalog identifiers as the key. As you point out >>>>>> this is inline with #2 (try to minimize serial catalog lookups) in >>>>>> addition >>>>>> to supporting requirement #3. >>>>>> >>>>>> On Thu, Aug 15, 2024 at 3:27 PM Benny Chow <btc...@gmail.com> wrote: >>>>>> >>>>>> I think given the constraint that catalog lookup has to be by >>>>>> identifier and not UUID, I'd prefer using identifier in the refresh >>>>>> state. >>>>>> If we use identifiers, we can directly parallelize the catalog calls to >>>>>> fetch the latest state. If we use UUID, the engine has to go back to the >>>>>> MV and possibly additional views to reconstruct the lineage map. It's >>>>>> just >>>>>> a lot slower and more work for the engine when there is a MV that >>>>>> references a lot of views (and those views reference additional views). >>>>>> >>>>>> Thanks >>>>>> Benny >>>>>> >>>>>> >>>>>> On Thu, Aug 15, 2024 at 2:14 PM Walaa Eldin Moustafa < >>>>>> wa.moust...@gmail.com> wrote: >>>>>> >>>>>> Thanks Jan, Micah, and Karuppayya for chiming in. >>>>>> >>>>>> I do not think 3 and 4 are at odds with each other (for example >>>>>> maintaining both lineage map and state map through UUID can achieve >>>>>> both). >>>>>> Also, I do not think we can drop the lineage map since in many catalogs, >>>>>> the only lookup method is by the catalog identifier, and not the UUID. >>>>>> >>>>>> I think if we go with UUIDs in the state, we should have a lineage >>>>>> map (from identifiers to UUIDs) to go with it. >>>>>> >>>>>> Thanks, >>>>>> Walaa. >>>>>> >>>>>> >>>>>> On Thu, Aug 15, 2024 at 1:45 PM karuppayya <karuppayya1...@gmail.com> >>>>>> wrote: >>>>>> >>>>>> +1 to storing the refresh state as a map of UUIDs to snapshot IDs, >>>>>> and deferring the inclusion of lineage to a future iteration.(like Micha >>>>>> mentioned) >>>>>> This would greatly simplify the current design. >>>>>> >>>>>> Also in terms of identifiers to use(UUID or catalog identifier) for >>>>>> the refresh state >>>>>> We will not be able to fetch the table/View using the UUID alone, for >>>>>> example from Hive based catalog. >>>>>> We do not have the direct mapping between UUID and table/view. >>>>>> Which leaves us only with the catalog identifiers? >>>>>> >>>>>> Thanks & Regards >>>>>> Karuppayya >>>>>> >>>>>> >>>>>> On Thu, Aug 15, 2024 at 9:16 AM Micah Kornfield < >>>>>> emkornfi...@gmail.com> wrote: >>>>>> >>>>>> I think it might be worth restating perceived requirements and making >>>>>> sure there is alignment on them. >>>>>> >>>>>> If I am reading correctly, I think the following are perceived >>>>>> requirements: >>>>>> 1. An engine must be able to unambiguously detect that an underlying >>>>>> queried entity has changed or not via metadata to decide if materialized >>>>>> table data can be used. >>>>>> 2. The number of sequential catalog reads an engine needs to make to >>>>>> make use of a materialized table state at read time is minimized. >>>>>> 3. Engines that don't understand a SQL dialect can still use MV >>>>>> information if it is not stale. >>>>>> 4. Table refs (catalog identifiers) should not appear in the >>>>>> materialized table metadata (i.e. state). >>>>>> 5. The view part of the MV definition should not need a new revision >>>>>> for any changes to objects it queries as long as their schemas stay >>>>>> compatible (only state information on the materialized table need to >>>>>> change). >>>>>> >>>>>> In my mind, requirement 1, is the only true requirement. I think >>>>>> this necessitates having UUID + snapshot ID as part of the state >>>>>> information (not necessarily part of the Lineage). I think it also >>>>>> necessitates having a denormalized view of all entities that are inputs >>>>>> into the MV in the state information (a view object might not change but >>>>>> its underlying tables or views could change and that must be detected). >>>>>> >>>>>> Requirements 2 and 5 are somewhat at odds with each other. If >>>>>> information is denormalized (fully expanded) in Lineage, it means if >>>>>> table >>>>>> information is somehow dropped from an intermediate view, one would need >>>>>> to >>>>>> update the view (or make excess calls to the catalog). In my mind, this >>>>>> argues for normalization of the lineage stored on the view (with the cost >>>>>> of potentially 1 additional serial catalog lookup once the state >>>>>> information is retrieved). >>>>>> >>>>>> I think #3 is at odds with #4. I think #3 is more worthwhile, then >>>>>> keeping #4 (and as Jan noted #4 adds complexity). >>>>>> >>>>>> I think the last remaining question is if lineage serves any >>>>>> purpose. I think it is useful for the following reasons: >>>>>> a) When there are no intermediate views queried, it allows for fully >>>>>> parallelized lookup calls to the catalog without having to parse the SQL >>>>>> statement first >>>>>> b) Allows tools that don't need to lookup state information or >>>>>> parse SQL but still navigate MV/view trees. >>>>>> >>>>>> Both of these seem relatively minor, so lineage could perhaps be left >>>>>> out in the first iteration. >>>>>> >>>>>> As it applies to Jan's questions: >>>>>> >>>>>> 1. Should we move the identifiers out of the refresh-state into a new >>>>>> lineage record that is stored as part of the view metadata? >>>>>> >>>>>> No, I don't think so, I think #5 is a reasonable requirement and I >>>>>> think this violates it. >>>>>> >>>>>> >>>>>> 2. If yes, should the lineage in the view be fully expanded? >>>>>> >>>>>> No, I think only the state should be fully expanded (for reasons >>>>>> mentioned above, it potentially requires more updates to the view then >>>>>> necessary). >>>>>> >>>>>> >>>>>> 3. What should be used as an identifier in the lineage to reference >>>>>> entries in the refresh-state? >>>>>> >>>>>> >>>>>> Catalog identifiers make sense to me. If we agree requirement #3 is >>>>>> not a requirement then it seems like this could also be UUIDs. >>>>>> >>>>>> Thanks, >>>>>> Micah >>>>>> >>>>>> On Thu, Aug 15, 2024 at 7:57 AM Benny Chow <btc...@gmail.com> wrote: >>>>>> >>>>>> If we go with either UUID or Table Identifier + VersionID/SnapshotId >>>>>> in the refresh state, then this list is fully expanded already. So, to >>>>>> validate the freshness of a materialization, the engine doesn't even need >>>>>> to look at the view lineage. IMO, the view lineage is nice to have but >>>>>> not >>>>>> a necessary requirement for MVs. The view lineage makes sharing of views >>>>>> between engines without common SQL dialects possible. >>>>>> >>>>>> Benny >>>>>> >>>>>> On Thu, Aug 15, 2024 at 12:22 AM Jan Kaul >>>>>> <jank...@mailbox.org.invalid> <jank...@mailbox.org.invalid> wrote: >>>>>> >>>>>> Hi all, >>>>>> >>>>>> I would like to reemphasize the purpose of the refresh-state for >>>>>> materialized views. The purpose is to determine if the precomputed data >>>>>> is >>>>>> fresh, stale or invalid. For that the current snapshot-id of every table >>>>>> in >>>>>> the query tree has to be fetched from the catalog by using its full >>>>>> identifier and ref. Additionally the refresh state stores the snapshot-id >>>>>> of the last refresh. >>>>>> >>>>>> To summarize: *To determine the freshness of the precomputed data we >>>>>> require the full identifier + ref and snapshot-id of the last refresh for >>>>>> every table in the fully expanded query tree* >>>>>> >>>>>> This is a requirement from how the catalog works and independent from >>>>>> how we design the lineage/refresh state. Additionally we previously >>>>>> agreed >>>>>> that we should be able to obtain the full list of identifiers without >>>>>> needing to parse the SQL definition. >>>>>> >>>>>> Now we are having a discussion in how to store and obtain the fully >>>>>> expanded list of table identifiers and snapshot-ids. To move the >>>>>> discussion >>>>>> forward I think it would be valuable to answer the following 3 questions: >>>>>> >>>>>> 1. Should we move the identifiers out of the refresh-state into a new >>>>>> lineage record that is stored as part of the view metadata? >>>>>> >>>>>> 2. If yes, should the lineage in the view be fully expanded? >>>>>> >>>>>> 3. What should be used as an identifier in the lineage to reference >>>>>> entries in the refresh-state? >>>>>> >>>>>> 1. Question: >>>>>> >>>>>> We already agreed that this would be a good idea because we wouldn't >>>>>> introduce the identifier concept to the table metadata. However, looking >>>>>> at >>>>>> the complexity that comes with the alternatives, I would like to keep >>>>>> this >>>>>> question open. >>>>>> >>>>>> 2. Question: >>>>>> >>>>>> I'm against using a not fully expanded lineage in the view struct. To >>>>>> recall we require every identifier in the fully expanded query tree to >>>>>> determine the freshness. Not storing all identifiers in the lineage would >>>>>> mean to recursively call the catalog and expand the query tree at read >>>>>> time. This can lead to a large overhead for determining the refresh state >>>>>> compared to expanding the query tree once at creation time and then >>>>>> storing >>>>>> the fully expanded lineage. >>>>>> >>>>>> 3. Question: >>>>>> >>>>>> This depends on Question 2. >>>>>> >>>>>> For a not fully expanded lineage, the only options would be uuids or >>>>>> catalog identifiers. >>>>>> >>>>>> For a fully expanded lineage the question isn't all that relevant. >>>>>> The current design specifies that the lineage is a map from an identifier >>>>>> to an id and the refresh-state is a map from such id to a snapshot-id. >>>>>> For >>>>>> this to work we don't have to specify which kind of identifier has to be >>>>>> used. One query engine could use uuids, the other engine sequence-ids. >>>>>> The >>>>>> important assumption we are making is that every id that is used in the >>>>>> refresh-state has to be defined in the lineage. >>>>>> So the question about using uuids is rather, can the query engine >>>>>> trust that the id defined in the lineage is the uuid of the table. >>>>>> >>>>>> >>>>>> Regarding the complexity that comes from introducing the lineage in >>>>>> the view I would like to revisit question 1. Introducing the lineage in >>>>>> the >>>>>> view metadata opens up the question of when should the lineage be fully >>>>>> expanded. We see that we have 3 options: >>>>>> >>>>>> 1. Not fully expanded lineage -> Expansion at read time >>>>>> >>>>>> 2. Fully expanded lineage -> Expansion at creation time >>>>>> >>>>>> 3. No lineage (use identifiers in refresh-state) -> Expansion at >>>>>> refresh time >>>>>> >>>>>> As reading is expected to be the most frequent operation I see option >>>>>> 1 as not favorable. As the query engine has to fully expand the query >>>>>> tree >>>>>> for a refresh anyway, I see option 3 as the most natural. For a refresh >>>>>> operation the query engine must understand the SQL dialects of all views >>>>>> in >>>>>> the query tree and therefore is guaranteed to successfully expand the >>>>>> lineage. This might not be the case at creation time, which makes option >>>>>> 2 >>>>>> less favorable. >>>>>> >>>>>> As can be seen, I'm in favor of just storing the refresh-state as a >>>>>> map from identifier to snapshot-id and not using the lineage. I know that >>>>>> this introduces the concept of a catalog identifiers to the table >>>>>> metadata >>>>>> spec, but in my opinion it is by far the simplest option. >>>>>> >>>>>> I'm interested in your opinions. >>>>>> >>>>>> Best wishes, >>>>>> >>>>>> Jan >>>>>> On 14.08.24 22:24, Walaa Eldin Moustafa wrote: >>>>>> >>>>>> Thanks Benny. For refs, I am +1 to represent them as UUID + optional >>>>>> ref, although we can iterate ohe exact JSON structure (e.g., another >>>>>> option >>>>>> is splitting for (UUID) state from (UUID + ref) state into two separate >>>>>> higher-level fields). >>>>>> >>>>>> Generally agree on REFRESH VIEW strategy could be up to the engine, >>>>>> but it seems like an area where Iceberg could have an opinion/spec on. I >>>>>> will start a separate thread for that. >>>>>> >>>>>> Thanks, >>>>>> Walaa. >>>>>> >>>>>> >>>>>>
