scylladb / cpp-rust-driver

API-compatible rewrite of https://github.com/scylladb/cpp-driver as a wrapper for Rust driver.
GNU Lesser General Public License v2.1
13 stars 11 forks source link

Materialized view #76

Closed Gor027 closed 1 year ago

Gor027 commented 1 year ago

Pre-review checklist

This PR is dependent upon #72

Adds support to fetch materialized views from cluster metadata and implementation for functions to return fields of a certain view in the metadata. Additionally, it adds support to iterate over materialized views.

Lorak-mmk commented 1 year ago

Please rebase on current version of #72

Lorak-mmk commented 1 year ago

Please rebase on current master - it's a bit difficult to review with those unrelated changes from #72

Gor027 commented 1 year ago

I don't quite like how are the structs / functions places between session.rs and metadata.rs. Before this PR we had no metadata.rs file, so everything was put into session.rs. Now that we have it, we can fix this situation.

I'd propose the following:

  • Leave query_results.rs as it is - meaning that functions accepting or returning CassIterator go there.
  • Move metadata structs to metadata.rs - CassKeyspaceMeta_, CassKeyspaceMeta, CassTableMeta_, CassTableMeta, CassMaterializedViewMeta_, CassMaterializedViewMeta, CassColumnMeta, CassSchemaMeta_, CassSchemaMeta.
  • Move metadata functions that don't touch session into metadata.rs - everything from cass_schema_meta_free downward.
  • create_table_metadata should probably also go into metadata.rs, but I'm not 100% sure about it.

What do you think?

As you suggested above, I moved all metadata-related structures and functions to metadata.rs. Importing the CassColumnType enum is also moved to metadata.rs, so I think it is better to have create_table_metadata also in that file rather than in session.rs.

Gor027 commented 1 year ago

Update:

Rebased on #95 and enabled materialized view metadata tests on GitHub Actions (SchemaMetadataTest.*View*). This PR should be merged only after #95 is merged.

Lorak-mmk commented 1 year ago

Actually, one more changed is needed - adjustment of the limitations table in README to take into account changes made by this PR

Gor027 commented 1 year ago

@Lorak-mmk Guess it is good now to be merged.

Gor027 commented 1 year ago

Also, as you mentioned earlier, a nice optimization maybe not copy the metadata information, but have a pointer to a ClusterData object and on each metadata-related function call calculate and return the necessary information. I will open an issue to be discussed in the future.