Skip to content

Commit 0777ba7

Browse files
committed
fix(test): ignore comments around set-operation branches in SQL-test fixtures
A comment before or after UNION ALL in an __expected__ fixture stayed attached to the neighbouring branch, so the projection scanners read the comment as an alias and reported a false P001 branch mismatch. Branch and projection splits now drop surrounding comments, alias detection ignores trailing comments, the Python fallback splitter follows the adapter's lexical syntax, and Polyglot's annotated comment wrapper is unwrapped.
1 parent 5aa05f5 commit 0777ba7

13 files changed

Lines changed: 413 additions & 52 deletions

File tree

‎crates/sqlbuild-rules-native/src/compiler/_helpers/sql_tests/extraction.rs‎

Lines changed: 33 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -512,7 +512,7 @@ fn projection_name(expression: &str, file: &str, label: &str) -> Result<String,
512512
if let Some(position) = find_last_top_level_keyword(expression, "AS")? {
513513
let alias_start = skip_ignorable(expression, position + 2)?;
514514
if let Some((alias, end)) = read_identifier(expression, alias_start)
515-
&& expression[end..].trim().is_empty()
515+
&& skip_ignorable(expression, end)? == expression.len()
516516
{
517517
return Ok(alias);
518518
}
@@ -880,8 +880,34 @@ fn scan_code<T>(
880880
Ok(None)
881881
}
882882

883-
fn non_empty_trimmed(value: &str) -> Option<&str> {
884-
Some(value.trim()).filter(|trimmed| !trimmed.is_empty())
883+
/// Return `value` without surrounding whitespace and comments, or `None` when no code remains.
884+
fn non_empty_trimmed(value: &str) -> Result<Option<&str>, String> {
885+
if !value.contains("--") && !value.contains("/*") {
886+
return Ok(Some(value.trim()).filter(|trimmed| !trimmed.is_empty()));
887+
}
888+
let start = skip_ignorable(value, 0)?;
889+
let mut end = start;
890+
let mut index = start;
891+
while index < value.len() {
892+
if let Some(comment) = comment_end(value.as_bytes(), index)
893+
.map_err(|error| scan_error_message(error, "SQL test"))?
894+
{
895+
index = comment;
896+
continue;
897+
}
898+
let next = skip_non_code(value, index)?;
899+
if next != index {
900+
end = next;
901+
index = next;
902+
continue;
903+
}
904+
let width = char_len(value, index);
905+
if !value[index..].starts_with(char::is_whitespace) {
906+
end = index + width;
907+
}
908+
index += width;
909+
}
910+
Ok(Some(&value[start..end]).filter(|trimmed| !trimmed.is_empty()))
885911
}
886912

887913
/// Split on top-level `UNION`, `INTERSECT` and `EXCEPT`, each with an optional `ALL`/`DISTINCT`.
@@ -895,7 +921,7 @@ pub(crate) fn split_set_operations(sql: &str) -> Result<Vec<&str>, String> {
895921
let Some(end) = set_operator_end(sql, index)? else {
896922
return Ok(CodeStep::Advance);
897923
};
898-
values.extend(non_empty_trimmed(&sql[start..index]));
924+
values.extend(non_empty_trimmed(&sql[start..index])?);
899925
let mut resume = skip_ignorable(sql, end)?;
900926
if let Some(quantifier_end) =
901927
consume_keyword(sql, resume, "ALL").or_else(|| consume_keyword(sql, resume, "DISTINCT"))
@@ -905,7 +931,7 @@ pub(crate) fn split_set_operations(sql: &str) -> Result<Vec<&str>, String> {
905931
start = resume;
906932
Ok(CodeStep::Resume(resume))
907933
})?;
908-
values.extend(non_empty_trimmed(&sql[start..]));
934+
values.extend(non_empty_trimmed(&sql[start..])?);
909935
Ok(values)
910936
}
911937

@@ -927,12 +953,12 @@ pub(crate) fn split_top_level(sql: &str, separator: u8) -> Result<Vec<&str>, Str
927953
let mut start = 0;
928954
scan_code::<()>(sql, 0, |index, depth| {
929955
if depth == 0 && byte_at(sql, index) == Some(separator) {
930-
values.extend(non_empty_trimmed(&sql[start..index]));
956+
values.extend(non_empty_trimmed(&sql[start..index])?);
931957
start = index + 1;
932958
}
933959
Ok(CodeStep::Advance)
934960
})?;
935-
values.extend(non_empty_trimmed(&sql[start..]));
961+
values.extend(non_empty_trimmed(&sql[start..])?);
936962
Ok(values)
937963
}
938964

‎crates/sqlbuild-rules-native/src/compiler/tests/test_sql_test_extraction.rs‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,12 +91,26 @@ fn given_expected_cte_sql_when_scanning_top_level_then_set_operations_commas_and
9191
]),
9292
expected_commas: Ok(vec![
9393
"SELECT 'x, UNION (' AS a",
94-
"/* UNION , FROM */ \"b)\"",
94+
"\"b)\"",
9595
"`c,d`",
9696
"f(1, (SELECT 2 FROM t UNION SELECT 3)) -- , FROM\nFROM t",
9797
]),
9898
expected_from: Ok(Some(107)),
9999
},
100+
TopLevelScanTestCase {
101+
description: "comments around branches and projections are not code",
102+
sql: "-- lead\nSELECT 1 AS a, /* b */ 2 AS b -- first\n/* x */ UNION ALL -- second\nSELECT '--' AS a, 3 AS b /* end */",
103+
expected_unions: Ok(vec![
104+
"SELECT 1 AS a, /* b */ 2 AS b",
105+
"SELECT '--' AS a, 3 AS b",
106+
]),
107+
expected_commas: Ok(vec![
108+
"SELECT 1 AS a",
109+
"2 AS b -- first\n/* x */ UNION ALL -- second\nSELECT '--' AS a",
110+
"3 AS b",
111+
]),
112+
expected_from: Ok(None),
113+
},
100114
TopLevelScanTestCase {
101115
description: "keyword boundaries and empty pieces",
102116
sql: ",SELECT reunion, union_id,, x FROMAGE UNION",

‎src/sqlbuild/compiler/compile/_helpers/analysis/tests.py‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,14 +15,19 @@
1515
split_set_operation_branches,
1616
)
1717
from sqlbuild.compiler.sql_analysis.main.import_polyglot_sql import import_polyglot_sql
18+
from sqlbuild.compiler.sql_analysis.models import SqlLexicalSyntax
1819

1920
_POLYGLOT_VALUES_EXPRESSION_NAME: str = "Values"
2021
_POLYGLOT_VALUES_SET_ALIAS: str = "_values"
2122
_SCAN_CONTEXT: str = "SQL test"
2223

2324

2425
def extract_expected_branch_column_names_with_sql_analysis(
25-
*, sql: str, file_label: str, label: str = "__expected__<model>"
26+
*,
27+
sql: str,
28+
file_label: str,
29+
syntax: SqlLexicalSyntax,
30+
label: str = "__expected__<model>",
2631
) -> tuple[tuple[str, ...], ...] | None:
2732
"""Return expected SELECT branch names using required Polyglot analysis."""
2833

@@ -32,17 +37,23 @@ def extract_expected_branch_column_names_with_sql_analysis(
3237
parsed_expression: Any = polyglot_module.parse_one(sql, dialect="generic")
3338
except polyglot_module.PolyglotError:
3439
return _extract_unparsed_branch_names(
35-
sql=sql, polyglot_module=polyglot_module, file_label=file_label, label=label
40+
sql=sql,
41+
polyglot_module=polyglot_module,
42+
file_label=file_label,
43+
label=label,
44+
syntax=syntax,
3645
)
3746
return _extract_branch_names(expression=parsed_expression, file_label=file_label, label=label)
3847

3948

4049
def _extract_unparsed_branch_names(
41-
*, sql: str, polyglot_module: Any, file_label: str, label: str
50+
*, sql: str, polyglot_module: Any, file_label: str, label: str, syntax: SqlLexicalSyntax
4251
) -> tuple[tuple[str, ...], ...] | None:
4352
"""Parse each top-level set-operation branch separately when the whole query cannot parse."""
4453

45-
branches: tuple[str, ...] = split_set_operation_branches(sql=sql, context=_SCAN_CONTEXT)
54+
branches: tuple[str, ...] = split_set_operation_branches(
55+
sql=sql, context=_SCAN_CONTEXT, syntax=syntax
56+
)
4657
if len(branches) <= 1:
4758
return None
4859
try:

‎src/sqlbuild/compiler/compile/_helpers/sql_tests/core.py‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -762,12 +762,14 @@ def _extract_expected_branch_column_names(
762762
) -> tuple[tuple[str, ...], ...]:
763763
sql_analysis_column_names: tuple[tuple[str, ...], ...] | None = (
764764
extract_expected_branch_column_names_with_sql_analysis(
765-
sql=sql, file_label=file_label, label=label
765+
sql=sql, file_label=file_label, label=label, syntax=syntax
766766
)
767767
)
768768
if sql_analysis_column_names is not None:
769769
return sql_analysis_column_names
770-
branches: tuple[str, ...] = split_set_operation_branches(sql=sql, context=_CONTEXT)
770+
branches: tuple[str, ...] = split_set_operation_branches(
771+
sql=sql, context=_CONTEXT, syntax=syntax
772+
)
771773
return tuple(
772774
_extract_expected_select_column_names(
773775
branch_sql=branch, file_label=file_label, label=label, syntax=syntax
@@ -837,9 +839,13 @@ def _extract_expected_projection_name(
837839
alias_name: str | None = _extract_as_alias(expression=expression, syntax=syntax)
838840
if alias_name is not None:
839841
return alias_name
840-
stripped_expression: str = expression.strip()
841-
if _is_simple_identifier(stripped_expression):
842-
return stripped_expression
842+
start: int = _skip_ignorable(sql=expression, start=0, syntax=syntax)
843+
if start < len(expression) and is_identifier_start(expression[start]):
844+
name: str
845+
end: int
846+
name, end = _read_identifier(sql=expression, start=start, file_label="projection")
847+
if _skip_ignorable(sql=expression, start=end, syntax=syntax) == len(expression):
848+
return name
843849
raise CompileInputError(
844850
f"SQL test '{file_label}' must alias every non-trivial {label} projection"
845851
)
@@ -862,17 +868,11 @@ def _extract_as_alias(*, expression: str, syntax: SqlLexicalSyntax) -> str | Non
862868
start=alias_index,
863869
file_label="projection",
864870
)
865-
if not expression[alias_end:].strip():
871+
if _skip_ignorable(sql=expression, start=alias_end, syntax=syntax) == len(expression):
866872
last_alias_name = alias_name
867873
return last_alias_name
868874

869875

870-
def _is_simple_identifier(value: str) -> bool:
871-
if not value or not is_identifier_start(value[0]):
872-
return False
873-
return all(is_identifier_character(character) for character in value[1:])
874-
875-
876876
def _contains_select_star(*, sql: str, syntax: SqlLexicalSyntax) -> bool:
877877
index: int
878878
for index, _depth in iter_code_positions(sql=sql, context=_CONTEXT, syntax=syntax):

‎src/sqlbuild/compiler/compile/constants.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@
9696
)
9797
POLYGLOT_SELECT_EXPRESSION_NAME: str = "Select"
9898
POLYGLOT_COLUMN_EXPRESSION_NAME: str = "Column"
99-
POLYGLOT_WRAPPER_EXPRESSION_NAMES: frozenset[str] = frozenset({"Subquery", "Paren"})
99+
POLYGLOT_WRAPPER_EXPRESSION_NAMES: frozenset[str] = frozenset({"Annotated", "Subquery", "Paren"})
100100

101101
TABLE_FUNCTION_RETURN_KEYS: frozenset[str] = frozenset({"table"})
102102

‎src/sqlbuild/compiler/sql_analysis/_helpers/scanning.py‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -231,6 +231,50 @@ def iter_code_positions_impl(
231231
index += 1
232232

233233

234+
def skip_ignorable_impl(
235+
*, sql: str, start: int, context: str = "SQL", syntax: SqlLexicalSyntax | None = None
236+
) -> int:
237+
"""Return the first offset at or after `start` that is not whitespace or a comment."""
238+
239+
index: int = start
240+
while index < len(sql):
241+
if sql[index].isspace():
242+
index += 1
243+
continue
244+
if sql[index] in SQL_TEXT_START_CHARACTERS:
245+
return index
246+
comment_end: int | None = _non_code_end(
247+
sql=sql, index=index, context=context, syntax=syntax
248+
)
249+
if comment_end is None:
250+
return index
251+
index = comment_end
252+
return index
253+
254+
255+
def strip_ignorable_impl(
256+
*, sql: str, context: str = "SQL", syntax: SqlLexicalSyntax | None = None
257+
) -> str:
258+
"""Return the SQL without leading or trailing whitespace and comments."""
259+
260+
code_start: int = skip_ignorable_impl(sql=sql, start=0, context=context, syntax=syntax)
261+
code_end: int = code_start
262+
index: int = code_start
263+
while index < len(sql):
264+
non_code_end: int | None = _non_code_end(
265+
sql=sql, index=index, context=context, syntax=syntax
266+
)
267+
if non_code_end is None:
268+
index += 1
269+
if not sql[index - 1].isspace():
270+
code_end = index
271+
continue
272+
if sql[index] in SQL_TEXT_START_CHARACTERS:
273+
code_end = non_code_end
274+
index = non_code_end
275+
return sql[code_start:code_end]
276+
277+
234278
def _non_code_end(
235279
*, sql: str, index: int, context: str, syntax: SqlLexicalSyntax | None
236280
) -> int | None:

‎src/sqlbuild/compiler/sql_analysis/_helpers/set_operations.py‎

Lines changed: 18 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,10 @@
55
from sqlbuild.compiler.sql_analysis._helpers.scanning import (
66
is_identifier_character_impl,
77
iter_code_positions_impl,
8-
skip_block_comment_impl,
9-
skip_line_comment_impl,
8+
skip_ignorable_impl,
9+
strip_ignorable_impl,
1010
)
11+
from sqlbuild.compiler.sql_analysis.models import SqlLexicalSyntax
1112

1213
_SET_OPERATOR_KEYWORDS: tuple[str, ...] = ("UNION", "INTERSECT")
1314
_EXCEPT_KEYWORD: str = "EXCEPT"
@@ -16,8 +17,10 @@
1617
_STAR_CHARACTER: str = "*"
1718

1819

19-
def split_set_operation_branches_impl(*, sql: str, context: str) -> tuple[str, ...]:
20-
"""Split SQL on top-level `UNION`, `INTERSECT` and `EXCEPT` with optional quantifiers."""
20+
def split_set_operation_branches_impl(
21+
*, sql: str, context: str, syntax: SqlLexicalSyntax | None = None
22+
) -> tuple[str, ...]:
23+
"""Split SQL on top-level set operators; branches exclude surrounding comments."""
2124

2225
branches: list[str] = []
2326
branch_start: int = 0
@@ -26,7 +29,7 @@ def split_set_operation_branches_impl(*, sql: str, context: str) -> tuple[str, .
2629
previous_depth: int = 0
2730
index: int
2831
depth: int
29-
for index, depth in iter_code_positions_impl(sql=sql, context=context):
32+
for index, depth in iter_code_positions_impl(sql=sql, context=context, syntax=syntax):
3033
if depth != previous_depth:
3134
previous_code = None
3235
previous_depth = depth
@@ -39,17 +42,23 @@ def split_set_operation_branches_impl(*, sql: str, context: str) -> tuple[str, .
3942
previous_code = sql[index]
4043
if operator_end is None:
4144
continue
42-
branch_sql: str = sql[branch_start:index].strip()
45+
branch_sql: str = strip_ignorable_impl(
46+
sql=sql[branch_start:index], context=context, syntax=syntax
47+
)
4348
if branch_sql:
4449
branches.append(branch_sql)
45-
resume = _skip_ignorable(sql=sql, start=operator_end, context=context)
50+
resume = skip_ignorable_impl(sql=sql, start=operator_end, context=context, syntax=syntax)
4651
quantifier_end: int | None = _keyword_end(
4752
sql=sql, start=resume, keyword=_ALL_KEYWORD
4853
) or _keyword_end(sql=sql, start=resume, keyword=_DISTINCT_KEYWORD)
4954
if quantifier_end is not None:
50-
resume = _skip_ignorable(sql=sql, start=quantifier_end, context=context)
55+
resume = skip_ignorable_impl(
56+
sql=sql, start=quantifier_end, context=context, syntax=syntax
57+
)
5158
branch_start = resume
52-
final_branch_sql: str = sql[branch_start:].strip()
59+
final_branch_sql: str = strip_ignorable_impl(
60+
sql=sql[branch_start:], context=context, syntax=syntax
61+
)
5362
if final_branch_sql:
5463
branches.append(final_branch_sql)
5564
return tuple(branches)
@@ -77,19 +86,3 @@ def _keyword_end(*, sql: str, start: int, keyword: str) -> int | None:
7786
if start > 0 and is_identifier_character_impl(sql[start - 1]):
7887
return None
7988
return keyword_end
80-
81-
82-
def _skip_ignorable(*, sql: str, start: int, context: str) -> int:
83-
index: int = start
84-
while index < len(sql):
85-
if sql[index].isspace():
86-
index += 1
87-
continue
88-
if sql.startswith("--", index):
89-
index = skip_line_comment_impl(sql=sql, start=index)
90-
continue
91-
if sql.startswith("/*", index):
92-
index = skip_block_comment_impl(sql=sql, start=index, context=context)
93-
continue
94-
return index
95-
return index

‎src/sqlbuild/compiler/sql_analysis/main/_split_set_operation_branches.py‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,12 @@
33
from sqlbuild.compiler.sql_analysis._helpers.set_operations import (
44
split_set_operation_branches_impl,
55
)
6+
from sqlbuild.compiler.sql_analysis.models import SqlLexicalSyntax
67

78

8-
def split_set_operation_branches(*, sql: str, context: str = "SQL") -> tuple[str, ...]:
9+
def split_set_operation_branches(
10+
*, sql: str, context: str = "SQL", syntax: SqlLexicalSyntax | None = None
11+
) -> tuple[str, ...]:
912
"""Split SQL on top-level `UNION`, `INTERSECT` and `EXCEPT`, ignoring quotes and comments."""
1013

11-
return split_set_operation_branches_impl(sql=sql, context=context)
14+
return split_set_operation_branches_impl(sql=sql, context=context, syntax=syntax)

‎tests/e2e/src/sqlbuild/cli/commands/main/test/_test_types.py‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -212,3 +212,13 @@ class AuthoredSqlParityTestCase:
212212
description: str
213213
compiled_test_name: str
214214
expected_fragments: tuple[str, ...]
215+
216+
217+
@dataclass(frozen=True)
218+
class CommentedFixtureE2ETestCase:
219+
"""SQL tests whose fixture CTEs carry comments around set-operation branches."""
220+
221+
description: str
222+
test_files: dict[str, str]
223+
expected_exit_code: int
224+
expected_output_fragments: tuple[str, ...]

0 commit comments

Comments
 (0)