Fix ss-159: empty text columns panics#36890
Conversation
This reverts commit fbabae7.
| } | ||
| } | ||
|
|
||
| async fn purify_create_table_from_source( |
There was a problem hiding this comment.
Hmmmmmmm.. MZ currently has 2 flavors of SOURCE syntax - old and new. I expected that we were already rejecting this behavior in the old syntax, but it turns out I was wrong! The old accepts TEXT COLUMNS = () (see purify_create_source and purify_alter_source_add_subsource). I favor being consistent in how these behave.
Given that, I'm rethinking my stance on having this be an error. Since the behavior today is that we always accept it (soft_panic_or_log! is not a panic in production). I feel like changing behavior for the command across the board has the potential to break someone. What do you think about just accepting empty?
There was a problem hiding this comment.
I'm comfortable with accepting empty for the sake of avoiding backwards-incompatible changes.
I'm less familiar with how to weigh some of the other points you brought up in standup -- for example, it sounds like in SQL an IN will error if you try to pass in IN (). This was counter to my intuition that would have said it should just evaluate to false rather than fail outright.
There was a problem hiding this comment.
My reasoning is based on failing fast. It's more correct to error on missing information in a DDL statement (fail at setup before ingesting data). If I went to create an index with no columns, it wouldn't make any sense to just not create the index and let the user find out later when query performance is terrible.
Granted, at the time I thought that's what were doing in other code paths, but that isn't the case. This situation isn't so severe that I'd be willing to break existing uses of the syntax.
| None => soft_panic_or_log!( | ||
| "text_columns should be Some if text_cols_option is present" | ||
| ), | ||
| if let Some(gen_text_columns) = gen_text_columns { |
There was a problem hiding this comment.
because gen_text_columns is an Option, you can use .map instead of if let Some(..)
text_cols_option.value = gen_text_columns.map(WithOptionValue::Sequence);
There was a problem hiding this comment.
I think this is technically slightly different behavior in the (unexpected to me) case that text_cols_option.value is not None and gen_text_columns is None.
Agreed this is nicer though.
There was a problem hiding this comment.
Ope, I think this did cause an issue because when we get TEXT COLUMNS () that actually maps to Some with an empty vector in text_cols_option.value. So I think I need to stick with the current code or something more like this earlier version of the PR 180bbb6 for it to work without broader changes.
This reverts commit dda02ca.
2 participants
Motivation
Closes SS-159
Description
Return an error instead of panicking when the
TEXT COLUMNSis specified but the list is empty().Verification
Testdrive test, graciously provided by @def-, thanks @def-