sql: change checkResultDatum to use a separate error for DValArg#4162
sql: change checkResultDatum to use a separate error for DValArg#4162
Conversation
sql/executor.go
Outdated
| case parser.DTimestamp: | ||
| case parser.DInterval: | ||
| case parser.DValArg: | ||
| return fmt.Errorf("could not infer %s", datum.Type()) |
There was a problem hiding this comment.
I think this should be return fmt.Errorf("could not determine data type of parameter %s", d.name) with switch d := datum.(type) { at the top. This matches postgres' error message.
There was a problem hiding this comment.
Ah, I see. Fixed. (Used just d as it seems d.String() is what we want.)
ac9022f to
16548dd
Compare
sql/executor.go
Outdated
| case parser.DTimestamp: | ||
| case parser.DInterval: | ||
| case parser.DValArg: | ||
| return fmt.Errorf("could not determine data type of parameter %s", d) |
There was a problem hiding this comment.
Ah, nice. If you wanted to golf a bit more, you could even get rid of d and just use datum here.
|
LGTM |
16548dd to
037dbab
Compare
sql/pgwire_test.go
Outdated
| "SELECT $1 > 0 AND NOT $1": "pq: incompatible NOT argument type: int", | ||
| "SELECT $1": "pq: unsupported result type: valarg", | ||
| "SELECT $1": "pq: could not determine data type of parameter $1", | ||
| "SELECT $1 + $1": "pq: unsupported binary operator: <valarg> + <valarg>", |
There was a problem hiding this comment.
I mentioned this in another PR, but it would be nice to replace all the uses of valarg in error messages with something more user friendly, like parameter.
There was a problem hiding this comment.
I see. Pushed another commit to change DValArg.Type() to return "parameter". Does this look fine?
Change other cases to return util.Errorf().
037dbab to
4c6b873
Compare
sql/executor.go
Outdated
| case parser.DTimestamp: | ||
| case parser.DInterval: | ||
| case parser.DValArg: | ||
| return fmt.Errorf("could not determine data type of parameter %s", datum) |
There was a problem hiding this comment.
you can use Type() instead of literally parameter now
4c6b873 to
defbbd9
Compare
|
🚢 |
"parameter" would be more user-friendly than "valarg".
defbbd9 to
c5bddd7
Compare
sql: change checkResultDatum to use a separate error for DValArg
Most of these commits are from [engine: Add ExportToSst method with all logic moved to C++](cockroachdb@75f1314). It is not currently testable (PASS on my local) and can be checked when [roachtest: add flag (or env var) to request pebble cockroachdb#4162](cockroachdb#41620) is completed. Fixes cockroachdb#41739 Release note: None
Change other cases to return
util.Errorf().@tamird , does this look good?