1use crate::linter::rule::{LintContext, LintRule};
6use crate::types::{issue_codes, Dialect, Issue, IssueAutofixApplicability, IssuePatchEdit, Span};
7use sqlparser::ast::{
8 Expr, FunctionArg, FunctionArgExpr, FunctionArguments, OrderByKind, Query, Select, SetExpr,
9 Statement, TableFactor, WindowType,
10};
11use sqlparser::keywords::Keyword;
12use sqlparser::tokenizer::{Token, TokenWithSpan, Tokenizer, Whitespace};
13
14use super::semantic_helpers::join_on_expr;
15
16pub struct AmbiguousOrderBy;
17
18impl LintRule for AmbiguousOrderBy {
19 fn code(&self) -> &'static str {
20 issue_codes::LINT_AM_003
21 }
22
23 fn name(&self) -> &'static str {
24 "Ambiguous ORDER BY"
25 }
26
27 fn description(&self) -> &'static str {
28 "Ambiguous ordering directions for columns in order by clause."
29 }
30
31 fn check(&self, statement: &Statement, ctx: &LintContext) -> Vec<Issue> {
32 let mut violation_count = 0usize;
33 check_statement(statement, &mut violation_count);
34 let clause_autofixes = am003_clause_autofixes(ctx.statement_sql(), ctx.dialect());
35 let clauses_align = clause_autofixes.len() == violation_count;
36
37 (0..violation_count)
38 .map(|index| {
39 let mut issue = Issue::warning(
40 issue_codes::LINT_AM_003,
41 "Ambiguous ORDER BY clause. Specify ASC/DESC for all columns or none.",
42 )
43 .with_statement(ctx.statement_index);
44
45 if clauses_align {
46 let clause_fix = &clause_autofixes[index];
47 let span =
48 ctx.span_from_statement_offset(clause_fix.span.start, clause_fix.span.end);
49 let edits = clause_fix
50 .edits
51 .iter()
52 .map(|edit| {
53 IssuePatchEdit::new(
54 ctx.span_from_statement_offset(edit.span.start, edit.span.end),
55 edit.replacement.clone(),
56 )
57 })
58 .collect();
59 issue = issue
60 .with_span(span)
61 .with_autofix_edits(IssueAutofixApplicability::Safe, edits);
62 }
63
64 issue
65 })
66 .collect()
67 }
68}
69
70fn check_statement(statement: &Statement, violations: &mut usize) {
71 match statement {
72 Statement::Query(query) => check_query(query, violations),
73 Statement::Insert(insert) => {
74 if let Some(source) = &insert.source {
75 check_query(source, violations);
76 }
77 }
78 Statement::CreateView { query, .. } => check_query(query, violations),
79 Statement::CreateTable(create) => {
80 if let Some(query) = &create.query {
81 check_query(query, violations);
82 }
83 }
84 _ => {}
85 }
86}
87
88fn check_query(query: &Query, violations: &mut usize) {
89 if let Some(with) = &query.with {
90 for cte in &with.cte_tables {
91 check_query(&cte.query, violations);
92 }
93 }
94
95 check_set_expr(&query.body, violations);
96
97 if order_by_mixes_explicit_and_implicit_direction(query) {
98 *violations += 1;
99 }
100}
101
102fn check_set_expr(set_expr: &SetExpr, violations: &mut usize) {
103 match set_expr {
104 SetExpr::Select(select) => check_select(select, violations),
105 SetExpr::Query(query) => check_query(query, violations),
106 SetExpr::SetOperation { left, right, .. } => {
107 check_set_expr(left, violations);
108 check_set_expr(right, violations);
109 }
110 SetExpr::Insert(statement)
111 | SetExpr::Update(statement)
112 | SetExpr::Delete(statement)
113 | SetExpr::Merge(statement) => check_statement(statement, violations),
114 _ => {}
115 }
116}
117
118fn check_select(select: &Select, violations: &mut usize) {
119 for table in &select.from {
120 check_table_factor(&table.relation, violations);
121 for join in &table.joins {
122 check_table_factor(&join.relation, violations);
123 if let Some(on_expr) = join_on_expr(&join.join_operator) {
124 check_expr_for_subqueries(on_expr, violations);
125 }
126 }
127 }
128
129 for item in &select.projection {
130 if let sqlparser::ast::SelectItem::UnnamedExpr(expr)
131 | sqlparser::ast::SelectItem::ExprWithAlias { expr, .. } = item
132 {
133 check_expr_for_subqueries(expr, violations);
134 }
135 }
136
137 if let Some(prewhere) = &select.prewhere {
138 check_expr_for_subqueries(prewhere, violations);
139 }
140
141 if let Some(selection) = &select.selection {
142 check_expr_for_subqueries(selection, violations);
143 }
144
145 if let sqlparser::ast::GroupByExpr::Expressions(exprs, _) = &select.group_by {
146 for expr in exprs {
147 check_expr_for_subqueries(expr, violations);
148 }
149 }
150
151 if let Some(having) = &select.having {
152 check_expr_for_subqueries(having, violations);
153 }
154
155 if let Some(qualify) = &select.qualify {
156 check_expr_for_subqueries(qualify, violations);
157 }
158
159 for order_expr in &select.sort_by {
160 check_expr_for_subqueries(&order_expr.expr, violations);
161 }
162}
163
164fn check_table_factor(table_factor: &TableFactor, violations: &mut usize) {
165 match table_factor {
166 TableFactor::Derived { subquery, .. } => check_query(subquery, violations),
167 TableFactor::NestedJoin {
168 table_with_joins, ..
169 } => {
170 check_table_factor(&table_with_joins.relation, violations);
171 for join in &table_with_joins.joins {
172 check_table_factor(&join.relation, violations);
173 if let Some(on_expr) = join_on_expr(&join.join_operator) {
174 check_expr_for_subqueries(on_expr, violations);
175 }
176 }
177 }
178 TableFactor::Pivot { table, .. }
179 | TableFactor::Unpivot { table, .. }
180 | TableFactor::MatchRecognize { table, .. } => check_table_factor(table, violations),
181 _ => {}
182 }
183}
184
185fn check_expr_for_subqueries(expr: &Expr, violations: &mut usize) {
186 match expr {
187 Expr::Subquery(query)
188 | Expr::Exists {
189 subquery: query, ..
190 } => check_query(query, violations),
191 Expr::InSubquery {
192 expr: inner,
193 subquery,
194 ..
195 } => {
196 check_expr_for_subqueries(inner, violations);
197 check_query(subquery, violations);
198 }
199 Expr::BinaryOp { left, right, .. }
200 | Expr::AnyOp { left, right, .. }
201 | Expr::AllOp { left, right, .. } => {
202 check_expr_for_subqueries(left, violations);
203 check_expr_for_subqueries(right, violations);
204 }
205 Expr::UnaryOp { expr: inner, .. }
206 | Expr::Nested(inner)
207 | Expr::IsNull(inner)
208 | Expr::IsNotNull(inner)
209 | Expr::Cast { expr: inner, .. } => check_expr_for_subqueries(inner, violations),
210 Expr::InList { expr, list, .. } => {
211 check_expr_for_subqueries(expr, violations);
212 for item in list {
213 check_expr_for_subqueries(item, violations);
214 }
215 }
216 Expr::Between {
217 expr, low, high, ..
218 } => {
219 check_expr_for_subqueries(expr, violations);
220 check_expr_for_subqueries(low, violations);
221 check_expr_for_subqueries(high, violations);
222 }
223 Expr::Case {
224 operand,
225 conditions,
226 else_result,
227 ..
228 } => {
229 if let Some(operand) = operand {
230 check_expr_for_subqueries(operand, violations);
231 }
232 for when in conditions {
233 check_expr_for_subqueries(&when.condition, violations);
234 check_expr_for_subqueries(&when.result, violations);
235 }
236 if let Some(otherwise) = else_result {
237 check_expr_for_subqueries(otherwise, violations);
238 }
239 }
240 Expr::Function(function) => {
241 if let FunctionArguments::List(arguments) = &function.args {
242 for arg in &arguments.args {
243 match arg {
244 FunctionArg::Unnamed(FunctionArgExpr::Expr(expr))
245 | FunctionArg::Named {
246 arg: FunctionArgExpr::Expr(expr),
247 ..
248 } => check_expr_for_subqueries(expr, violations),
249 _ => {}
250 }
251 }
252 }
253
254 if let Some(filter) = &function.filter {
255 check_expr_for_subqueries(filter, violations);
256 }
257
258 for order_expr in &function.within_group {
259 check_expr_for_subqueries(&order_expr.expr, violations);
260 }
261
262 if let Some(WindowType::WindowSpec(spec)) = &function.over {
263 for expr in &spec.partition_by {
264 check_expr_for_subqueries(expr, violations);
265 }
266 for order_expr in &spec.order_by {
267 check_expr_for_subqueries(&order_expr.expr, violations);
268 }
269 }
270 }
271 _ => {}
272 }
273}
274
275fn order_by_mixes_explicit_and_implicit_direction(query: &Query) -> bool {
276 let Some(order_by) = &query.order_by else {
277 return false;
278 };
279
280 let OrderByKind::Expressions(order_exprs) = &order_by.kind else {
281 return false;
282 };
283
284 let mut has_explicit = false;
285 let mut has_implicit = false;
286
287 for order_expr in order_exprs {
288 if order_expr.options.asc.is_some() {
289 has_explicit = true;
290 } else {
291 has_implicit = true;
292 }
293 }
294
295 has_explicit && has_implicit
296}
297
298#[derive(Clone, Debug)]
299struct Am003ClauseAutofix {
300 span: Span,
301 edits: Vec<IssuePatchEdit>,
302}
303
304#[derive(Clone, Debug)]
305struct Am003OrderItem {
306 has_direction: bool,
307 insert_offset: usize,
308}
309
310fn am003_clause_autofixes(sql: &str, dialect: Dialect) -> Vec<Am003ClauseAutofix> {
311 let Some(tokens) = tokenized(sql, dialect) else {
312 return Vec::new();
313 };
314
315 let mut fixes = Vec::new();
316 let mut index = 0usize;
317 while index < tokens.len() {
318 let order_index = index;
319 if !is_order_keyword(&tokens[order_index].token) {
320 index += 1;
321 continue;
322 }
323
324 let Some(by_index) = next_non_trivia_index(&tokens, order_index + 1) else {
325 break;
326 };
327 if !is_by_keyword(&tokens[by_index].token) {
328 index += 1;
329 continue;
330 }
331
332 let Some(clause_start) = next_non_trivia_index(&tokens, by_index + 1) else {
333 break;
334 };
335 let (items, clause_end, next_index) = collect_order_by_items(sql, &tokens, clause_start);
336 index = next_index;
337
338 if items.len() < 2 {
339 continue;
340 }
341
342 let has_explicit = items.iter().any(|item| item.has_direction);
343 let has_implicit = items.iter().any(|item| !item.has_direction);
344 if !has_explicit || !has_implicit {
345 continue;
346 }
347
348 let mut edits = Vec::new();
349 for item in items {
350 if !item.has_direction {
351 edits.push(IssuePatchEdit::new(
352 Span::new(item.insert_offset, item.insert_offset),
353 " ASC",
354 ));
355 }
356 }
357 if edits.is_empty() {
358 continue;
359 }
360
361 let Some((clause_start_offset, _)) = token_with_span_offsets(sql, &tokens[order_index])
362 else {
363 continue;
364 };
365 fixes.push(Am003ClauseAutofix {
366 span: Span::new(clause_start_offset, clause_end),
367 edits,
368 });
369 }
370
371 fixes
372}
373
374fn collect_order_by_items(
375 sql: &str,
376 tokens: &[TokenWithSpan],
377 start_index: usize,
378) -> (Vec<Am003OrderItem>, usize, usize) {
379 let mut depth = 0usize;
380 let mut cursor = start_index;
381 let mut item_start = start_index;
382 let mut items = Vec::new();
383
384 while cursor < tokens.len() {
385 let token = &tokens[cursor].token;
386 if is_trivia(token) {
387 cursor += 1;
388 continue;
389 }
390
391 match token {
392 Token::LParen => {
393 depth += 1;
394 cursor += 1;
395 }
396 Token::RParen => {
397 if depth == 0 {
398 break;
399 }
400 depth -= 1;
401 cursor += 1;
402 }
403 Token::Comma if depth == 0 => {
404 if let Some(item) = analyze_order_item(sql, tokens, item_start, cursor) {
405 items.push(item);
406 }
407 cursor += 1;
408 item_start = cursor;
409 }
410 Token::SemiColon if depth == 0 => break,
411 Token::Word(word) if depth == 0 && order_by_clause_terminator(word.keyword) => break,
412 _ => cursor += 1,
413 }
414 }
415
416 if let Some(item) = analyze_order_item(sql, tokens, item_start, cursor) {
417 items.push(item);
418 }
419
420 let clause_end = clause_end_offset(sql, tokens, start_index, cursor);
421 (items, clause_end, cursor)
422}
423
424fn analyze_order_item(
425 sql: &str,
426 tokens: &[TokenWithSpan],
427 start_index: usize,
428 end_index: usize,
429) -> Option<Am003OrderItem> {
430 let mut depth = 0usize;
431 let mut has_direction = false;
432 let mut nulls_insert_offset = None;
433 let mut last_significant_end = None;
434
435 for token in tokens.iter().take(end_index).skip(start_index) {
436 let raw = &token.token;
437 if is_trivia(raw) {
438 continue;
439 }
440
441 match raw {
442 Token::LParen => {
443 depth += 1;
444 }
445 Token::RParen => {
446 depth = depth.saturating_sub(1);
447 }
448 Token::Word(word) if depth == 0 => {
449 if word.keyword == Keyword::ASC || word.keyword == Keyword::DESC {
450 has_direction = true;
451 } else if word.keyword == Keyword::NULLS && nulls_insert_offset.is_none() {
452 nulls_insert_offset = last_significant_end;
453 }
454 }
455 _ => {}
456 }
457
458 last_significant_end = token_with_span_offsets(sql, token).map(|(_, end)| end);
459 }
460
461 let fallback_insert = last_significant_end?;
462 Some(Am003OrderItem {
463 has_direction,
464 insert_offset: nulls_insert_offset.unwrap_or(fallback_insert),
465 })
466}
467
468fn clause_end_offset(
469 sql: &str,
470 tokens: &[TokenWithSpan],
471 start_index: usize,
472 end_index: usize,
473) -> usize {
474 for token in tokens.iter().take(end_index).skip(start_index).rev() {
475 if is_trivia(&token.token) {
476 continue;
477 }
478 if let Some((_, end)) = token_with_span_offsets(sql, token) {
479 return end;
480 }
481 }
482 sql.len()
483}
484
485fn tokenized(sql: &str, dialect: Dialect) -> Option<Vec<TokenWithSpan>> {
486 let dialect = dialect.to_sqlparser_dialect();
487 let mut tokenizer = Tokenizer::new(dialect.as_ref(), sql);
488 tokenizer.tokenize_with_location().ok()
489}
490
491fn is_order_keyword(token: &Token) -> bool {
492 matches!(token, Token::Word(word) if word.keyword == Keyword::ORDER)
493}
494
495fn is_by_keyword(token: &Token) -> bool {
496 matches!(token, Token::Word(word) if word.keyword == Keyword::BY)
497}
498
499fn order_by_clause_terminator(keyword: Keyword) -> bool {
500 matches!(
501 keyword,
502 Keyword::LIMIT
503 | Keyword::OFFSET
504 | Keyword::FETCH
505 | Keyword::UNION
506 | Keyword::EXCEPT
507 | Keyword::INTERSECT
508 | Keyword::WINDOW
509 | Keyword::INTO
510 | Keyword::FOR
511 )
512}
513
514fn next_non_trivia_index(tokens: &[TokenWithSpan], mut index: usize) -> Option<usize> {
515 while index < tokens.len() {
516 if !is_trivia(&tokens[index].token) {
517 return Some(index);
518 }
519 index += 1;
520 }
521 None
522}
523
524fn token_with_span_offsets(sql: &str, token: &TokenWithSpan) -> Option<(usize, usize)> {
525 let start = line_col_to_offset(
526 sql,
527 token.span.start.line as usize,
528 token.span.start.column as usize,
529 )?;
530 let end = line_col_to_offset(
531 sql,
532 token.span.end.line as usize,
533 token.span.end.column as usize,
534 )?;
535 Some((start, end))
536}
537
538fn line_col_to_offset(sql: &str, line: usize, column: usize) -> Option<usize> {
539 if line == 0 || column == 0 {
540 return None;
541 }
542
543 let mut current_line = 1usize;
544 let mut current_col = 1usize;
545 for (offset, ch) in sql.char_indices() {
546 if current_line == line && current_col == column {
547 return Some(offset);
548 }
549 if ch == '\n' {
550 current_line += 1;
551 current_col = 1;
552 } else {
553 current_col += 1;
554 }
555 }
556
557 if current_line == line && current_col == column {
558 return Some(sql.len());
559 }
560 None
561}
562
563fn is_trivia(token: &Token) -> bool {
564 matches!(
565 token,
566 Token::Whitespace(
567 Whitespace::Space
568 | Whitespace::Newline
569 | Whitespace::Tab
570 | Whitespace::SingleLineComment { .. }
571 | Whitespace::MultiLineComment(_)
572 )
573 )
574}
575
576#[cfg(test)]
577mod tests {
578 use super::*;
579 use crate::parser::parse_sql;
580 use crate::types::IssueAutofixApplicability;
581
582 fn run(sql: &str) -> Vec<Issue> {
583 let statements = parse_sql(sql).expect("parse");
584 let rule = AmbiguousOrderBy;
585 statements
586 .iter()
587 .enumerate()
588 .flat_map(|(index, statement)| {
589 rule.check(
590 statement,
591 &LintContext {
592 sql,
593 statement_range: 0..sql.len(),
594 statement_index: index,
595 },
596 )
597 })
598 .collect()
599 }
600
601 fn apply_issue_autofix(sql: &str, issue: &Issue) -> Option<String> {
602 let autofix = issue.autofix.as_ref()?;
603 let mut edits = autofix.edits.clone();
604 edits.sort_by(|left, right| right.span.start.cmp(&left.span.start));
605
606 let mut out = sql.to_string();
607 for edit in edits {
608 out.replace_range(edit.span.start..edit.span.end, &edit.replacement);
609 }
610 Some(out)
611 }
612
613 #[test]
616 fn allows_unspecified_single_order_item() {
617 let issues = run("SELECT * FROM t ORDER BY a");
618 assert!(issues.is_empty());
619 }
620
621 #[test]
622 fn allows_unspecified_all_order_items() {
623 let issues = run("SELECT * FROM t ORDER BY a, b");
624 assert!(issues.is_empty());
625 }
626
627 #[test]
628 fn allows_all_explicit_order_items() {
629 let issues = run("SELECT * FROM t ORDER BY a ASC, b DESC");
630 assert!(issues.is_empty());
631
632 let issues = run("SELECT * FROM t ORDER BY a DESC, b ASC");
633 assert!(issues.is_empty());
634 }
635
636 #[test]
637 fn flags_mixed_implicit_and_explicit_order_items() {
638 let issues = run("SELECT * FROM t ORDER BY a, b DESC");
639 assert_eq!(issues.len(), 1);
640 assert_eq!(issues[0].code, issue_codes::LINT_AM_003);
641
642 let issues = run("SELECT * FROM t ORDER BY a DESC, b");
643 assert_eq!(issues.len(), 1);
644 }
645
646 #[test]
647 fn flags_nulls_clause_without_explicit_direction_when_mixed() {
648 let issues = run("SELECT * FROM t ORDER BY a DESC, b NULLS LAST");
649 assert_eq!(issues.len(), 1);
650 }
651
652 #[test]
653 fn allows_consistent_order_by_with_comments() {
654 let issues = run("SELECT * FROM t ORDER BY a /* Comment */ DESC, b ASC");
655 assert!(issues.is_empty());
656 }
657
658 #[test]
659 fn mixed_order_by_emits_safe_autofix_patch() {
660 let sql = "SELECT * FROM t ORDER BY a DESC, b";
661 let issues = run(sql);
662 assert_eq!(issues.len(), 1);
663
664 let autofix = issues[0].autofix.as_ref().expect("autofix metadata");
665 assert_eq!(autofix.applicability, IssueAutofixApplicability::Safe);
666 let fixed = apply_issue_autofix(sql, &issues[0]).expect("apply autofix");
667 assert_eq!(fixed, "SELECT * FROM t ORDER BY a DESC, b ASC");
668 }
669
670 #[test]
671 fn mixed_order_by_with_nulls_clause_inserts_asc_before_nulls() {
672 let sql = "SELECT * FROM t ORDER BY a DESC, b NULLS LAST";
673 let issues = run(sql);
674 assert_eq!(issues.len(), 1);
675
676 let fixed = apply_issue_autofix(sql, &issues[0]).expect("apply autofix");
677 assert_eq!(fixed, "SELECT * FROM t ORDER BY a DESC, b ASC NULLS LAST");
678 }
679}