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.

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

Unify ColumnType and CqlType #691

Open
piodul opened this issue Apr 5, 2023 · 2 comments · May be fixed by #1166
Open

Unify ColumnType and CqlType #691

piodul opened this issue Apr 5, 2023 · 2 comments · May be fixed by #1166
Assignees
Labels
API-breaking This might introduce incompatible API changes API-stability Part of the effort to stabilize the API area/metadata
Milestone

Comments

@piodul
Copy link
Collaborator

piodul commented Apr 5, 2023

Currently, the driver has two complex but similar types: ColumnType and CqlType. They are both supposed to represent a CQL type, but have slight differences:

  • ColumnType is used in metadata of prepared statements and responses to queries. It does not include information about whether collections and UDTs are frozen. The type definitions are self contained - if it represents a complex type that contains a UDT somewhere inside it, it has the whole definition of the UDT inline.
  • CqlType is used in ClusterMetadata - it is computed based on the information in the system tables. It does include information about whether collections and UDTs are frozen. Until recently (changed in topology: UDTs hold complete definition of themselves #649) information about UDTs was not inlined, but now it is; however, if metadata fetch returns inconsistent metadata and it does not have a definition of a UDT that was needed inside some CqlType, then a placeholder MissingUserDefinedType will be put instead.

Those types have very similar names and function, so it might make sense to unify them. This will help write user logic that is supposed to operate on both representations. We will have to account for the differences:

  • Presence vs. absence of information about being frozen: the newly introduced type could be generic over the "frozen flag" type. The "frozen flag" type could be () vs. bool, or two new enums isomorphic to those types that express the intent better.
  • Handling the MissingUserDefinedType case:
    • The simplest way out would be to put Result<_, MissingUserDefinedType> in the unified type. However, this would force the users that need to operate on the existing ColumnType representation to handle the Result, even though it would never be an error, so that would introduce unnecessary complication and would be ugly.
    • An improvement upon the previous would be to convert MissingUserDefinedType into a generic parameter, like in the case of the "frozen flag" - in case of ColumnType it would be an uninhabited type. Slightly better, but the Result is still there.
    • (Preferred) Get rid of MissingUserDefinedType altogether! I insisted on handling the MissingUserDefinedType error like this to make the driver more robust; if we didn't have it then we would have to fail the whole metadata fetch and probably fail the creation of Session. However, if we move the cluster metadata module out of the driver like described in Move non-critical metadata out of Session #595, then it should be fine to fail the whole metadata fetch as it won't reduce availability. Therefore Move non-critical metadata out of Session #595 is a requirement for this solution.
@piodul piodul added this to the 1.0.0 milestone Apr 5, 2023
@wprzytula wprzytula added API-breaking This might introduce incompatible API changes API-stability Part of the effort to stabilize the API labels Jul 30, 2023
@roydahan roydahan modified the milestones: 1.0.0, 0.12.0 Nov 12, 2023
@Lorak-mmk Lorak-mmk self-assigned this Nov 15, 2023
@avelanarius avelanarius assigned muzarski and unassigned Lorak-mmk Jan 15, 2024
@roydahan
Copy link
Collaborator

If we decided that it shouldn't be part of 0.12.0, please comment and describe the decision and dependency and change the Milestone.

@piodul
Copy link
Collaborator Author

piodul commented Jan 30, 2024

We discussed it offline with @muzarski and decided to go with the last option on the "Handling the MissingUserDefinedType case". It introduces a dependency on #595, which is currently being worked on and targets 0.13.0, so I'll move this one to 0.13.0, too.

@piodul piodul modified the milestones: 0.12.0, 0.13.0 Jan 30, 2024
@avelanarius avelanarius modified the milestones: 0.13.0, 0.14.0 Apr 30, 2024
@wprzytula wprzytula modified the milestones: 0.14.0, 0.16.0 Aug 20, 2024
@roydahan roydahan assigned Lorak-mmk and unassigned muzarski Dec 18, 2024
Lorak-mmk added a commit to Lorak-mmk/scylla-rust-driver that referenced this issue Jan 8, 2025
Removed types: CqlType, CollectionType, NativeType.

Fixes: scylladb#691
@Lorak-mmk Lorak-mmk linked a pull request Jan 9, 2025 that will close this issue
8 tasks
Lorak-mmk added a commit to Lorak-mmk/scylla-rust-driver that referenced this issue Jan 9, 2025
Removed types: CqlType, CollectionType, NativeType.
Their usages are replaced with the relevant types from scylla_cql.

Fixes: scylladb#691
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API-breaking This might introduce incompatible API changes API-stability Part of the effort to stabilize the API area/metadata
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants