-
Notifications
You must be signed in to change notification settings - Fork 146
Fix escaping during check execution #1486
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 5 commits
95ffb4b
0b987ab
b5ac9d2
9376f75
d88f8de
eda7515
c96ae40
0ffe3ac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -78,6 +78,9 @@ def _validate_spark_column(value: Any) -> Any: | |
| COLUMN_NORMALIZE_EXPRESSION = re.compile("[^a-zA-Z0-9]+") | ||
| COLUMN_PATTERN = re.compile(r"Column<'(.*?)(?: AS (\w+))?'>$", re.DOTALL) | ||
| INVALID_COLUMN_NAME_PATTERN = re.compile(r"[\s,;{}\(\)\n\t=]+") | ||
| VALID_UNQUOTED_IDENTIFIER_PATTERN = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") | ||
| SQL_EXPRESSION_STRUCTURE_PATTERN = re.compile(r"""[()\[\]{},;'"`*]""") | ||
| SQL_EXPRESSION_OPERATOR_PATTERN = re.compile(r"[-+/%<>=!&|~^]\s|\s[-+/%<>=!&|~^]") | ||
| _UNRESOLVED_PLACEHOLDER_PATTERN = re.compile(r"\{\{[^}]*\}\}") | ||
|
|
||
| # Destructive SQL statement keywords rejected by `is_sql_query_safe`. SELECT is intentionally | ||
|
|
@@ -308,6 +311,62 @@ def quote_column_name(name: str) -> str: | |
| return f"`{escaped}`" | ||
|
|
||
|
|
||
| def unquote_column_name(name: str) -> str: | ||
| """ | ||
| Removes surrounding back-quotes from a column name, reversing :func:`quote_column_name`. | ||
|
|
||
| A column reference that the user already back-quoted (e.g. "`Customer Name`") is unwrapped to its | ||
| plain form ("Customer Name") for use in display names and messages. Strings that are not a single | ||
| back-quoted identifier (plain names, dotted paths, SQL expressions) are returned unchanged. | ||
|
|
||
| Args: | ||
| name: Column reference provided as a string. | ||
|
|
||
| Returns: | ||
| The column name without surrounding back-quotes. | ||
| """ | ||
| if len(name) >= 2 and name.startswith("`") and name.endswith("`"): | ||
| return name[1:-1].replace("``", "`") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Finding: dotted path of separately back-quoted segments is mangled (cosmetic)
Failure scenario: |
||
| return name | ||
|
|
||
|
|
||
| def normalize_column_expr(column: str) -> str: | ||
| """ | ||
| Prepares a column reference string for use with ``F.expr``, back-quoting names that require SQL | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: docstring uses double-backtick object references, against the repo convention AGENTS.md (Docstrings) says: No backticks around object names — use italics instead (e.g., arg1, column). Backticks cause rendering issues in API docs. This docstring uses |
||
| identifier escaping while leaving SQL expressions untouched. | ||
|
|
||
| Check functions accept a column as either a plain name or a SQL expression string. A plain name that | ||
| is not a valid bare identifier (spaces, dashes, non-ASCII letters, etc., e.g. "Customer Name", | ||
| "gross-margin" or "Päivämäärä") does not parse when passed to ``F.expr`` and must be back-quoted. A | ||
| SQL expression (e.g. "a + b", "substr(x, 1, 2)", "*") must be passed through as-is. | ||
|
|
||
| Name and expression cannot be told apart with certainty from a string alone. This function uses a | ||
| conservative heuristic. A string is treated as an expression, and returned unchanged, when it | ||
| contains characters that typically appear in expressions (parentheses, brackets, quotes, comma, star) | ||
| or arithmetic/comparison operators that are whitespace-separated (e.g. "a + b"). Everything else is | ||
| treated as a dotted column path: each segment is left alone if it is a valid bare identifier. Otherwise | ||
| it is back-quoted (e.g. "struct_col.field1" is unchanged, "Customer Name" becomes "`Customer Name`") | ||
|
|
||
| This heuristic intentionally excludes two ambiguous cases: names that contain expression characters | ||
| (e.g. "amount (usd)"), and operator-free SQL expressions such as "col IS NOT NULL" (which is treated | ||
| as a name). Callers with such columns should back-quote the name themselves, pass a Column expression, | ||
| or use the ``sql_expression`` check. | ||
|
|
||
| Args: | ||
| column: Column reference provided as a string (plain name, nested path, or SQL expression). | ||
|
|
||
|
mwojtyczka marked this conversation as resolved.
|
||
| Returns: | ||
| A string safe to pass to ``F.expr``. | ||
| """ | ||
| if SQL_EXPRESSION_STRUCTURE_PATTERN.search(column) or SQL_EXPRESSION_OPERATOR_PATTERN.search(column): | ||
| return column | ||
|
|
||
| return ".".join( | ||
| segment if VALID_UNQUOTED_IDENTIFIER_PATTERN.match(segment) else quote_column_name(segment) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Finding: a column whose name equals a zero-arg SQL function resolves to the function, not the column (silent wrong result) A bare identifier that matches Failure scenario: a DataFrame has a column named
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is also DBSQL's convention. Users must escape zero-arg identifiers in any DBSQL query if they use them as column names. Should we change this or align with DBSQL?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sounds good, I think it's better to stay aligned with DBSQL |
||
| for segment in column.split(".") | ||
| ) | ||
|
|
||
|
|
||
| def normalize_col_str(col_str: str) -> str: | ||
| """ | ||
| Normalizes string to be compatible with metastore column names by applying the following transformations: | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this logic be implemented inside
get_column_name_or_alias?We already handle the case there:
we just need to extend it with an option to normalize:
Then we don't need to change check_funcs.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still not resolved. The current implementation branches on
isinstance(column, str)insideget_normalized_column_and_exprand callsunquote_column_name+normalize_col_strdirectly, rather than adding anormalizeoption toget_column_name_or_aliasas suggested — socheck_funcs.pywas still changed. Leaving open to confirm whether we want to centralize this inget_column_name_or_aliasinstead.