View Spec implementation

[c-thiel]

Implementation of the Iceberg V1 ViewMetadata.

My plan of heading forward:

1. Get this PR merged which just contains the ViewSpec and is not very usable on its own. (Similar to TableSpec which is missing the complete builder also)
2. Add TableMetadataBuilder including partition binding in separate PR
3. Add full ViewMetadataBuilder in separate PR

[ZENOTME]

Thanks! Nice work! @c-thiel

[c-thiel]

@ZENOTME one feature I did not implement yet is respecting "version.history.num-entries" as mentioned in the View Spec. I noticed that the table implementation also doesn't implement it. I don't think it matters much unless one is implementing a catalog, but I wanted to mention it.

[Fokko]

Thanks for working on this @c-thiel. Out of curiosity, and probably this is a broader discussion, but currently, the view spec is around SQL representation. This might not be the most natural fit with dataframe-based systems. Do you have any thoughts on how to represent views in iceberg-rust?

[c-thiel]

@Fokko thats is a hard topic.

The idealist in me would like to eventually see something like substrait beeing used.
Adoption of the project is very slow across engines though.

Maybe the more practical short-term solution would be to stick to representations and even limit to one dialect - the one of the "writer", including its technology. Then each client can decide if there is a transpiler good enough for that dialect.
Funnily enough exactly this ambiguity - that there could be different dialects in the representation (say spark and trino) - gave me a lot of headaches. Which should I use if I am Datafusion? From which should I transpile? What to do if they are not identical?

Sticking to multiple representations is in my opinion only an option if we let the catalog handle this complexity.
Unless substrait gets a significant boost, I would assume that the following approach presents the most stable API (Assuming there is only REST Catalog left ;) ):

• A client would create a view as it does today - however limited to a single representation
• Upon read, a client would additionally present a set of "understood" dialects. The catalog can either serve them or it can't.

This way a Rest-Catalog could even switch to Substrait internally in the future and start now with storing the presented SQL "as-is" without beeing able to serve any other dialect.

With SQLMesh coming up and also Substrait continuing there is at least some development in the area.

[c-thiel]

@Fokko from my side this is good to merge - types are complete and tests are passing.

[c-thiel]

@Fokko @ZENOTME
are there any points open from your side that prevent us to merge the View Spec?
If so, please let me know :)

[Fokko]

The idealist in me would like to eventually see something like substrait beeing used.

There is an open discussion on evolving the spec into something like that. I think transpiling SQL will never work in the long run because there are so many variations and extensions to the SQL dialect (custom UDFs and such), maybe you can reach 90% of the SQL which is good enough.

@nastra would you have time to go over this PR?

[nastra]

it would be good to have more JSON files for testing (not just the happy case)

[nastra]

also in general I think this needs more extensive testing. For example on the java side we have https://github.com/apache/iceberg/tree/5ea78e3fbe5d8c9c846a1b86bcd2b77d13a31acd/core/src/test/java/org/apache/iceberg/view.

Specifically in TestViewMetadata we have a bunch of test cases that would be good to cover here as well

[c-thiel]

Ok, thanks for the feedback @nastra. I'll add some more tests next week.

[c-thiel]

@nastra I added more tests that cover non-happy cases.
However, there isn't much in https://github.com/apache/iceberg/tree/5ea78e3fbe5d8c9c846a1b86bcd2b77d13a31acd/core/src/test/java/org/apache/iceberg/view we can use for now. The ViewMetadataBuilder I started with is on the same level as the TableMetadataBuilder - it can do almost nothing except changing the UUID. The functions tested in the java part are simply not implemented yet. We do have a more complete implementation over at our Iceberg catalog and want to PR them here (lakekeeper/lakekeeper#46), I just want to start with the foundation before heading into the more controversial builder bits.
Would this be alright for you?

Besides the Metadata-Builder, I would like to create 2 follow-up issues:

• Currently in Snapshots we use a fallible method for the timestamp() method. For views I am using a non-fallible version, creating a small inconsistency. I would create a new issues to remove unwrap from timestamps where they are used.
• Allow for empty namespaces, following our discussion below. I am not sure exactly how to handle this, but right now Rust does not allow the creation of the empty Namespace as used in spark (

  iceberg-rust/crates/iceberg/src/catalog/mod.rs

  Line 111 in 48f9e3e

  if names.is_empty() {

  ).

[nastra]

However, there isn't much in https://github.com/apache/iceberg/tree/5ea78e3fbe5d8c9c846a1b86bcd2b77d13a31acd/core/src/test/java/org/apache/iceberg/view we can use for now.

In the java code we typically don't construct the full JSON just to test something, but there are plenty of tests in TestViewMetadata to make sure schema / view version re-assignment / de-duplication happens or SQL dialect deduplication behaves correctly.

I just want to start with the foundation before heading into the more controversial builder bits.

I was merely asked to look at this view implementation since I did the Java implementation, but I'm not involved in iceberg-rust, so I'll let others decide how to proceed with adding views to iceberg-rust and what to include in this PR vs in follow-ups.
Regarding follow-ups, I think a few more things would be needed for the view metadata builder, which I mentioned above. `TestViewMetadata´ gives a good overview over how the view metadata / builder should behave.

Thanks for working on this btw 💯

[c-thiel]

Ok, thanks Eduard for the Feedback.

My preferred way of going forward would be:

1. Merge this PR which just contains the structs & (de)serialization (very much like the TableMetadaData in its current state)
2. Figure out a good way for the TableMetadataBuilder including partition binding. We have a first shot ready that we can create a PR for.
3. Use the same pattern we use for the TableMetadataBuilder for the much lighter ViewMetadataBuilder which then includes all the tests Eduard mentioned.

I am aware that a lot of things are missing in the builder. It was a deliberate decision from me to get views up-to-speed with tables first, and then in a second step extend both tables and views features to the java level. This way we have a handleable PR here that just takes care or (ser)de and can build on it in a next step.

@ZENOTME , @nastra would that be OK for you?

[c-thiel]

@nastra, @Fokko during testing we found a Problem with the "default-namespace". I am hoping for some insights from your side:
According to the iceberg view spec, "default-namespace" is required: https://iceberg.apache.org/view-spec/#versions.
As such it is modeled as NamespaceIdent:
https://github.com/c-thiel/iceberg-rust/blob/d8a7aabce73888e5b22712d60ca95b856db66fc2/crates/iceberg/src/spec/view_version.rs#L56

Valid Namespace Identifiers have at least one element, which is validated by the NamespaceIdent constructor.

When creating views, spark creates Metadata objects that specify the "default-namespace" field but as an empty vec. This is a quite unlucky situation, I am not sure what the desired behavior is:

1. My feeling is that this is a bug in the Iceberg Spec. [] as "default-namespace" provides the same information as a non-required field (null), as a namespace without an element is unusable. However, views in spark work, which shows that default-namespace is not always required. Thus it should be optional.
2. It's a bug in the spark implementation and in fact "default-namespace" should always contain at least one element.
3. They are both right, in which case we would except [] as a valid namespace and must change the rust implementation.
4. [] is in general not a valid namespace unless it is used as a "default-namespace" in which some special behavior is expected. This however brings me back to 1 - why not just use null in this case?

[nastra]

When creating views, spark creates Metadata objects that specify the "default-namespace" field but as an empty vec

Is this with plain OSS Spark or are you using the iceberg-spark-runtime.jar from Iceberg? Just asking, because we're properly handling the namespace when creating a view in https://github.com/apache/iceberg/blob/2a39af894f4f00aa37922ef765cc2583517fa1d1/spark/v3.5/spark-extensions/src/main/scala/org/apache/spark/sql/execution/datasources/v2/CreateV2ViewExec.scala#L51. This code then calls https://github.com/apache/iceberg/blob/81b3310ab469408022cc14af51257b7e8b36614f/spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/SparkCatalog.java#L593.

It would ne helpful to understand which Spark + Iceberg version you're using and how you're creating the view in Spark.

[twuebi]

Hi @nastra,

this script:

import pyspark
from pyspark.conf import SparkConf
from pyspark.sql import SparkSession
import pandas as pd

CATALOG_URL = "http://server:8080/catalog"
MANAGEMENT_URL = "http://server:8080/management"
DEMO_WAREHOUSE = "demo"

config = {
    "spark.sql.catalog.demo-catalog": "org.apache.iceberg.spark.SparkCatalog",
    "spark.sql.catalog.demo-catalog.type": "rest",
    "spark.sql.catalog.demo-catalog.uri": CATALOG_URL,
    "spark.sql.catalog.demo-catalog.warehouse": DEMO_WAREHOUSE,
    "spark.sql.catalog.demo-catalog.io-impl": "org.apache.iceberg.aws.s3.S3FileIO",
    "spark.sql.extensions": "org.apache.iceberg.spark.extensions.IcebergSparkSessionExtensions",
    "spark.sql.defaultCatalog": "demo-catalog",
    "spark.jars.packages": "org.apache.iceberg:iceberg-spark-runtime-3.5_2.12:1.5.2,org.apache.iceberg:iceberg-aws-bundle:1.5.2",
}

spark_config = SparkConf().setMaster('local').setAppName("Iceberg-REST")
for k, v in config.items():
    spark_config = spark_config.set(k, v)

spark = SparkSession.builder.config(conf=spark_config).getOrCreate()
spark.sql("CREATE NAMESPACE IF NOT EXISTS spark_demo")
data = pd.DataFrame([[1, 'a-string', 2.2]], columns=['id', 'strings', 'floats'])
sdf = spark.createDataFrame(data)
sdf.writeTo("spark_demo.my_table").createOrReplace()
spark.sql("CREATE view spark_demo.vv as select * from spark_demo.my_table")

ends up sending that POST to /catalog/v1/df910b7e-3912-11ef-a048-6b2680efd54d/namespaces/spark_demo/views

{
  "name": "vv",
  "schema": {
    "schema-id": 0,
    "type": "struct",
    "fields": [
      {
        "id": 0,
        "name": "id",
        "required": false,
        "type": "long"
      },
      {
        "id": 1,
        "name": "strings",
        "required": false,
        "type": "string"
      },
      {
        "id": 2,
        "name": "floats",
        "required": false,
        "type": "double"
      }
    ]
  },
  "view-version": {
    "version-id": 1,
    "schema-id": 0,
    "timestamp-ms": 1719994069884,
    "summary": {
      "app-id": "local-1719994018459",
      "engine-name": "spark",
      "iceberg-version": "Apache Iceberg 1.5.2 (commit cbb853073e681b4075d7c8707610dceecbee3a82)",
      "engine-version": "3.5.1"
    },
    "representations": [
      {
        "type": "sql",
        "sql": "select * from spark_demo.my_table",
        "dialect": "spark"
      }
    ],
    "default-namespace": []
  },
  "properties": {
    "create_engine_version": "Spark 3.5.1",
    "spark.query-column-names": "id,strings,floats",
    "engine_version": "Spark 3.5.1"
  }
}

[twuebi]

Setting the namespace via:

spark.sql("USE spark_demo")
spark.sql("CREATE view vv3 as select * from spark_demo.mytable")

ends up setting default-namespace:

{
  "name": "vv3",
  "schema": {
    "schema-id": 0,
    "type": "struct",
    "fields": [
      {
        "id": 0,
        "name": "id",
        "required": false,
        "type": "long"
      },
      {
        "id": 1,
        "name": "strings",
        "required": false,
        "type": "string"
      },
      {
        "id": 2,
        "name": "floats",
        "required": false,
        "type": "double"
      }
    ]
  },
  "view-version": {
    "version-id": 1,
    "schema-id": 0,
    "timestamp-ms": 1719994988301,
    "summary": {
      "engine-version": "3.5.1",
      "app-id": "local-1719994018459",
      "engine-name": "spark",
      "iceberg-version": "Apache Iceberg 1.5.2 (commit cbb853073e681b4075d7c8707610dceecbee3a82)"
    },
    "representations": [
      {
        "type": "sql",
        "sql": "select * from spark_demo.my_table",
        "dialect": "spark"
      }
    ],
    "default-namespace": [
      "spark_demo"
    ]
  },
  "properties": {
    "spark.query-column-names": "id,strings,floats",
    "engine_version": "Spark 3.5.1",
    "create_engine_version": "Spark 3.5.1"
  }
}

[nastra]

"default-namespace": [] indicates an empty namespace, not a null one, which is a valid case and there are tests for that here: https://github.com/apache/iceberg/blob/42a2c19cec31c626cbff6cc2dfafb86cdf223bd0/core/src/test/java/org/apache/iceberg/view/TestViewMetadataParser.java#L56. I've also opened apache/iceberg#9890 a while ago to properly test empty namespaces with catalogs that support it.

However, why you're getting an empty namespace when creating the view with a qualified namespace is a different thing (which I need to look into and reproduce on my end - which I'll try to do before the end of the week)

[nastra]

Ok I checked the surrounding code and the handling is correct. In https://github.com/apache/iceberg/blob/2a39af894f4f00aa37922ef765cc2583517fa1d1/spark/v3.5/spark-extensions/src/main/scala/org/apache/spark/sql/execution/datasources/v2/CreateV2ViewExec.scala#L51 we're always determining the current namespace (which can be empty), which is then what ViewVersion#defaultNamespace is set to. As I mentioned earlier that there's a difference between a null namespace and an empty one.

Spark later uses this info in https://github.com/apache/iceberg/blob/6bbf70a52ebccfaba4e7e08facd72b84b571e2a6/spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/source/SparkView.java#L75 in case there's no namespace in the underlying SQL and in your case you fully-qualified select * from spark_demo.my_table with a namespace.

In your second example you configured the current namespace via USE <namespace>, which was then evaluated https://github.com/apache/iceberg/blob/2a39af894f4f00aa37922ef765cc2583517fa1d1/spark/v3.5/spark-extensions/src/main/scala/org/apache/spark/sql/execution/datasources/v2/CreateV2ViewExec.scala#L51 and passed to ViewVersion#defaultNamespace (but not used when actually querying the view).

[twuebi]

Thanks for checking this @nastra, I understand that we should be relaxing the constraints of NamespaceIdent then since an empty default-namespace is expected.

[nastra]

in java we call this history

[c-thiel]

Hm, in the iceberg spec its called versions & version-log https://iceberg.apache.org/view-spec/#view-metadata.
I implemented the same way TableMetadata is currently implemented: Calling the struct field identically to the spec, but then creating a history accessor.

TableMetadata:
https://github.com/c-thiel/iceberg-rust/blob/ca9de89ac9d95683c8fe9191f72ab922dc4c7672/crates/iceberg/src/spec/table_metadata.rs#L208-L211

ViewMetadata:
https://github.com/c-thiel/iceberg-rust/blob/44630160be1bcf48249c31006b76a7150a029619/crates/iceberg/src/spec/view_metadata.rs#L151-L156

As the versions field is not public, it actually implements almost the same interface as java.

@ZENOTME or some other rust dev, it would be great to get some opinion on this.

[liurenjie1024]

I prefer to use similar name as spec as it makes things cleaner. history maybe an alias, and we can add a method for it if required in future.

[nastra]

I feel that things like view version id re-assignment and view version de-duplication are missing in the builder.
The same applies for schema id re-assignment / schema de-duplication.

There are a bunch of tests in the java ref impl in TestViewMetadata that make sure these things behave correctly

[c-thiel]

Yes, you are right. Bad idea to implement this half-done.
I am removing this function and handle it properly later in the full implementation of the builders.

[nastra]

in java this is called ViewHistoryEntry, maybe worth naming it here similarly?

[c-thiel]

I am also happy with renaming.
I think we should use the same naming we settle on in our discussion above. #331 (comment)

[nastra]

is it normal in rust to create a separate type for a list of X?

[c-thiel]

Normally not. This is looking forward to the ViewMetadataBuilder where we want to implement methods on those.
The motivation is that multiple ViewRepresentations are more than just a Vec - there are many rules in constructing it (i.e. no duplicate dialects). We shouldn't just expose the Vec interface externally because then users could just add an item to the list without going through any of our validation functions (which are not there yet, will come with the ViewMetadataBuilder).

Its a very valuable comment though, because I am still exposing the interface currently. I am putting it in a struct with a private field.

[c-thiel]

Fix: c-thiel@2b427e5

[nastra]

does this make sure to ignore the case of the dialect when comparing two dialects? If no, then it should

[c-thiel]

No, it did not. I fixed it here:
c-thiel@bf95eb7

The behavior is now to ignore the case when adding, but storing the proper case internally (see the test in my commit).
Is this the desired behavior or should we just lowercase it?

[nastra]

why is it better to overwrite a dialect rather than throw an error saying that there are duplicate dialects?

[c-thiel]

I guess it depends on the usage. This method is part of a builder which is not only used to create an object initially, but also to modify an object.
If we fail, there is no way to ever modify a dialect in the builder.
I am adding fallible methods additionally in c-thiel@83e4a6f (tests in next commit)

[nastra]

However, there isn't much in https://github.com/apache/iceberg/tree/5ea78e3fbe5d8c9c846a1b86bcd2b77d13a31acd/core/src/test/java/org/apache/iceberg/view we can use for now.

In the java code we typically don't construct the full JSON just to test something, but there are plenty of tests in TestViewMetadata to make sure schema / view version re-assignment / de-duplication happens or SQL dialect deduplication behaves correctly.

I just want to start with the foundation before heading into the more controversial builder bits.

I was merely asked to look at this view implementation since I did the Java implementation, but I'm not involved in iceberg-rust, so I'll let others decide how to proceed with adding views to iceberg-rust and what to include in this PR vs in follow-ups.
Regarding follow-ups, I think a few more things would be needed for the view metadata builder, which I mentioned above. `TestViewMetadata´ gives a good overview over how the view metadata / builder should behave.

Thanks for working on this btw 💯

[c-thiel]

@nastra I hope I addressed all your issues regarding functionality. If you are happy with the implementations, feel free to close our discussions :).
@ZENOTME it would now be great to get some Feedback on the rust side of things. Please check my discussions with Eduard above. There are a few open points regarding naming and also the introduction of types. It would be great to get a review from you or some other Rust Maintainer for this.

[c-thiel]

@nastra just following up on my last comment - could you close our discussion where you agree with my fix?
@liurenjie1024 could you maybe have a look at the rust side of things? Please check my discussions with Eduard above. There are a few open points regarding naming and also the introduction of types. It would be great to get a review from you or some other Rust Maintainer for this.

[liurenjie1024]

@nastra just following up on my last comment - could you close our discussion where you agree with my fix? @liurenjie1024 could you maybe have a look at the rust side of things? Please check my discussions with Eduard above. There are a few open points regarding naming and also the introduction of types. It would be great to get a review from you or some other Rust Maintainer for this.

Sure, I'll find time this week to go throught this.

[liurenjie1024]

Thanks @c-thiel for this pr, it looks great to me! I've left some minor suggestion and some question.

[liurenjie1024]

Why not impl Iterator<Item=&ViewRepresentation>?

[c-thiel]

Oh, I screwed up the impl, we cannot implement Iterator on an immutable object - no place to store the state. However, I just implemented iter() on the object itself.

[c-thiel]

New version: https://github.com/c-thiel/iceberg-rust/blob/06d09ab674f46465293be5dcb58a5af705863bd2/crates/iceberg/src/spec/view_version.rs#L150-L152

[liurenjie1024]

Could we remove this part for now? I'm expecting view manipulation would be similar to table transaction ?

[c-thiel]

Don't we still need it to create an action?

Should we keep the ViewMetadataBuilder?

[liurenjie1024]

Don't we still need it to create an action?

Yes, I think we will eventually need it, but I don't think we should expose it now as a public api. What I mean is to add it later when we actually start to implement view related action.

Should we keep the ViewMetadataBuilder?

I think we should keep it, it's useful for simple case, like creating a new view.

[c-thiel]

Ok, I removed the builder in c-thiel@06d09ab.
Is there an issue already for the transaction we could mention this commit in? I assume most of the tests and maybe a bit of the logic might be re-usable.

[liurenjie1024]

Is there an issue already for the transaction we could mention this commit in? I assume most of the tests and maybe a bit of the logic might be re-usable.

Not yet. Feel free to create issues to track them. Recently I've been focusing on table read part and 0.3 release, but never hesitate to create issues if you are interested in view support.

[liurenjie1024]

I prefer to use similar name as spec as it makes things cleaner. history maybe an alias, and we can add a method for it if required in future.

[liurenjie1024]

Why we return MappedLocalTime here? Why not just DateTime<Utc> directly?

[c-thiel]

Conversion to DateTime<Utc> is fallible - thus it would become a Result. We already have timestamp_ms() as a non-fallible version. Do you still think we should change?

[c-thiel]

What I absolutely don't like is the TableMetadata way of doing this which just panics:

iceberg-rust/crates/iceberg/src/spec/table_metadata.rs

Line 148 in f5b406f

Utc.timestamp_millis_opt(self.last_updated_ms).unwrap()

[liurenjie1024]

Thanks for explaination, I didn't notice that chrono changed their api. To be hones, I feel the name MappedLocalTime is a little difficult to understand. Also the doc says that it's only Single or None. I agree that we should not use unwrap in table_metadata. How about we convert it to crate::error::Result, e.g.

Single => Ok,
Ambiguous => Err(Unexpected),
None => Err(InvalidData)

[c-thiel]

c-thiel@d354e63

[c-thiel]

And my proposal for TableMetadata.
c-thiel@1c49ff9

Do you have a better Idea for method naming? I think last_updated_ms was a really bad method name for returning a DateTime in the first place. Nonetheless, it now changes the behavior to return an int instead so that we still have a non-fallible accessor.

[liurenjie1024]

The name last_updated_timestamp is good engouh for me. Thanks for changing this.

[liurenjie1024]

Seems this is not change?

[liurenjie1024]

Don't we still need it to create an action?

Yes, I think we will eventually need it, but I don't think we should expose it now as a public api. What I mean is to add it later when we actually start to implement view related action.

Should we keep the ViewMetadataBuilder?

I think we should keep it, it's useful for simple case, like creating a new view.

[liurenjie1024]

This should be 1?

[c-thiel]

You are right. In Java it seems to be one as well.
Why though? SortOrder, Schema, PartitionSpec all start from 0. Why not views?

[nastra]

the concept of a sequence number is thing that's only used for tables and not for views, so using INITIAL_SEQUENCE_NUMBERseems rather confusing. However, I don't think we actually validate that the view version starts at 1 I think. Other places that create a new view version however always start at 1, such as https://github.com/apache/iceberg/blob/63af974efe51486c89bff8df5416781ab3181976/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java#L1275

[c-thiel]

Lets stick to 1 then :)

Fixed in 792129d

[liurenjie1024]

Thanks for explaination, I didn't notice that chrono changed their api. To be hones, I feel the name MappedLocalTime is a little difficult to understand. Also the doc says that it's only Single or None. I agree that we should not use unwrap in table_metadata. How about we convert it to crate::error::Result, e.g.

Single => Ok,
Ambiguous => Err(Unexpected),
None => Err(InvalidData)

[liurenjie1024]

Thanks @c-thiel for this high quality pr, and I think we are quite close to merge it. I have several minor nits to fix, but in general LGTM.

[liurenjie1024]

This appears several times, maybe we can move it to error module?

[c-thiel]

Hm, I added it to the error module but I am not happy with it.
c-thiel@82dc949

The problem is that the return value of Utc.timestamp_millis_opt(timestamp_ms) offers no built-in method to convert it to an Result. So we cannot use ? but require a uitility function instead. The one I've written is only remotely related to errors - which is why I don't really like it to be in errors.rs. I am not sure though if it's worth adding a new utility module just for timestamp conversions.

Any thoughts?

I made it crate internal - so maybe its not too bad.

[liurenjie1024]

I think the MappedLocalTime is somehow another kind of result with extra values, so putting it in error module seems fine to me. Also it's not a public api, so we wil be able to change it in future if we have better solutions.

[liurenjie1024]

The name last_updated_timestamp is good engouh for me. Thanks for changing this.

[liurenjie1024]

Thanks @c-thiel 's effort on this, looks great to me!

[liurenjie1024]

cc @Xuanwo @nastra @Fokko to take another look

[c-thiel]

Rebased on current master

[liurenjie1024]

I think this pr is good enough, so I will merge it since it has been long. Thanks @c-thiel 's effort on this, and thanks @Xuanwo @nastra @ZENOTME 's review!