SQL: wrong number of values for columns

[emasab]

No description provided.

[emasab]

Closes #41811

[emasab]

Hi I've done three bugfixes, done some test locally and added two unit tests.

Cheers

[elasticmachine]

Pinging @elastic/es-search

[tvernum]

@elasticmachine test this please

[tvernum]

@elasticmachine test this please

[astefan]

In general it looks good. I left some comments mostly cosmetic.
Also, I would like to see some integration tests, as well. For example, x-pack\plugin\sql\qa\src\main\resources\select.sql-spec with actual queries involving duplicated columns being returned. An example query: SELECT salary, first_name, salary AS x FROM test_emp ORDER BY x LIMIT 3.

Also, in elasticsearch\x-pack\plugin\sql\qa\src\main\resources\agg.sql-spec I would add more queries. One example: SELECT gender AS g, gender, SUM(salary) AS s3, SUM(salary), SUM(salary) AS s5 FROM test_emp GROUP BY 2

[matriv]

@emasab It would be great to also add tests to check if something breaks for the ordering on aggregates: https://github.com/elastic/elasticsearch/blob/master/x-pack/plugin/sql/qa/src/main/resources/agg-ordering.sql-spec when there is a duplicate column.

[astefan]

@emasab did you get the chance to review our feedback?

[emasab]

@astefan yes, it's almost ready. I was working on the other PR before

[emasab]

made the changes and added some integration tests but these three are failing, although they produce correct results when called manually through the rest api.

aggSumWithColumnRepeated
SELECT gender AS g, gender, SUM(salary) AS s3, SUM(salary), SUM(salary) AS s5 FROM test_emp GROUP BY gender;

aggSumWithAliasWithColumnRepeated
SELECT gender AS g, gender, SUM(salary) AS s3, SUM(salary), SUM(salary) AS s5 FROM test_emp GROUP BY g;

aggSumWithNumericRefWithColumnRepeated
SELECT gender AS g, gender, SUM(salary) AS s3, SUM(salary), SUM(salary) AS s5 FROM test_emp GROUP BY 2;

[astefan]

@elasticmachine test this please

[astefan]

@emasab I see why those are failing and it's ok.

Please, move those three to agg.csv-spec where you should also add the expected output. Follow the other queries examples in that file to add those three. If you need help, just ask. Thanks.

[astefan]

@emasab also, don't forget to add some tests following @matriv 's comment, as well. Thanks.

[emasab]

rebased to current master

[matriv]

LGTM.

Thank you for the contribution!
Thanks a lot for your patience and for addressing our comments.

[emasab]

@matriv I was happy to contribute! There is always to learn from such important projects.

[matriv]

@elasticmachine test this please

[matriv]

@elasticmachin run elasticsearch-ci/bwc

[matriv]

@elasticmachine update branch

[matriv]

@elasticmachine test this please

[astefan]

LGTM