ORDER BY ColumnNumber with Collations #11139
Conversation
| if (collation.child->expression_class == ExpressionClass::CONSTANT) { | ||
| auto &constant = collation.child->Cast<ConstantExpression>(); | ||
| auto index = (idx_t)constant.value.GetValue<int64_t>() - 1; | ||
| D_ASSERT(index < extra_list->size()); |
There was a problem hiding this comment.
This is user input, right?
So we should probably not use D_ASSERT and instead check if the value is within the valid range, otherwise throw an InvalidInputException
| auto &collation = expr->Cast<CollateExpression>(); | ||
| if (collation.child->expression_class == ExpressionClass::CONSTANT) { | ||
| auto &constant = collation.child->Cast<ConstantExpression>(); | ||
| auto index = (idx_t)constant.value.GetValue<int64_t>() - 1; |
There was a problem hiding this comment.
I believe this needs a NumericCast, and we should add a test with a negative value, and 0
| INSERT INTO collate_test VALUES ('ã'),('B'),('a'),('A'); | ||
|
|
||
| query I | ||
| SELECT s FROM collate_test ORDER BY 1 COLLATE NOCASE; |
There was a problem hiding this comment.
In addition to the tests @Tishj requested could we add some tests with more complex expressions as well? i.e.
SELECT CONCAT(s, s) FROM collate_test ORDER BY 1 COLLATE NOCASE;
SELECT (SELECT s) FROM collate_test ORDER BY 1 COLLATE NOCASE;
SELECT CONCAT(s, s) AS concat, concat FROM collate_test ORDER BY 2 COLLATE NOCASE;
SELECT CASE WHEN s[1]='a' THEN s ELSE NULL END FROM collate_test ORDER BY 1 COLLATE NOCASE;
SELECT collate_test.s FROM collate_test ORDER BY 1 COLLATE NOCASE;There was a problem hiding this comment.
Thank you @Tishj and @Mytherin for the suggestions.
I've applied the NumericCast and added more test cases.
The only one that failed references a sub-query in the select list:
SELECT (SELECT s) FROM collate_test ORDER BY 1 COLLATE NOCASE;
But this case doesn't work with alias as well:
SELECT (SELECT s) AS c1 FROM collate_test ORDER BY c1 COLLATE NOCASE;
Binder Error: Alias "c1" referenced in a SELECT clause - but the expression has a subquery. This is not yet supported.
| auto &sel_entry = extra_list->at(index); | ||
| if (!sel_entry->alias.empty()) { | ||
| vector<string> column_names = {sel_entry->alias}; | ||
| auto colref = make_uniq<ColumnRefExpression>(std::move(column_names)); |
There was a problem hiding this comment.
I don't quite follow what the goal of this code path is, e.g. it seems like this would not work:
SELECT 'a' AS my_alias ORDER BY 1 COLLATE NOCASE;Perhaps we can just remove this code path and always use copy?
| colref->query_location = sel_entry->query_location; | ||
| collation.child = std::move(colref); | ||
| } else { // no alias, e.g.: SELECT 'a' ORDER BY 1 COLLATE NOCASE | ||
| if (sel_entry->expression_class == ExpressionClass::SUBQUERY) { |
There was a problem hiding this comment.
This should use HasSubquery(), otherwise e.g. this will result in an error:
SELECT concat('a', (SELECT s)) FROM collate_test ORDER BY 1 COLLATE NOCASE;There was a problem hiding this comment.
Thanks @Mytherin, I've applied all suggestions and added extra test cases.
|
Thanks! LGTM |
Merge pull request duckdb/duckdb#11139 from tiagokepe/master
Merge pull request duckdb/duckdb#11139 from tiagokepe/master
Merge pull request duckdb/duckdb#11139 from tiagokepe/master
Merge pull request duckdb/duckdb#11139 from tiagokepe/master
Hello all,
This PR enables ORDER BY ColumnNumber with collations like
SELECT 'a' ORDER BY 1 COLLATE NOCASE.