implemented query hints (flatten) (fb-2124)

[paddyjok]

This PR implements query options hints that govern behavior for distinct and group by on set columns.

1. introduces syntax to support query hints on table source references ...from table_name with (hint(param))
2. introduces the flatten(column) hint
3. ensures distinct with and without flatten with a single set column ref works correctly
4. ensures group by with and without flatten with a single set column ref works correctly

[seebs]

This looks good but I would like to push for slightly more comprehensive testing just because this is a pretty major new feature surface.

[seebs]

This doesn't look like ids1 got flattened?

I am ... not actually sure what I expect here. I think I sort of expect the id set column values to all be 1, 2, 3, or 4, rather than sets, because they're being flattened. And I think that implies something like

1, {"a", "b"}
2, {"a", "b"}
3, {"d", "e"}
4, {"d", "e"}

etcetera? but now i'm not sure.

[seebs]

so, the behavior of flatten when there's more than one column being selected is "just ignore it", which i think maybe ought to get a ticket, but that's the understood/expected behavior, so i think we can be okay with this for now? but maybe it should be an error to specify a flatten that won't have any effect, because otherwise it might confuse users.

So i'm okay with this as is, although i think i'd prefer "error on a flatten we won't do" because otherwise users won't understand why it's not happening, if we can do that now. otherwise, i guess, ticket it for doing that next.

[seebs]

looking at the CI failures, they look intentional in that we changed the semantics of group by. e.g., we would now need:

--- a/sql3/test/defs/defs_groupby.go
+++ b/sql3/test/defs/defs_groupby.go
@@ -225,7 +225,7 @@ var groupByTests = TableTest{
                },
                {
                        SQLs: sqls(
-                               "SELECT COUNT(*), is1 FROM groupby_test group by is1",
+                               "SELECT COUNT(*), is1 FROM groupby_test with(flatten(is1)) group by is1",
                        ),
                        ExpHdrs: hdrs(
                                hdr("", fldTypeInt),

[seebs]

We do need to fix the existing tests, because we've fixed group by semantics to match SQL semantics by default, which is intentional, but it does mean the existing tests will need updates. I'm fine with either direction of changing that (changing expected test results or adding the with(flatten(...)) to them).

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
8.2% Duplication