Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Materialized view #76

Merged
merged 5 commits into from
Jan 4, 2023
Merged

Materialized view #76

merged 5 commits into from
Jan 4, 2023

Conversation

Gor027
Copy link
Contributor

@Gor027 Gor027 commented Sep 9, 2022

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have enabled appropriate tests in .github/workflows/build.yaml in gtest_filter.

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.

@Gor027 Gor027 force-pushed the materialized_view branch 2 times, most recently from 0e67c5b to b4eea0d Compare September 13, 2022 08:38
@Gor027 Gor027 mentioned this pull request Sep 19, 2022
Merged
4 tasks
@Gor027 Gor027 mentioned this pull request Oct 2, 2022
Closed
@Lorak-mmk
Copy link
Collaborator

Please rebase on current version of #72

@Gor027 Gor027 force-pushed the materialized_view branch 2 times, most recently from f49ab2b to b60383d Compare October 9, 2022 23:33
@Lorak-mmk
Copy link
Collaborator

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

Lorak-mmk
Lorak-mmk requested changes Nov 24, 2022
View reviewed changes
Copy link
Collaborator

@Lorak-mmk Lorak-mmk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like that all the metadata methods are in session.rs - they should have their own file, as they are not methods on the CassSesson object.

scylla-rust-wrapper/src/user_type.rs Outdated Show resolved Hide resolved
scylla-rust-wrapper/src/user_type.rs Outdated Show resolved Hide resolved
scylla-rust-wrapper/src/session.rs Outdated Show resolved Hide resolved
scylla-rust-wrapper/src/session.rs Outdated Show resolved Hide resolved
scylla-rust-wrapper/src/session.rs Outdated Show resolved Hide resolved
Lorak-mmk
Lorak-mmk requested changes Dec 19, 2022
View reviewed changes
Copy link
Collaborator

@Lorak-mmk Lorak-mmk left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

scylla-rust-wrapper/src/session.rs Outdated Show resolved Hide resolved
scylla-rust-wrapper/src/session.rs Outdated Show resolved Hide resolved
scylla-rust-wrapper/src/session.rs Outdated Show resolved Hide resolved
scylla-rust-wrapper/src/session.rs Outdated Show resolved Hide resolved
@Gor027 Gor027 force-pushed the materialized_view branch 2 times, most recently from 8d1201e to 7a424cc Compare December 20, 2022 12:03
@Gor027
Copy link
Contributor Author

Gor027 commented Dec 20, 2022

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.

Lorak-mmk
Lorak-mmk requested changes Dec 21, 2022
View reviewed changes
Copy link
Collaborator

@Lorak-mmk Lorak-mmk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the overall changes now (apart from few comments I left).
Few issues I have with the commits:

  • "Add materialized view metadata ": message is incorrect, there is not RefCell anymore
  • "Add impl for functions returning view metadata fields " - this commit should not include moving existing functions between files, that should be a seperate one.

scylla-rust-wrapper/src/metadata.rs Outdated Show resolved Hide resolved
scylla-rust-wrapper/src/metadata.rs Outdated Show resolved Hide resolved
scylla-rust-wrapper/src/metadata.rs Outdated Show resolved Hide resolved
scylla-rust-wrapper/src/metadata.rs Outdated Show resolved Hide resolved
scylla-rust-wrapper/src/metadata.rs Outdated Show resolved Hide resolved
@Gor027 Gor027 force-pushed the materialized_view branch 2 times, most recently from 01b5ae7 to a4274ac Compare December 22, 2022 11:57
@Gor027 Gor027 mentioned this pull request Dec 22, 2022
Merged
5 tasks
@Gor027
Copy link
Contributor Author

Gor027 commented Dec 22, 2022

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.

@Gor027 Gor027 force-pushed the materialized_view branch 2 times, most recently from 2306d4b to c5db322 Compare December 23, 2022 17:32
@Gor027
Add materialized view metadata
526d8e7 
Added `CassMaterializedViewMeta` struct to store metadata for views. As
the interface should enable to get base table metadata for each view, as
well as to get all views for a given table metadata, to avoid memory
leaks because of the cyclic references, `base_table` of
`CassMaterializedViewMeta` is a `Weak` reference to `CassTableMeta`.
@Gor027
Move metadata-related structs and functions to metadata.rs
17c2103 
Moved metadata-related structs and functions from `session.rs`
to `metadata.rs` to have only session-related functions in `session.rs`.
@Gor027
Add impl for functions returning view metadata fields
754e6fe 
Added implementation for the following functions:
 * cass_keyspace_meta_materialized_view_by_name
 * cass_keyspace_meta_materialized_view_by_name_n
 * cass_table_meta_materialized_view_by_name
 * cass_table_meta_materialized_view_by_name_n
 * cass_table_meta_materialized_view_count
 * cass_table_meta_materialized_view
 * cass_materialized_view_meta_column_by_name
 * cass_materialized_view_meta_column_by_name_n
 * cass_materialized_view_meta_name
 * cass_materialized_view_meta_base_table
 * cass_materialized_view_meta_column_count
 * cass_materialized_view_meta_column
 * cass_materialized_view_meta_partition_key_count
 * cass_materialized_view_meta_partition_key
 * cass_materialized_view_meta_clustering_key_count
 * cass_materialized_view_meta_clustering_key
@Gor027
Add materialized view iterator
8d3bc8e 
Added CassViewMetaIterator struct to enable iteration over materialized
view metadata for keyspace and table metadata, as well as iteration over
view metadata fields such as columns.

Enabled materialized view metadata tests on GitHub Actions.
@Lorak-mmk
Copy link
Collaborator

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

@Gor027
Copy link
Contributor Author

Gor027 commented Jan 3, 2023

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

@Gor027
Copy link
Contributor Author

Gor027 commented Jan 3, 2023

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.

@Lorak-mmk Lorak-mmk merged commit 0af6fae into scylladb:master Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lorak-mmk Lorak-mmk Lorak-mmk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Gor027 @Lorak-mmk