SQL-3387: Implement support for ARRAY_REMOVE with proper semantics - #194
SQL-3387: Implement support for ARRAY_REMOVE with proper semantics#194mattChiaravalloti wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
If you are looking in GitHub order, this is the first file you'll see. This is pretty much what you'll see in all test files! The flag is introduced with the false value wherever necessary. In some files my IDE also made local edits removing extraneous commas and newlines. It looks like none of those types of edits are in this file but that's the only other type of change you'll see in test files.
There was a problem hiding this comment.
Thile includes a mod force_mql_semantics at the bottom with new unit test coverage.
| function, | ||
| is_nullable, | ||
| args, | ||
| force_mql_semantics, |
There was a problem hiding this comment.
The only ast struct that defines force_mql_semantics is ast::BinaryExpr. That is why this is the only spot where we set the value. All other locations use false.
| is_nullable: sf.is_nullable, | ||
| args: vec![arg.clone(), top], | ||
| force_mql_semantics: sf.force_mql_semantics, | ||
| is_nullable: arg_is_nullable || top_is_nullable, |
There was a problem hiding this comment.
🚗 drive-by minor correctness improvement here. It's actually inconsequential given the way this entire module is implemented but it's better to be correct here in case other changes are made around this later.
There was a problem hiding this comment.
This file contains unit tests for relevant functions.
1 participant
This PR concludes the array functions work for the Dialect Improvement project. It implements support for
ARRAY_REMOVEwith the intended semantics; that is, it supportsARRAY_REMOVEsuch that null elements are not removed from the array unless the specified value to remove is itself null. See the spec/query tests for details.The ticket proposed indicating the
FilterExprinclude a flag to force MQL semantics (as opposed to SQL 3-value null semantics) but I ultimately decided to push that lower, to the expression nested within theFilterExpr. The reason I did this is because it is really theNeqexpression within the rewrite that needs to force MQL semantics, not theFilterExpritself. That flag would be too powerful and ambiguous if it was available at theFilterExprlevel.I documented at the
astandmirlevels why the flag exists so that future implementors could avoid misusing it.Apologies for the size of the PR. It is mostly a consequence of adding a new flag to two pretty popular structs. For the most part, the only relevant files are
parsers/mongosql.lalrpop,ast/definitions.rs,ast/rewrites/higher_order_functions.rs,algebrizer/definitions.rs,mir/definitions.rs,translator/{expression.rs, util.rs}, andtests/spec_tests/query_tests/array_functions.yml. All other files are incidental updates that simply introduce the new flag with the default value (false). I did add unit testing for relevant functions, if you are interested in those.