Skip to content
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

SQL: Allow whitespaces in escape patterns #47577

Merged
merged 2 commits into from
Oct 7, 2019
Merged

Conversation

matriv
Copy link
Contributor

@matriv matriv commented Oct 4, 2019

Previously, we supported only the format {fn <FUNCTION_NAME>()}
but other DBs like MSSQL, DB2, MariaDB/MySQL alos allow whitespaces
between { and fn. Furhermore, also some applications - like PowerBI -
generate escape sequences with spaces: select { fn name(params) } etc.

Add support for white spaces between { and the escape pattern definition
like fn, ts, d, guid etc.

Closes: #47401

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (:Search/SQL)

@matriv
Copy link
Contributor Author

matriv commented Oct 4, 2019

I'm considering to backport it to earlier versions too, more like a fix.
What would you think?

Previously, we supported only the format `{fn <FUNCTION_NAME>()}`
but other DBs like MSSQL, DB2, MariaDB/MySQL alos allow whitespaces
between `{` and `fn`. Furhermore, also some applications - like PowerBI -
generate escape sequences with spaces: `select { fn name(params) } etc.`

Add support for white spaces between `{` and the escape pattern definition
like `fn`, `ts`, `d`, `guid` etc.

Closes: elastic#47401
@matriv
Copy link
Contributor Author

matriv commented Oct 4, 2019

There is another possibility to refactor the grammar like this: https://github.com/matriv/elasticsearch/compare/impl-47401...matriv:refactor-grammar?expand=1#diff-14b1f1a74854b9993f46da9467f6abcb so that we avoid the explicit (WS)*.

Personally I prefer the solution as is currently in the PR, which distinguishes the LIMIT from the LIMIT_ESC and the ESCAPE from the ESCAPE_ESC.

Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@bpintea bpintea left a comment

Choose a reason for hiding this comment

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

LGTM

@costin
Copy link
Member

costin commented Oct 7, 2019

LGTM
For the record, the JDBC spec mandates that there should be no whitespace in the header (e.g. {fn NOT { fn ).

Copy link
Member

@costin costin left a comment

Choose a reason for hiding this comment

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

LGTM

@matriv
Copy link
Contributor Author

matriv commented Oct 7, 2019

I'm considering to backport it to earlier versions too, more like a fix.
What would you think?

@costin what's your opinion on this ^ ?

@matriv
Copy link
Contributor Author

matriv commented Oct 7, 2019

@elasticmachine run elasticsearch-ci/packaging-sample

@matriv matriv merged commit 08a22d0 into elastic:master Oct 7, 2019
@matriv matriv deleted the impl-47401 branch October 7, 2019 12:37
matriv added a commit that referenced this pull request Oct 7, 2019
Previously, we supported only the format `{fn <FUNCTION_NAME>()}`
but other DBs like MSSQL, DB2, MariaDB/MySQL alos allow whitespaces
between `{` and `fn`. Furhermore, also some applications - like PowerBI -
generate escape sequences with spaces: `select { fn name(params) } etc.`

Add support for white spaces between `{` and the escape pattern definition
like `fn`, `ts`, `d`, `guid` etc.

Closes: #47401

(cherry picked from commit 08a22d0)
matriv added a commit that referenced this pull request Oct 7, 2019
Previously, we supported only the format `{fn <FUNCTION_NAME>()}`
but other DBs like MSSQL, DB2, MariaDB/MySQL alos allow whitespaces
between `{` and `fn`. Furhermore, also some applications - like PowerBI -
generate escape sequences with spaces: `select { fn name(params) } etc.`

Add support for white spaces between `{` and the escape pattern definition
like `fn`, `ts`, `d`, `guid` etc.

Closes: #47401

(cherry picked from commit 08a22d0)
matriv added a commit that referenced this pull request Oct 7, 2019
Previously, we supported only the format `{fn <FUNCTION_NAME>()}`
but other DBs like MSSQL, DB2, MariaDB/MySQL alos allow whitespaces
between `{` and `fn`. Furhermore, also some applications - like PowerBI -
generate escape sequences with spaces: `select { fn name(params) } etc.`

Add support for white spaces between `{` and the escape pattern definition
like `fn`, `ts`, `d`, `guid` etc.

Closes: #47401

(cherry picked from commit 08a22d0)
@jakelandis jakelandis removed the v8.0.0 label Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SQL: allow spaces in escape sequences
6 participants