uber-go / dosa

DOSA is a data object abstraction layer
MIT License
197 stars 37 forks source link

index columns zero value is []string(nil) but not []string{} #383

Closed skywhat closed 5 years ago

skywhat commented 5 years ago

Why I am doing this change? Here is the error messages in integration test after using this dosa version.

Err: \"incompatible schema: index \\\"searchbystr\\\" mismatch: (&{(strv, an_uuid_key ASC, strkey ASC, int64key ASC) []} vs &{(strv, an_uuid_key ASC, strkey ASC, int64key ASC) []})\",",

After print the struct variables to explore more.

index searchbystr mismatch: (Key: &{(strv, an_uuid_key ASC, strkey ASC, int64key ASC), Columns: []string{}} vs &{ Key: (strv, an_uuid_key ASC, strkey ASC, int64key ASC) , Columns: []string(nil)}

They are not deep equal. After I debug throughout the whole lifecycle how the values change from DOSA cli to gateway to Schema service. I found out the value Columns: nil changed into Columns: []string{} right here. https://sourcegraph.uberinternal.com/code.uber.internal/infra/dosa-gateway@1e2ac98c230a95b1a9762078b4545d233022ad8f/-/blob/datastore/cassandra/connector.go#L50 So the Clone API messed it up.

the zero value of index columns if it's not specified should be []string(nil) https://github.com/uber-go/dosa/blob/master/entity_parser.go#L355

ben-mays commented 5 years ago

Sorry if I'm naive, I'm still getting ramped up but I have 2 questions:

skywhat commented 5 years ago

Sorry if I'm naive, I'm still getting ramped up but I have 2 questions:

  • Do we provide a specification for our entity parser that outlines the behavior for various zero values? It appears we're struggling with the implicit behavior and this may continue to change in the future with the evolution of the language. Maybe we can define that specification exhaustively and then enforce it?
  • Can you update the PR body with the problem we're trying to solve, how the existing behavior is broken and how this change will fix it?

update the PR body to explain more why I am making this PR.

Answer ur questions:

  1. Actually this change has nothing to do with entity parser. Because dosa cli sending nil from the very beginning, but somewhere in dosa-gateway, when it called Clone, the value changed from []string(nil) to []string{}, then it's messed up. CheckSchema successfully check whether the value is deep equal with the one in schema service.
  2. update in the first comment.