Skip to content

Commit ba8baae

Browse files
wprzytulawprzytula
authored
custom_type_parser: fix parsing frozen types (#1567)
Fixes: #1566 Frozen types were not handled correctly in the custom type parser. When a type was wrapped in `FrozenType(...)`, the parser did not set the `frozen` flag on the inner type. Even worse: `FrozenType` was not recognized at all, leading to parse errors. This commit introduces a `frozen_context` field in the parser state, which tracks whether we are currently parsing a frozen type. When `FrozenType(...)` is encountered, this field is set to true for the duration of parsing the inner type, and then set back to false. This commit also adds unit test cases for parsing frozen collections inside vectors, as well as extends the integration tests for vectors with a test case for vectors of frozen collections. ## Pre-review checklist <!-- Make sure you took care of the issues on the list. Put 'x' into those boxes which apply. You can also create the PR now and click on all relevant checkboxes. See CONTRIBUTING.md for more details. --> - [x] I have split my patch into logically separate commits. - [x] All commit messages clearly explain what they change and why. - [x] I added relevant tests for new features and bug fixes. - [x] All commits compile, pass static checks and pass test. - [x] PR description sums up the changes and reasons why they should be introduced. - ~~[ ] I have provided docstrings for the public items that I want to introduce.~~ - ~~[ ] I have adjusted the documentation in `./docs/source/`.~~ - [x] I added appropriate `Fixes:` annotations to PR description.
2 parents fd543e4 + d54d238 commit ba8baae

File tree

3 files changed

+103
-14
lines changed

3 files changed

+103
-14
lines changed

‎scylla-cql/src/deserialize/value_tests.rs‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,59 @@ fn test_custom_cassandra_type_parser() {
104104
),
105105
},
106106
),
107+
// Frozen collection inside vector.
108+
(
109+
"org.apache.cassandra.db.marshal.VectorType(org.apache.cassandra.db.marshal.FrozenType(org.apache.cassandra.db.marshal.SetType(org.apache.cassandra.db.marshal.Int32Type)), 3)",
110+
ColumnType::Vector {
111+
typ: Box::new(ColumnType::Collection {
112+
frozen: true,
113+
typ: CollectionType::Set(Box::new(ColumnType::Native(NativeType::Int))),
114+
}),
115+
dimensions: 3,
116+
},
117+
),
118+
// Frozen collection inside another frozen collection, inside vector.
119+
(
120+
"org.apache.cassandra.db.marshal.VectorType(org.apache.cassandra.db.marshal.FrozenType(org.apache.cassandra.db.marshal.SetType(org.apache.cassandra.db.marshal.FrozenType(org.apache.cassandra.db.marshal.SetType(org.apache.cassandra.db.marshal.Int32Type)))), 3)",
121+
ColumnType::Vector {
122+
typ: Box::new(ColumnType::Collection {
123+
frozen: true,
124+
typ: CollectionType::Set(Box::new(ColumnType::Collection {
125+
frozen: true,
126+
typ: CollectionType::Set(Box::new(ColumnType::Native(NativeType::Int))),
127+
})),
128+
}),
129+
dimensions: 3,
130+
},
131+
),
132+
// Tuple: non-frozen collection, frozen collection, non-frozen collection, frozen collection.
133+
// Tests that frozen context is correctly set and unset.
134+
(
135+
"org.apache.cassandra.db.marshal.TupleType(\
136+
org.apache.cassandra.db.marshal.SetType(org.apache.cassandra.db.marshal.Int32Type),\
137+
org.apache.cassandra.db.marshal.FrozenType(org.apache.cassandra.db.marshal.SetType(org.apache.cassandra.db.marshal.Int32Type)),\
138+
org.apache.cassandra.db.marshal.SetType(org.apache.cassandra.db.marshal.Int32Type),\
139+
org.apache.cassandra.db.marshal.FrozenType(org.apache.cassandra.db.marshal.SetType(org.apache.cassandra.db.marshal.Int32Type)),\
140+
)",
141+
ColumnType::Tuple(vec![
142+
ColumnType::Collection {
143+
frozen: false,
144+
typ: CollectionType::Set(Box::new(ColumnType::Native(NativeType::Int))),
145+
},
146+
ColumnType::Collection {
147+
frozen: true,
148+
typ: CollectionType::Set(Box::new(ColumnType::Native(NativeType::Int))),
149+
},
150+
ColumnType::Collection {
151+
frozen: false,
152+
typ: CollectionType::Set(Box::new(ColumnType::Native(NativeType::Int))),
153+
},
154+
ColumnType::Collection {
155+
frozen: true,
156+
typ: CollectionType::Set(Box::new(ColumnType::Native(NativeType::Int))),
157+
},
158+
]),
159+
),
107160
];
108161

109162
for (input, expected) in tests {

‎scylla-cql/src/frame/response/custom_type_parser.rs‎

Lines changed: 31 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -28,12 +28,18 @@ struct UDTParameters<'result> {
2828

2929
pub(crate) struct CustomTypeParser<'result> {
3030
parser: ParserState<'result>,
31+
/// This field is used to track whether we are currently parsing a frozen type.
32+
/// Being frozen flows down to all nested types, so we need to track it in the parser state.
33+
/// When we encounter a `FrozenType(...)`, this field is set to true for the duration
34+
/// of parsing the inner type, and then set back to false.
35+
frozen_context: bool,
3136
}
3237

3338
impl<'result> CustomTypeParser<'result> {
3439
fn new(input: &'result str) -> CustomTypeParser<'result> {
3540
Self {
3641
parser: ParserState::new(input),
42+
frozen_context: false,
3743
}
3844
}
3945

@@ -241,6 +247,7 @@ impl<'result> CustomTypeParser<'result> {
241247
) -> Result<[Result<ColumnType<'result>, CustomTypeParseError>; N], CustomTypeParseError> {
242248
let mut backup = Self {
243249
parser: self.parser,
250+
frozen_context: self.frozen_context,
244251
};
245252

246253
// FIXME: Rewrite using std::iter::FromIterator::collect_array after it is stabilized.
@@ -270,15 +277,15 @@ impl<'result> CustomTypeParser<'result> {
270277
let [element_type_result] = self.get_n_type_parameters::<1>()?;
271278
let element_type = element_type_result?;
272279
Ok(ColumnType::Collection {
273-
frozen: false,
280+
frozen: self.frozen_context,
274281
typ: CollectionType::List(Box::new(element_type)),
275282
})
276283
}
277284
"SetType" => {
278285
let [element_type_result] = self.get_n_type_parameters::<1>()?;
279286
let element_type = element_type_result?;
280287
Ok(ColumnType::Collection {
281-
frozen: false,
288+
frozen: self.frozen_context,
282289
typ: CollectionType::Set(Box::new(element_type)),
283290
})
284291
}
@@ -287,7 +294,7 @@ impl<'result> CustomTypeParser<'result> {
287294
let key_type = key_type_result?;
288295
let value_type = value_type_result?;
289296
Ok(ColumnType::Collection {
290-
frozen: false,
297+
frozen: self.frozen_context,
291298
typ: CollectionType::Map(Box::new(key_type), Box::new(value_type)),
292299
})
293300
}
@@ -313,14 +320,34 @@ impl<'result> CustomTypeParser<'result> {
313320
"UserType" => {
314321
let params = self.get_udt_parameters()?;
315322
Ok(ColumnType::UserDefinedType {
316-
frozen: false,
323+
frozen: self.frozen_context,
317324
definition: Arc::new(UserDefinedType {
318325
name: params.type_name.into(),
319326
keyspace: params.keyspace.into(),
320327
field_types: params.field_types,
321328
}),
322329
})
323330
}
331+
"FrozenType" => {
332+
// Frozeness flows down to all nested types.
333+
//
334+
// In my tests, ScyllaDB always wraps the inner collection types in FrozenType anyway.
335+
// Given CQL definition:
336+
// > vector<frozen<list<set<int>>>, 3>
337+
// The type string received is (omitting the `org.apache.cassandra.db.marshal.` prefix for brevity):
338+
// > VectorType(FrozenType(ListType(FrozenType(SetType(Int32Type)))))
339+
//
340+
// But it logically could omit the inner FrozenType wrappers, so we need to handle that case as well
341+
// (by pushing `frozen` flag as `true` down the parsing process).
342+
343+
let previous_frozen_context = self.frozen_context;
344+
self.frozen_context = true;
345+
346+
let [result] = self.get_n_type_parameters::<1>()?;
347+
348+
self.frozen_context = previous_frozen_context;
349+
result
350+
}
324351
name => Err(CustomTypeParseError::UnknownComplexCustomTypeName(
325352
name.into(),
326353
)),

‎scylla/tests/integration/types/cql_collections.rs‎

Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -387,40 +387,47 @@ async fn test_vector_type_unprepared() {
387387
let ks = unique_keyspace_name();
388388

389389
session.ddl(format!("CREATE KEYSPACE IF NOT EXISTS {ks} WITH REPLICATION = {{'class' : 'NetworkTopologyStrategy', 'replication_factor' : 1}}")).await.unwrap();
390+
390391
session
391392
.ddl(
392393
format!(
393-
"CREATE TABLE IF NOT EXISTS {ks}.t (a int PRIMARY KEY, b vector<int, 4>, c vector<text, 2>)"
394+
"CREATE TABLE IF NOT EXISTS {ks}.t (a int PRIMARY KEY, b vector<int, 4>, c vector<text, 2>, d vector<frozen<list<set<int>>>, 2>)"
394395
),
395396
)
396397
.await
397398
.unwrap();
398399

400+
let complex_vector: Vec<Vec<HashSet<i32>>> = vec![
401+
vec![HashSet::from([1, 2, 3]), HashSet::from([4, 5])],
402+
vec![HashSet::from([6, 7, 8])],
403+
];
404+
399405
session
400406
.query_unpaged(
401-
format!("INSERT INTO {ks}.t (a, b, c) VALUES (?, ?, ?)"),
402-
&(1, vec![1, 2, 3, 4], vec!["foo", "bar"]),
407+
format!("INSERT INTO {ks}.t (a, b, c, d) VALUES (?, ?, ?, ?)"),
408+
&(1, vec![1, 2, 3, 4], vec!["foo", "bar"], &complex_vector),
403409
)
404410
.await
405411
.unwrap();
406412

407413
session
408414
.query_unpaged(
409-
format!("INSERT INTO {ks}.t (a, b, c) VALUES (?, ?, ?)"),
410-
&(2, &[5, 6, 7, 8][..], &["afoo", "abar"][..]),
415+
format!("INSERT INTO {ks}.t (a, b, c, d) VALUES (?, ?, ?, ?)"),
416+
&(2, &[5, 6, 7, 8][..], &["afoo", "abar"][..], &complex_vector),
411417
)
412418
.await
413419
.unwrap();
414420

415421
let query_result = session
416-
.query_unpaged(format!("SELECT a, b, c FROM {ks}.t"), &[])
422+
.query_unpaged(format!("SELECT a, b, c, d FROM {ks}.t"), &[])
417423
.await
418424
.unwrap();
419425

420-
let rows: Vec<(i32, Vec<i32>, Vec<String>)> = query_result
426+
type RowType = (i32, Vec<i32>, Vec<String>, Vec<Vec<HashSet<i32>>>);
427+
let rows: Vec<RowType> = query_result
421428
.into_rows_result()
422429
.unwrap()
423-
.rows::<(i32, Vec<i32>, Vec<String>)>()
430+
.rows::<RowType>()
424431
.unwrap()
425432
.map(|r| r.unwrap())
426433
.collect();
@@ -429,15 +436,17 @@ async fn test_vector_type_unprepared() {
429436
(
430437
1,
431438
vec![1, 2, 3, 4],
432-
vec!["foo".to_string(), "bar".to_string()]
439+
vec!["foo".to_string(), "bar".to_string()],
440+
complex_vector.clone(),
433441
)
434442
);
435443
assert_eq!(
436444
rows[1],
437445
(
438446
2,
439447
vec![5, 6, 7, 8],
440-
vec!["afoo".to_string(), "abar".to_string()]
448+
vec!["afoo".to_string(), "abar".to_string()],
449+
complex_vector,
441450
)
442451
);
443452

0 commit comments

Comments
 (0)