smiklosovic commented on code in PR #4411:
URL: https://github.com/apache/cassandra/pull/4411#discussion_r2454625820
##########
src/java/org/apache/cassandra/schema/Types.java:
##########
@@ -487,7 +555,20 @@ public Types deserialize(String keyspace, DataInputPlus
in, Version version) thr
List<String> fieldTypes = new ArrayList<>(fieldTypeSize);
for (int x = 0; x < fieldTypeSize; x++)
fieldTypes.add(in.readUTF());
- builder.add(name, fieldNames, fieldTypes);
+ String comment = EMPTY_COMMENT;
+ String securityLabel = EMPTY_SECURITY_LABEL;
+ List<String> fieldComments = EMPTY_FIELD_COMMENTS;
Review Comment:
so, what you did here is that you have replaced this by _static_ class
variable of `new ArrayList<>()` type. So upon deserialization you will use
that static variable and you go with it to `deserializeFieldMetadata`. There
you add comments to _static variable_. Then upon next deserialisation, you
would reuse that array list again, because it is static? With the comments from
the previous deserialisation?
You do not want to do this. What you want to do is to break
`deserializeFieldMetadata` into two methods. Each returning List<String> and
you return `List.of(EMPTY_COMMENT)` in case there are no comments.
This whole logic should be rewritten like here
https://github.com/smiklosovic/cassandra/commit/cf9609236bc97f51c8caaf7531b3352f1d6fdab7
I started to do that but I find the way how you serialize fields a little
bit cumbersome. What you do in `deserializeFieldMetadata` is that you
1. read how many fields there is
2. you loop over fields and you read boolean to see if there is a comment
3. you read if there is a comment and if not you add empty comment to list
4. you read if there is a security label and if not you add empty label to
another list
5. these lists are passed into method as parameters.
I do not think that you should "inject" these lists into that method. I
think you should adopt the style I have shown in the above link. It is way more
declarative and way easier to follow what is happening and you do not reuse
variables / reassign them and so on.
What I could not achieve with your current style of deserialisation as
described in the steps above is to separate deserialisation of comments and
labels into two methods. If you think about that, the deserialisation of
comments and labels are basically the same - twice. Same logic. So why could
not I just use `deserializeFieldCommentsMetadata` _twice_?
List<String> comments = deserializeFieldCommentsMetadata(in);
List<String> labels = deserializeFieldCommentsMetadata(in);
Right? But for now I can not do this because you are serialising it in such
a way that you basically mix comments with labels.
`comment1|label1|comment2|label2...`
instead of
`commen1|comment2|comment3|label1|label2|label3...`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
