iterator: cleanup iterator module and implement cass_iterator_type #230

muzarski · 2025-03-12T16:00:10Z

This PR mostly does some cleanups/small refactors. In addition, it implements cass_iterator_type method.

Fixes: #47

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 implemented Rust unit tests for the features/changes introduced.~~
~~[ ] I have enabled appropriate tests in .github/workflows/build.yml in gtest_filter.~~
~~[ ] I have enabled appropriate tests in .github/workflows/cassandra.yml in gtest_filter.~~

query_result.rs needed to import metadata structs just because iterator methods were defined here. I also removed the commented CassIterator methods. I don't see a purpose of having this comment. I'll do the same for other types as well in later commit.

scylla-rust-wrapper/src/iterator.rs

muzarski · 2025-03-12T21:16:50Z

v1.1: Fixed the typo in the comment

Lorak-mmk · 2025-03-13T12:42:07Z

scylla-rust-wrapper/src/iterator.rs

+#[no_mangle]
+pub unsafe extern "C" fn cass_iterator_type(iterator: *mut CassIterator) -> CassIteratorType {
+    let iter = BoxFFI::as_ref(iterator);
+
+    // TODO: Adjust CassIterator enum hierarchy (done later in this PR).
+    match iter {
+        CassIterator::CassResultIterator(_) => CassIteratorType::CASS_ITERATOR_TYPE_RESULT,
+        CassIterator::CassRowIterator(_) => CassIteratorType::CASS_ITERATOR_TYPE_ROW,
+        CassIterator::CassCollectionIterator(_) => CassIteratorType::CASS_ITERATOR_TYPE_COLLECTION,
+        CassIterator::CassMapIterator(_) => CassIteratorType::CASS_ITERATOR_TYPE_MAP,
+        CassIterator::CassUdtIterator(_) => CassIteratorType::CASS_ITERATOR_TYPE_USER_TYPE_FIELD,
+        CassIterator::CassSchemaMetaIterator(_) => {
+            CassIteratorType::CASS_ITERATOR_TYPE_KEYSPACE_META
+        }
+        CassIterator::CassKeyspaceMetaTableIterator(_) => {
+            CassIteratorType::CASS_ITERATOR_TYPE_TABLE_META
+        }
+        CassIterator::CassKeyspaceMetaUserTypeIterator(_) => {
+            CassIteratorType::CASS_ITERATOR_TYPE_TYPE_META
+        }
+        CassIterator::CassKeyspaceMetaViewIterator(_) => {
+            CassIteratorType::CASS_ITERATOR_TYPE_MATERIALIZED_VIEW_META
+        }
+        CassIterator::CassTableMetaIterator(_) => CassIteratorType::CASS_ITERATOR_TYPE_COLUMN_META,
+        CassIterator::CassViewMetaIterator(_) => CassIteratorType::CASS_ITERATOR_TYPE_COLUMN_META,
+    }
+}
+


Commit: "iterator: implement cass_iterator_type"

What does it mean that it needs to be adjusted? I've read the commit message, I've read the code, and I don't know what it means.

As I mentioned in my other comment, I want to adjust CassIterator enum hierarchy so each variant corresponds to the variant in CassIteratorType enum. I'll put this information in the code comment and commit message.

Later commits explained that it is about naming of the variants.
I don't think this commit needs to mention that. Naming change is unrelated.

Later commits explained that it is about naming of the variants. I don't think this commit needs to mention that. Naming change is unrelated.

Not only the naming, but structure as well.

Lorak-mmk · 2025-03-13T12:44:34Z

scylla-rust-wrapper/src/iterator.rs

"iterator: add CassTupleIterator variant "

Commit message says that we need a way to distinguish iterator over tuple from iterator over other collection, but does not explain why. Please explain - it is not obvious to me at all.

Because from user's perspective these are two different kinds of iterators. One created from cass_iterator_from_collection, and the other created from cass_iterator_from_tuple. In our case, the underlying structure is the same, but for the user these are not the same. Why? Because there is CASS_ITERATOR_TYPE_COLLETION and CASS_ITERATOR_TYPE_TUPLE. This is the exact reason why we need to distinguish them - to return corresponding CassIteratorType in cass_iterator_type method.

I'll put more context to the commit message.

Lorak-mmk · 2025-03-13T12:47:24Z

scylla-rust-wrapper/src/iterator.rs

Commit: "iterator: introduce CassColumnMeta iterator "

Again, why should we do this? Is there some difference in behavior between those iterators? Does it allow some nice code cleanups?

The name of this new variant seems inconsistent.
We have CassKeyspaceMetaTableIterator, which iterator over Tables in a Keyspace metadata.
There are 2 other similar variants (for udts and views).

We have CassTableMetaIterator, which iterates over stuff (columns) in a Table
We have CassViewMetaIterator, which iterates over stuff (columns) in a View

Now you introduce CassColumnsMetaIterator - interpreting the name the same as others, I would assume it iterates over some stuff in Column, but it iterates over columns.

Commit: "iterator: introduce CassColumnMeta iterator "

Again, why should we do this? Is there some difference in behavior between those iterators? Does it allow some nice code cleanups?

Notice that before this PR we used CassIterator::CassTableMetaIterator for two things:

as you noticed, to iterate over columns metadata (cass_iterator_columns_from_table_meta)

to iterate over materialized views meta (cass_iterator_materialized_views_from_table_meta)

But there is other way to create an iterator over columns. Namely cass_iterator_columns_from_materialized_view_meta.

And now, both iterators cass_iterator_columns_from_table_meta and cass_iterator_columns_from_materialized_view_meta should return CASS_ITERATOR_TYPE_COLUMN_META when cass_iterator_type is called on them.

This is why I introduce the additional layer (CassIterator::CassColumnsMetaIterator). I want CassIterator top-level hierarchy directly correspond to CassIteratorType enum variants.

Lorak-mmk · 2025-03-13T12:51:58Z

scylla-rust-wrapper/src/iterator.rs

    CassKeyspaceMetaUserTypeIterator(CassKeyspaceMetaIterator<'result_or_schema>),
    CassKeyspaceMetaViewIterator(CassKeyspaceMetaIterator<'result_or_schema>),
+    CassMaterializedViewsMetaIterator(CassMaterializedViewsMetaIterator<'result_or_schema>),
    CassColumnsMetaIterator(CassColumnsMetaIterator<'result_or_schema>),
    CassTableMetaIterator(CassTableMetaIterator<'result_or_schema>),


Ditto about inconsistent naming.

Lorak-mmk · 2025-03-13T12:55:39Z

scylla-rust-wrapper/src/iterator.rs

+    // Iterators derived from CassResult.
+    // Naming convention of the variants: the name of the collection.
+    /// Iterator over rows in a result.
+    Result(CassResultIterator<'result_or_schema>),
+    /// Iterator over columns (values) in a row.
+    Row(CassRowIterator<'result_or_schema>),
+    /// Iterator over values in a collection.
+    Collection(CassCollectionIterator<'result_or_schema>),
+    /// Iterator over key-value pairs in a map.
+    Map(CassMapIterator<'result_or_schema>),
+    /// Iterator over values in a tuple.
+    Tuple(CassCollectionIterator<'result_or_schema>),
+
+    // Iterators derived from CassSchemaMeta.
+    // Naming convention of the variants: name of item in the collection (plural).
+    /// Iterator over fields in UDT.
+    UdtFields(CassUdtIterator<'result_or_schema>),
+    /// Iterator over keyspaces in schema metadata.
+    KeyspacesMeta(CassSchemaMetaIterator<'result_or_schema>),
+    /// Iterator over tables in keyspace metadata.
+    TablesMeta(CassKeyspaceMetaIterator<'result_or_schema>),
+    /// Iterator over UDTs in keyspace metadata.
+    UserTypes(CassKeyspaceMetaIterator<'result_or_schema>),
+    /// Iterator over materialized views in either keyspace or table metadata.
+    MaterializedViewsMeta(CassMaterializedViewsMetaIterator<'result_or_schema>),
+    /// Iterator over columns metadata in either table or view metadata.


Hmm... ok so you try fix the consistency here, after introducing inconsistency before.

Previously we had one naming convention, now we have too. Why can't we have one?

The naming conventions are derived from CassColumnType enum variants. I don't know why, but this is the convention chosen by cpp-driver. For result-derived iterators, it's the name of the collection we iterate over. For schema-based iterators, it's the name of the items in the collection we iterate over.

As a side note, it took me a while to notice the difference. When I first saw that iterator returned from cass_iterator_from_result is CASS_ITERATOR_TYPE_RESULT, I assumed that iterator returned from cass_iterator_keyspaces_from_schema_meta, will be CASS_ITERATOR_TYPE_SCHEMA_META or sth like that. But no, its type is CASS_ITERATOR_TYPE_KEYSPACE_META (because we iterate over keyspaces metadata from schema metadata).

It still needs to be adjusted, but we need to adjust CassIterator enum hierarchy first to fix it. In the end, I want CassIterator enum top-level hierarchy directly correspond to CassIteratorType enum hierarchy. For example, notice that we currently don't return `CASS_ITERATOR_TYPE_TUPLE`. This is because iterator created from tuple is currently hidden under `CassIterator::CassCollectionIterator`.

cass_iterator_from_tuple creates a CassCollectionIterator, to reuse its logic. We need to have a way to distinguish such iterator from iterator created by `cass_iterator_from_collection`. Thanks to that, we can now return `CASS_ITERATOR_TYPE_TUPLE` from `cass_iterator_type` for iterator that was created via `cass_iterator_from_tuple`.

Iterator over columns metadata can be created from either materialized view metadata or table metadata. Let's express this relationship by introducing an additional enum. Thanks to that, all iterators that are of type `CASS_ITERATOR_TYPE_COLUMN_META` are placed under one enum.

Iterator over materialized views can be created from either keyspace metadata or table metadata. Let's express this relationship by introducing an additional enum. Thanks to that, all iterators that are of type `CASS_ITERATOR_TYPE_MATERIALIZED_VIEW_META` are placed under one enum.

These variants were moved to other enums.

The variant naming conventions come from CASS_ITERATOR_TYPE values that correspond to each variant. I also ditched the `Cass` prefix and `Iterator` suffix for variant names. These are redundant.

muzarski · 2025-03-13T16:28:26Z

v1.2: Adjusted commit messages so they better explain why CassIterator hierarchy is changed in this PR.

muzarski added 3 commits March 12, 2025 14:38

query_result: remove unnecessary comments

d61037c

build: generate bindings for CassIteratorType

d17b182

muzarski force-pushed the cass-iterator-type branch 2 times, most recently from 658e47c to 657ca19 Compare March 12, 2025 16:21

muzarski self-assigned this Mar 12, 2025

muzarski requested review from wprzytula and Lorak-mmk March 12, 2025 16:23

muzarski added this to the 0.4 milestone Mar 12, 2025

wprzytula requested changes Mar 12, 2025

View reviewed changes

scylla-rust-wrapper/src/iterator.rs Outdated Show resolved Hide resolved

scylla-rust-wrapper/src/iterator.rs Show resolved Hide resolved

scylla-rust-wrapper/src/iterator.rs Outdated Show resolved Hide resolved

muzarski force-pushed the cass-iterator-type branch from 657ca19 to 07cd160 Compare March 12, 2025 21:16

muzarski force-pushed the cass-iterator-type branch from 07cd160 to 185a92b Compare March 12, 2025 21:23

muzarski requested a review from wprzytula March 12, 2025 21:27

wprzytula approved these changes Mar 13, 2025

View reviewed changes

Lorak-mmk requested changes Mar 13, 2025

View reviewed changes

muzarski added 6 commits March 13, 2025 17:23

iterator: remove unused variants

8495aa3

These variants were moved to other enums.

iterator: adjust variant names and add documentation

89a106c

The variant naming conventions come from CASS_ITERATOR_TYPE values that correspond to each variant. I also ditched the `Cass` prefix and `Iterator` suffix for variant names. These are redundant.

muzarski force-pushed the cass-iterator-type branch from 185a92b to 89a106c Compare March 13, 2025 16:27

muzarski requested a review from Lorak-mmk March 13, 2025 16:28

Lorak-mmk approved these changes Mar 13, 2025

View reviewed changes

muzarski merged commit 937c737 into scylladb:master Mar 13, 2025
11 checks passed

muzarski mentioned this pull request Apr 14, 2025

Release v0.4.0 #245

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

iterator: cleanup iterator module and implement cass_iterator_type #230

iterator: cleanup iterator module and implement cass_iterator_type #230

muzarski commented Mar 12, 2025 •

edited

Loading

muzarski commented Mar 12, 2025

Lorak-mmk Mar 13, 2025

muzarski Mar 13, 2025

Lorak-mmk Mar 13, 2025

muzarski Mar 13, 2025

Lorak-mmk Mar 13, 2025

Lorak-mmk Mar 13, 2025

muzarski Mar 13, 2025

Lorak-mmk Mar 13, 2025

Lorak-mmk Mar 13, 2025

muzarski Mar 13, 2025

Lorak-mmk Mar 13, 2025

Lorak-mmk Mar 13, 2025

muzarski Mar 13, 2025

muzarski commented Mar 13, 2025

iterator: cleanup iterator module and implement cass_iterator_type #230

iterator: cleanup iterator module and implement cass_iterator_type #230

Conversation

muzarski commented Mar 12, 2025 • edited Loading

Pre-review checklist

muzarski commented Mar 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muzarski commented Mar 13, 2025

muzarski commented Mar 12, 2025 •

edited

Loading