joao-r-reis commented on code in PR #1828:
URL:
https://github.com/apache/cassandra-gocql-driver/pull/1828#discussion_r1810723411
##########
helpers.go:
##########
@@ -162,47 +164,83 @@ func getCassandraBaseType(name string) Type {
}
}
-func getCassandraType(name string, logger StdLogger) TypeInfo {
+func getCassandraType(name string, protoVer byte, logger StdLogger) TypeInfo {
if strings.HasPrefix(name, "frozen<") {
- return getCassandraType(strings.TrimPrefix(name[:len(name)-1],
"frozen<"), logger)
+ return getCassandraType(strings.TrimPrefix(name[:len(name)-1],
"frozen<"), protoVer, logger)
} else if strings.HasPrefix(name, "set<") {
return CollectionType{
- NativeType: NativeType{typ: TypeSet},
- Elem:
getCassandraType(strings.TrimPrefix(name[:len(name)-1], "set<"), logger),
+ NativeType: NativeType{typ: TypeSet, proto: protoVer},
+ Elem:
getCassandraType(strings.TrimPrefix(name[:len(name)-1], "set<"), protoVer,
logger),
}
} else if strings.HasPrefix(name, "list<") {
return CollectionType{
- NativeType: NativeType{typ: TypeList},
- Elem:
getCassandraType(strings.TrimPrefix(name[:len(name)-1], "list<"), logger),
+ NativeType: NativeType{typ: TypeList, proto: protoVer},
+ Elem:
getCassandraType(strings.TrimPrefix(name[:len(name)-1], "list<"), protoVer,
logger),
}
} else if strings.HasPrefix(name, "map<") {
names :=
splitCompositeTypes(strings.TrimPrefix(name[:len(name)-1], "map<"))
if len(names) != 2 {
logger.Printf("Error parsing map type, it has %d
subelements, expecting 2\n", len(names))
return NativeType{
- typ: TypeCustom,
+ proto: protoVer,
+ typ: TypeCustom,
}
}
return CollectionType{
- NativeType: NativeType{typ: TypeMap},
- Key: getCassandraType(names[0], logger),
- Elem: getCassandraType(names[1], logger),
+ NativeType: NativeType{typ: TypeMap, proto: protoVer},
+ Key: getCassandraType(names[0], protoVer,
logger),
+ Elem: getCassandraType(names[1], protoVer,
logger),
}
} else if strings.HasPrefix(name, "tuple<") {
names :=
splitCompositeTypes(strings.TrimPrefix(name[:len(name)-1], "tuple<"))
types := make([]TypeInfo, len(names))
for i, name := range names {
- types[i] = getCassandraType(name, logger)
+ types[i] = getCassandraType(name, protoVer, logger)
}
return TupleTypeInfo{
- NativeType: NativeType{typ: TypeTuple},
+ NativeType: NativeType{typ: TypeTuple, proto: protoVer},
Elems: types,
}
+ } else if strings.HasPrefix(name, "udt<") {
Review Comment:
Does this even exist? I think that the actual UDT name would appear here
instead of `udt<>` so I think this entire `if` block is not used
##########
helpers.go:
##########
@@ -236,19 +274,48 @@ func splitCompositeTypes(name string) []string {
}
func apacheToCassandraType(t string) string {
- t = strings.Replace(t, apacheCassandraTypePrefix, "", -1)
t = strings.Replace(t, "(", "<", -1)
t = strings.Replace(t, ")", ">", -1)
types := strings.FieldsFunc(t, func(r rune) bool {
return r == '<' || r == '>' || r == ','
})
- for _, typ := range types {
- t = strings.Replace(t, typ,
getApacheCassandraType(typ).String(), -1)
+ skip := 0
+ for _, class := range types {
+ class = strings.TrimSpace(class)
+ if !isDigitsOnly(class) {
+ // vector types include dimension (digits) as second
type parameter
+ // UDT fields are represented in format {field
id}:{class}, example
66697273745f6e616d65:org.apache.cassandra.db.marshal.UTF8Type
+ if skip > 0 {
+ skip -= 1
+ continue
+ }
+ idx := strings.Index(class, ":")
+ class = class[idx+1:]
+ act := getApacheCassandraType(class)
+ val := act.String()
+ switch act {
+ case TypeUDT:
+ val = "udt"
Review Comment:
Shouldn't we just set this to the actual `udt` name? This would be what
Cassandra returns in the short name format I believe.
##########
metadata_test.go:
##########
@@ -731,7 +733,7 @@ func assertParseCompositeType(
) {
log := &defaultLogger{}
- result := parseType(def, log)
+ result := parseType(def, 4, log)
Review Comment:
same here
##########
metadata_test.go:
##########
@@ -700,7 +702,7 @@ func assertParseNonCompositeType(
) {
log := &defaultLogger{}
- result := parseType(def, log)
+ result := parseType(def, 4, log)
Review Comment:
Use the constant for the protocol version so it is easier to read.
##########
frame.go:
##########
@@ -928,6 +929,30 @@ func (f *framer) readTypeInfo() TypeInfo {
collection.Elem = f.readTypeInfo()
return collection
+ case TypeCustom:
+ if strings.HasPrefix(simple.custom, VECTOR_TYPE) {
+ // TODO(lantoniak): There are currently two ways of
parsing types in the driver.
+ // a) using getTypeInfo()
+ // b) using parseType()
+ // I think we could agree to use getTypeInfo() when
parsing binary type definition
+ // and parseType() would be responsible for parsing
"custom" string definition.
+ //spec := strings.TrimPrefix(simple.custom, VECTOR_TYPE)
+ //spec = spec[1 : len(spec)-1] // remove parenthesis
+ //idx := strings.LastIndex(spec, ",")
+ //typeStr := spec[:idx]
+ //dimStr := spec[idx+1:]
+ //subType := getTypeInfo(strings.TrimSpace(typeStr),
f.proto, nopLogger{})
+ //dim, _ := strconv.Atoi(strings.TrimSpace(dimStr))
+ result := parseType(simple.custom, simple.proto,
nopLogger{})
Review Comment:
It looks like `getTypeInfo()` is meant to be used for both cases because it
checks for the `org.apache.cassandra.db.marshal.` prefix. My opinion is that we
should stay away from the `parseType` function and `typeParser` type since it
looks like it hasn't been maintained for a very long time and it is only being
used in C* versions below `3.0`. At some point we can delete all of that code
when we decide to drop support for these old C* versions.
##########
helpers.go:
##########
@@ -162,47 +164,83 @@ func getCassandraBaseType(name string) Type {
}
}
-func getCassandraType(name string, logger StdLogger) TypeInfo {
+func getCassandraType(name string, protoVer byte, logger StdLogger) TypeInfo {
Review Comment:
It's interesting that `protoVer` wasn't being set before in a lot of
cases... Can we replace all instances of direct struct initialization by a call
to the `NewNativeType()` function? It would make it clear in the future what
fields need to be provided. Could also create a utility private function that
always sets `custom` to `nil` to avoid having to provide `nil` on every call.
##########
helpers.go:
##########
@@ -236,19 +274,48 @@ func splitCompositeTypes(name string) []string {
}
func apacheToCassandraType(t string) string {
- t = strings.Replace(t, apacheCassandraTypePrefix, "", -1)
t = strings.Replace(t, "(", "<", -1)
t = strings.Replace(t, ")", ">", -1)
types := strings.FieldsFunc(t, func(r rune) bool {
return r == '<' || r == '>' || r == ','
})
- for _, typ := range types {
- t = strings.Replace(t, typ,
getApacheCassandraType(typ).String(), -1)
+ skip := 0
+ for _, class := range types {
+ class = strings.TrimSpace(class)
+ if !isDigitsOnly(class) {
Review Comment:
I think we should call `getApacheCassandraType(class)` and then if the
returned value matches the vector type then we keep the "dimensions" parameter
as is and if it matches UDT then we replace `UserType` with the actual UDT name
and remove the other 2 parameters. No need to check `isDigitsOnly`. We need to
use a `for` loop with an index instead of `for ... range` though.
##########
helpers.go:
##########
@@ -236,19 +274,48 @@ func splitCompositeTypes(name string) []string {
}
func apacheToCassandraType(t string) string {
Review Comment:
Similar to above, small comment to clarify that this function is meant to
convert java style long cql type names into the standard short cql type names.
##########
helpers.go:
##########
@@ -236,19 +274,48 @@ func splitCompositeTypes(name string) []string {
}
func apacheToCassandraType(t string) string {
- t = strings.Replace(t, apacheCassandraTypePrefix, "", -1)
t = strings.Replace(t, "(", "<", -1)
t = strings.Replace(t, ")", ">", -1)
types := strings.FieldsFunc(t, func(r rune) bool {
return r == '<' || r == '>' || r == ','
})
- for _, typ := range types {
- t = strings.Replace(t, typ,
getApacheCassandraType(typ).String(), -1)
+ skip := 0
+ for _, class := range types {
+ class = strings.TrimSpace(class)
+ if !isDigitsOnly(class) {
+ // vector types include dimension (digits) as second
type parameter
+ // UDT fields are represented in format {field
id}:{class}, example
66697273745f6e616d65:org.apache.cassandra.db.marshal.UTF8Type
+ if skip > 0 {
+ skip -= 1
+ continue
+ }
+ idx := strings.Index(class, ":")
+ class = class[idx+1:]
+ act := getApacheCassandraType(class)
+ val := act.String()
+ switch act {
+ case TypeUDT:
+ val = "udt"
+ skip = 2 // skip next two parameters (keyspace
and type ID), do not attempt to resolve their type
+ case TypeCustom:
+ val = getApacheCassandraCustomSubType(class)
Review Comment:
why not use the existing `getApacheCassandraType`?
##########
frame.go:
##########
@@ -928,6 +929,30 @@ func (f *framer) readTypeInfo() TypeInfo {
collection.Elem = f.readTypeInfo()
return collection
+ case TypeCustom:
+ if strings.HasPrefix(simple.custom, VECTOR_TYPE) {
+ // TODO(lantoniak): There are currently two ways of
parsing types in the driver.
+ // a) using getTypeInfo()
+ // b) using parseType()
+ // I think we could agree to use getTypeInfo() when
parsing binary type definition
+ // and parseType() would be responsible for parsing
"custom" string definition.
+ //spec := strings.TrimPrefix(simple.custom, VECTOR_TYPE)
+ //spec = spec[1 : len(spec)-1] // remove parenthesis
+ //idx := strings.LastIndex(spec, ",")
+ //typeStr := spec[:idx]
+ //dimStr := spec[idx+1:]
+ //subType := getTypeInfo(strings.TrimSpace(typeStr),
f.proto, nopLogger{})
+ //dim, _ := strconv.Atoi(strings.TrimSpace(dimStr))
+ result := parseType(simple.custom, simple.proto,
nopLogger{})
Review Comment:
I think parseType is not meant to be used this way, from what I've gathered
it is only used to parse some table metadata fields IF the C* version is below
3.0.0 so it is very old code.
##########
metadata_test.go:
##########
@@ -636,12 +636,14 @@ func TestTypeParser(t *testing.T) {
},
)
- // custom
+ // udt
assertParseNonCompositeType(
t,
"org.apache.cassandra.db.marshal.UserType(sandbox,61646472657373,737472656574:org.apache.cassandra.db.marshal.UTF8Type,63697479:org.apache.cassandra.db.marshal.UTF8Type,7a6970:org.apache.cassandra.db.marshal.Int32Type)",
- assertTypeInfo{Type: TypeCustom, Custom:
"org.apache.cassandra.db.marshal.UserType(sandbox,61646472657373,737472656574:org.apache.cassandra.db.marshal.UTF8Type,63697479:org.apache.cassandra.db.marshal.UTF8Type,7a6970:org.apache.cassandra.db.marshal.Int32Type)"},
+ assertTypeInfo{Type: TypeUDT, Custom: ""},
)
+
+ // custom
Review Comment:
should add a vector metadata test here
##########
helpers.go:
##########
@@ -162,47 +164,83 @@ func getCassandraBaseType(name string) Type {
}
}
-func getCassandraType(name string, logger StdLogger) TypeInfo {
+func getCassandraType(name string, protoVer byte, logger StdLogger) TypeInfo {
Review Comment:
On a separate note, providing go docs for this function would be great.
Something that clarifies that this function is meant to parse the "short" cql
type names instead of the "long" java type names.
--
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]
