Skip to content

[CALCITE-7667] Improve string-literal encoding in pushdown translators#5117

Open
rubenada wants to merge 2 commits into
apache:mainfrom
rubenada:CALCITE-7667
Open

[CALCITE-7667] Improve string-literal encoding in pushdown translators#5117
rubenada wants to merge 2 commits into
apache:mainfrom
rubenada:CALCITE-7667

Conversation

@rubenada

Copy link
Copy Markdown
Contributor

Jira Link

CALCITE-7667

Changes Proposed

Improve string-literal encoding in several translator:

  • MySQL dialect: escape backslash (as it happens already on BigQuery)
  • Cassandra, Geode, Pig: escape single quote

@mihaibudiu mihaibudiu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These look fine, but I don't really know the conventions of all these engines so I cannot tell whether the results are correct or some other important cases of characters that need to be escaped are missing. I hope that at least these test results were validated to be correct using a real engine. There is at least one test which used to have an incorrect expected result.

+ "from \"foodmart\".\"reserve_employee\"";
final String expectedStarRocks = "SELECT REGEXP_REPLACE('$@*AABC$@*AADCAA$@*A',"
+ " '^(\\$\\@\\*A)*|(\\$\\@\\*A)*$', '')\n"
+ " '^(\\\\$\\\\@\\\\*A)*|(\\\\$\\\\@\\\\*A)*$', '')\n"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this test was wrong?
I hope that this one was validated somehow.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems so. The test was "correct" for the "TRIM BOTH into REGEXP_REPLACE" part, but it was lacking the backslash escape.

@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants