cdc: support geometry types with the validator #134749
Conversation
|
This change is |
wenyihu6
changed the title
wenyihu6
changed the title
wenyihu6
force-pushed
the
rt
branch
7 times, most recently
from
0d7a4b1 to
db6d239
Compare
|
|
||
| func TestExtractFieldFromJSON(t *testing.T) { | ||
| defer leaktest.AfterTest(t)() | ||
| skip.WithIssue(t, 134904) |
There was a problem hiding this comment.
Note that this test is skipped with #134904.
wenyihu6
requested review from
asg0451 and
rharding6373
and removed request for
a team
| if err != nil { | ||
| return err | ||
| } | ||
| afterDatums, err := parseJSON(afterJson) |
There was a problem hiding this comment.
nit: naming: these arent datums, they're "go values"
There was a problem hiding this comment.
Discussed this offline. The name for Datums was adopted because we pass it as a RowDatum
afterValueDatums to be a bit clearer.
| return ret, err | ||
| } | ||
|
|
||
| func parseJSON(d json.JSON) (map[string]interface{}, error) { |
There was a problem hiding this comment.
i don't love this function's name -- it takes an already parsed crdb json and transforms it into go datatypes, including some weird geometry parsing stuff. can you update the name to reflect this?
There was a problem hiding this comment.
Renamed it to convertJSONToMap. Do you like this more?
| return nil, nil | ||
| } | ||
| ret := make(map[string]interface{}) | ||
| switch d.Type() { |
There was a problem hiding this comment.
also, the way this function is used means that the input is always an object. can you simplify it using this? i'm also not convinced using the crdb json stuff is adding value here over just using gojson. if so you could just pass a map[string]any into here and greatly simplify things
There was a problem hiding this comment.
Discussed this offline as well. Usually we take in Object, but we take in Null JSON sometimes as well. Addressed it by adding a case specifically for case json.NullJSONType:.
There was a problem hiding this comment.
i'm also not convinced using the crdb json stuff is adding value here over just using gojson.
Chatted this offline as well. It is a bit tricky here since parsing it using gojson would require us to parse it to an interface first and then convert this interface back to a JSON type so that we can use geo.ParseGeographyFromGeoJSON if this interface turns out to be for a geometry type. We decided to just keep it this way for now and will revisit and see if there is a better way to handle this in the future.
| "github.com/cockroachdb/cockroach/pkg/workload/rand" | ||
| ) | ||
|
|
||
| func extractFieldFromJSON(d json.JSON) (interface{}, error) { |
There was a problem hiding this comment.
can you make this name more descriptive or add a comment about what it's doing? maybe like attempts to coerce json data into a known go type, including geo or something
There was a problem hiding this comment.
Done. Added a comment.
wenyihu6
force-pushed
the
rt
branch
2 times, most recently
from
56e679d to
5870660
Compare
This patch supports geometry.geography and array types. We recognize there may be uncovered edge cases, but we’ve chosen a lenient approach as this is for testing purposes, and we want to avoid a potential time sink. We plan to revisit and refine this as we progress with the randomized testing framework. Release note: none Part of: cockroachdb#124146
|
Friendly ping for reviews here : ) |
There was a problem hiding this comment.
i'm literally looking at it right now ;)
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @rharding6373)
There was a problem hiding this comment.
Reviewable status:
|
lol i will be more patient next time TFTR! bors r=asg0451 |
This patch supports geometry,geography and array types.
We recognize there may be uncovered edge cases, but
we’ve chosen a lenient approach as this is for
testing purposes, and we want to avoid a potential
time sink. We plan to revisit and refine this as
we progress with the randomized testing framework.
Release note: none
Part of: #124146