result: small cleanups before bump to 0.15 #205
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CassResultData should represent a result metadata. Tracing id is not a part of it.
Again, paging state response has nothing to do with the metadata needed to deserialize the rows.
This test should not be enabled! See the description: * Verify that the column count of a bound statement's result metadata is * properly updated for newer protocol versions (v5 and greater) when a table's * schema is altered. This test will not work after next commit, thus it's being disabled. On the other hand, the test AlterDoesntUpdateColumnCount should be enabled. * Verify that the column count of a bound statement's result metadata doesn't * change for older protocol versions (v4 and less) when a table's schema is altered. It will be enabled later in this PR, once following issue is addressed: Currently, the `column_specs` and `col_data_types` may have different sizes for prepared statements. This is because, `col_data_types` is created based on prepared statement's cached metadata, and `column_specs` are taken from the QueryResult.
Currently, we heavily depend on the fact that `QueryResult::col_specs()` returns a `&[ColumnSpec<'static>]`. This won't be true once we bump rust-driver dependency to 0.15 - the lifetime of ColumnSpec will be tied to the lifetime of `QueryRowsResult` object. It looks like most of the information held by `col_spec` was reduntant. It was only used to retrieve the name of the column. This is why, we will only hold the information about the column names. The column types are stored in `col_data_types`. This also allows us to get rid of `'static` bound from `CassResultData::from_result_payload`. It will be helpful when bumping to 0.15. As a bonus, we can also replace the `Vec<>` with a slice `&[]` in `from_result_payload`, since we do not need to own the column specs anymore.
muzarski
force-pushed
the
cass-result-cleanup
branch
from
2069df0 to
904a971
Compare
|
I found a bug in the driver. The fix will be part of this PR. |
This structure will hold information about the column name and the column data type. Refactored the `CassResultData`, so now it holds one vector of `CassColumnSpec` instead of two separate vectors for names and data types. Previous version was buggy for prepared statements: - data types came from cached metadata from the statement - column names came from metadata provided by query result Because of this, the vector could be of a different length, when someone ALTERed the corresponding table. Notice that after this commit, we don't reuse cached metadata from prepared statement and reuse the metadata provided from the server. Next commits will fix that.
It is now enabled in favor of AlterProperlyUpdatesColumnCount. From now on, the metadata in `CassResultData` is consistent, and the behaviour is that metadata is not updated after altering the table. Original cpp-driver expects this behaviour for CQL v4 and less: * Verify that the column count of a bound statement's result metadata doesn't * change for older protocol versions (v4 and less) when a table's schema is altered.
|
v2: addressed the bug and updated the cover letter. It's ready for review. |
Lorak-mmk
approved these changes
Nov 21, 2024
|
v2.1: Renamed |
Lorak-mmk
approved these changes
Nov 21, 2024
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Tracing id and paging state response
These were moved from
CassResultDatatoCassResult.CassResultData
AlterProperlyUpdatesColumnCount test
This test should not be enabled. It expects that prepared statement's result metadata was updated after table schema was altered. It worked only because of a driver bug described later. I disabled this test. It's expected to be working for CQL protocol v5 and above. I think it can be addressed later.
Driver bug
Before this PR, there was an inconsistency of metadata stored in
CassResultData:column_specs: Vec<ColumnSpec<'static>>was obtained from a query resultcol_data_types: Arc<Vec<Arc<CassDataType>>>was obtained from the prepared statement's cached result metadataBecause of this, these vector could be of a different length when table schema was altered - query result provided the updated metadata, while prepared statement still held an outdated metadata.
Why was the aforementioned test working?
It's because
cass_result_column_countwould return the size ofcolumn_specs(i.e. updated metadata).This PR fixes the issue by unifying these two vectors into a single one.
CassColumnSpecwas introduced to address it. It holds a column name (String) and a column data type (Arc<CassDataType).AlterDoesntUpdateColumnCount
On the other hand, this test should be enabled. It expects, that for protocol version 4 or less, the prepared statement's metadata is not updated after table schema was altered. It's enabled at the end of PR, once the bug mentioned above is fixed.
Pre-review checklist
[ ] I have implemented Rust unit tests for the features/changes introduced..github/workflows/build.ymlingtest_filter..github/workflows/cassandra.ymlingtest_filter.