SQL-3294: Update schema checking for variadic operators to require minimum argument counts#171
SQL-3294: Update schema checking for variadic operators to require minimum argument counts#171mattChiaravalloti wants to merge 6 commits into
Conversation
…ions and usages of IncorrectArgumentCount
…nimum argument counts
|
|
||
| test_schema!( | ||
| and_first_arg_is_not_bool_is_error, | ||
| first_arg_is_not_bool_is_error, |
There was a problem hiding this comment.
drive-by clean up: the module is already called and, so there's no need to prefix each test with and_ since that ultimately displays the tests as mir::schema::test::expressions::scalar_functions::and::and_<test description>. By removing the and_ prefixes (and the or_ and concat_ prefixes for their modules), the output will now read more naturally: mir::schema::test::expressions::scalar_functions::and::<test description>.
| ); | ||
|
|
||
| test_schema!( | ||
| more_than_two_args, |
There was a problem hiding this comment.
Since we allow (and even construct, as part of the flatten_variadics optimization) variadic And functions. This same concept applies to Or and Concat. I did not include new tests for Add or Mul since this "more than 2 args" behavior is already implicitly covered by a number of our unit tests for those functions.
| mod arithmetic { | ||
| use super::*; | ||
|
|
||
| test_schema!( |
There was a problem hiding this comment.
These tests should never have been added. It seems more a consequence of implementation rather than a planned decision to support this feature. It is syntactically invalid to create an Add or Mul expression without at least 2 operands, and we should also reflect that semantically. (Note: unary addition does exist as the Pos operator, but that is determined syntactically and is represented distinctly in the mir so there is no risk here of unary addition failing this test).
| algebrize_error: 'Error 1001: incorrect argument count for Round: required exactly 2, found 3' | ||
|
|
||
| - description: Error 1001 IncorrectArgumentCount with Minimum requirement | ||
| query: "SELECT MAP(a, +) FROM foo" |
There was a problem hiding this comment.
This will ultimately be rewritten to MAP(a, this +) and will be rejected at semantic validation time since the arg count is too low. This is actually the motivation behind this work, as noted in the tech design!
1 participant
This PR updates schema checking for variadic operators to require minimum argument counts, as noted in the design doc. This set of operators includes
Add,And,Concat,Mul, andOr. This is the set of "variadic operators" we support in theflatten_variadicsoptimization, and they also correspond to the set of functions in schema checking for which we did not include an arg-count assertion already (either directly or implicitly).As part of this change, I updated the
IncorrectArgumentCounterror to include info on precision. Now, it reads either "required exactly X, found Y" or "required at least X, found Y", depending on how the error is constructed. This update to the error variant is why there are so many changes. The core change, really, is the introduction ofensure_minimum_arg_countand its three uses.