single quote not properly escaped

[mcannamela]

What

ParamEscaper's escape_string() gives incorrect behavior on Databricks SQL and in Databricks notebooks.

It replaces a single quote ' with '', but the correct way to escape ' is with a backslash, like \'.

You can verify in PySpark with:

assert spark.sql("select 'cat''s meow' as my_col").head(1)[0]['my_col'] == "cats meow"
assert spark.sql("select 'cat\\'s meow' as my_col").head(1)[0]['my_col'] == "cat's meow"

Note that because it starts as a Python literal, we need two backslashes \\ to get Python to first escape \\' to \' and then Spark escapes to '.

I don't know what the motivation for this implementation was, but the result seems to be concatenation instead of escaping the quote character.

Reproduction in databricks-sql-python

The following demonstrates the issue in version 1.2.2 2.0.5 of databricks-sql-python against a serverless SQL warehouse in Azure, v2022.30, plus an implementation without parameter substitution showing an escape treatment that does work :

from typing import List

from databricks import sql
import os

from databricks.sql import ServerOperationError

server_hostname= os.environ.get('DBT_DATABRICKS_HOST')
http_path=f'/sql/1.0/endpoints/{os.environ.get("DBT_DATABRICKS_ENDPOINT")}'
access_token=os.environ.get('DBT_DATABRICKS_TOKEN')
user_agent_entry = "dbt-databricks/1.2.2"
connection = sql.connect(
  server_hostname=server_hostname,
  http_path=http_path,
  access_token=access_token,
  _user_agent_entry=user_agent_entry
)


cursor = connection.cursor()


def get_result_using_parameter_bindings(p:List[str]):
  try:
    cursor.execute('select %s as my_col', p)
    result = list(cursor.fetchall())[0]['my_col']
  except ServerOperationError as exc:
    result = exc.message.strip()[:20] + '...'
  return result

def get_result_using_fstring(p:List[str]):
  try:
    escaped = p[0].replace('\\','\\\\' ).replace("'", "\\'")
    cursor.execute(f"select '{escaped}' as my_col")
    result = list(cursor.fetchall())[0]['my_col']
  except ServerOperationError as exc:
    result = exc.message.strip()[:20] + '...'
  return result


params = [
  ["cat's meow"],
  ["cat\'s meow"],
  ["cat\\'s meow"],
  ["cat''s meow"],
]



for p in params:
  # using dbt-databricks-sql's parameter substitution
  param_binding_result = get_result_using_parameter_bindings(p)

  # using manually built and escaped query
  f_string_result = get_result_using_fstring(p)



  print('\nparameter value:', p[0], 'parameter-binding result:', param_binding_result, 'round-trip ok?', p[0]==param_binding_result)
  print('parameter value:', p[0], 'f-string result:', f_string_result, 'round-trip ok?',
        p[0] == f_string_result
        )
  assert p[0] == f_string_result


cursor.close()
connection.close()

The output is:

bash_1  | parameter value: cat's meow parameter-binding result: cats meow round-trip ok? False
bash_1  | parameter value: cat's meow f-string result: cat's meow round-trip ok? True
bash_1  | 
bash_1  | parameter value: cat's meow parameter-binding result: cats meow round-trip ok? False
bash_1  | parameter value: cat's meow f-string result: cat's meow round-trip ok? True
bash_1  | 
bash_1  | parameter value: cat\'s meow parameter-binding result: [PARSE_SYNTAX_ERROR]... round-trip ok? False
bash_1  | parameter value: cat\'s meow f-string result: cat\'s meow round-trip ok? True
bash_1  | 
bash_1  | parameter value: cat''s meow parameter-binding result: cats meow round-trip ok? False
bash_1  | parameter value: cat''s meow f-string result: cat''s meow round-trip ok? True

Expected results

String parameters with single quotes and backslashes should be properly reproduced:
"cat's meow" would be escaped as "cat\\'s meow" and the resulting SQL would return cat's meow
"cat\\'s meow" would escape to "cat\\\\\\'s meow" and the SQL would return cat\'s meow

Suggested fix

I'm not sure how this is usually implemented, but in my example just doing param.replace('\\','\\\\' ).replace("'", "\\'") at least preserves single quotes and backslashes, which are probably the most common cases. It would also leave alone escaped unicode literals like \U0001F44D.

How I encountered it

I'm using dbt with Databricks and noticed on upgrading from dbt-databricks 1.0 to 1.2.2 that single quotes started disappearing from our "seeds" (csv files loaded as Delta tables). Code had changed in dbt-databricks to use the new parameter binding functionality in this library, whereas (I assume) before it must have been injecting the values as literals into the SQL.

[susodapop]

Thanks for opening a detailed issue report! I've opened #46 which introduces unit tests for the expected escaping behaviour. Will need to compare your inputs against the expected behaviour to see if I can reproduce.

Just for clarity, you wrote:

The following demonstrates the issue in version 1.2.2 of databricks-sql-python

Do you mean version 1.2.2 of the dbt package? I don't believe we've released a version 1.2.2 of databricks-sql-python.

[mcannamela]

Do you mean version 1.2.2 of the dbt package? I don't believe we've released a version 1.2.2 of databricks-sql-python.

@susodapop yes, my bad, it's version 2.0.5 of the sql connector. Just so there can be no mistakes:

root@4571a4538ab2:/dbt# pip freeze | grep databricks
databricks-sql-connector==2.0.5
dbt-databricks==1.2.2

[susodapop]

I agree there's a bug here. ParamEscaper should not try to escape a single quote ' with a second single quote ''. Per Chesterton's Fence, however, I wanted to suss out why the ParameterEscaper behaves the way it does. Here's what I found:

Background

The ANSI SQL spec says that a single quote ' within a string literal wrapped in single quotes ' may be escaped either by a back stroke \' or by an extra single quote ''. Databricks supports the first style of escape character but not the second. Instead of treating two single quotes '' as one, escaped single quote ', Databricks parses them as separate string literals and then concatenates them. In effect it appears to swallow both single-quotes '. Therefore we see the following behaviour:

Input query	ANSI SQL	Databricks	Same?
SELECT 'foo''bar'	foo'bar	foobar	🔴
SELECT 'cat''s meow'	cat's meow	cats meow	🔴
SELECT 'cat\'s meow'	cat's meow	cat's meow	🟢
SELECT "cat's meow"	cat's meow	cat's meow	🟢

The author of ParamEscaper() assumed that because Databricks advertises ANSI compliance the customary two single quote '' escape pattern would work. But it does not.

To behave properly, ParameterEscaper() should escape single quotes ' with a back stroke \'.

Impact

When Cursor.execute(statement:str, params:List) is called today, databricks-sql-connector renders the statement with the provided params then sends it to Databricks. Before rendering, ParamEscaper() is called to sanitise params and thereby avoid SQL injection. This is necessary only because Databricks does not yet support parameterised SQL. The ParamEscaper() will be removed once Databricks adds support for parameterised SQL.

Solution

ParamEscaper() should be modified to escape one single quote ' with a back stroke \'.

[mcannamela]

Ahhh, well played!

ANSI compliance is "opt in" I believe. Some months ago, I thought it would become "opt out", but that appears not to have been the case and I can no longer find evidence of this.

However, it still doesn't appear that ANSI mode gives the escaping behavior you found documented, even though it does change other behaviors (note in the following examples, be sure to run the set statements in the same session):

--convince yourself you know how to set ansi mode
set ansi_mode = false;
select 1/0; -- null

set ansi_mode = true;
select 1/0; -- throws

--now check escaping behavior, and be surprised and dismayed that it doesn't change
set ansi_mode = false;
select 'cat''s meow'; -- cats meow

set ansi_mode = true;
select 'cat''s meow'; -- cats meow

So having done that exercise I doubly agree with your conclusion

[courtneyholcomb]

Not sure if anyone was actively working on this, but I just put up a PR to fix it if it's helpful! #56

[susodapop]

Thanks @courtneyholcomb! We are working on a similar implementation internally but we hadn't incorporated an e2e test like you wrote. I'm going to keep your PR open so we can verify that our change will still pass your e2e test. If it doesn't, we can make sure that your use-case is still covered. I'll have more details within the next week!