Improve error for unsupported result type. #2585
Conversation
| @@ -206,8 +206,10 @@ func (e Executor) execStmt(stmt parser.Statement, params parameters, planMaker * | |||
| row.Values = append(row.Values, driver.Datum{ | |||
| Payload: &driver.Datum_IntervalVal{IntervalVal: vt.Nanoseconds()}, | |||
| }) | |||
| case parser.DTuple: | |||
| return fmt.Errorf("unsupported result type: tuple, remove extra parenthesis around column %s", result.Columns[i]) | |||
There was a problem hiding this comment.
I dont think this advice is appropriate. other databases support the tuple return; it's fine that we do not, but this advice is inappropriate because it changes the semantics. The new verbiage in the default clause LGTM.
There was a problem hiding this comment.
Agreed that the advice is inappropriate. I think we should have a specific error message for trying to return a tuple. Something like: "unsupported tuple result".
There was a problem hiding this comment.
I don't want folks to see parser.DTuple, and I much prefer having error messages provide guidance on what to do. Should I say "you might want to remove the extra..."
There was a problem hiding this comment.
@vivekmenezes you can get "tuple" instead of "parser.DTuple" by calling Type() on the datum.
There was a problem hiding this comment.
I'll change it to what Tamir suggests. You are sure you don't want the recommendation "you might want to remove..." right?
There was a problem hiding this comment.
yeah I suppose with "tuple" it's probably good enough. Changing
vivekmenezes
force-pushed
the
vivek/distinct-fix
branch
from
1739505 to
f3613d7
Compare
vivekmenezes
changed the title
|
LGTM |
Improve error for unsupported result type.
No description provided.