[go: nahoru, domu]
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.

Sign up for GitHub

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

Jump to bottom

feat(parser): support parsing placeholder #16030

Closed
wants to merge 2 commits into from
Closed

feat(parser): support parsing placeholder #16030

wants to merge 2 commits into from

Conversation

lewiszlw
Copy link
Contributor
@lewiszlw lewiszlw commented Jul 11, 2024 •
edited by drmingdrmer
Loading

I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/

Summary

Support ? placeholder.
Test not passed, could someone give it a fast review as I'm not very familiar about some detail code?

+---------- Input ----------
+select * from t where a = ?
+---------- Output ---------
+error:
+  --> SQL:1:27
+  |
+1 | select * from t where a = ?
+  | ------                -   ^ missing lhs or rhs for the binary operator
+  | |                     |
+  | |                     while parsing expression
+  | while parsing `SELECT ...`
+

Tests

  • Unit Test
  • Logic Test
  • Benchmark Test
  • No Test - Explain why

Type of change

  • Bug Fix (non-breaking change which fixes an issue)
  • New Feature (non-breaking change which adds functionality)
  • Breaking Change (fix or feature that could cause existing functionality not to work as expected)
  • Documentation Update
  • Refactoring
  • Performance Improvement
  • Other (please describe):

This change is Reviewable

@lewiszlw lewiszlw marked this pull request as draft July 11, 2024 11:04
@github-actions github-actions bot added the pr-feature this PR introduces a new feature to the codebase label Jul 11, 2024
forsaken628
forsaken628 reviewed Jul 12, 2024
View reviewed changes
src/query/ast/src/ast/expr.rs
/// `?` or `$` Prepared statement arg placeholder
Placeholder {
span: Span,
name: String,
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding an offset field, which would make the parameter binding easier to implement

FYI:
https://github.com/matrixorigin/matrixone/blob/bd8054941cfdab4cd94d17cd4260d11833a102b2/pkg/sql/parsers/dialect/mysql/mysql_lexer.go#L83-L86

https://github.com/matrixorigin/matrixone/pull/3957/files#diff-6f8d2e3805b41b9704c5d638c08078e8a7e5f6812ffa344ad607746319026e1fR856

@b41sh
Copy link
Member
b41sh commented Jul 13, 2024

Hi @lewiszlw thanks for your contribution.
? is also used by JsonOperator::Question, and ? is first matched by json_op function, so placeholder is never come into effect. We need add some extra work to check whether ? is between two exprs to convert JsonOperator::Question to Placeholder.

@sundy-li
Copy link
Member
sundy-li commented Jul 15, 2024 •
edited
Loading

Is is used to prepare stmt like mysql ? We don't have a plan to support prepare stmt in databend.

It could be supported on client side (JDBC...)

@lewiszlw
Copy link
Contributor Author

Thanks for your help.
Yes, I added this on my fork for prepared stmt. Closing the pr as you don't have plan to support prepared stmt.

@lewiszlw lewiszlw closed this Jul 15, 2024
@forsaken628
Copy link
Contributor

Is is used to prepare stmt like mysql ? We don't have a plan to support prepare stmt in databend.

It could be supported on client side (JDBC...)

But here's an open issue #4620

@sundy-li
Copy link
Member

The issue is outdated. Sorry for the confusion.

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

@forsaken628 forsaken628 forsaken628 left review comments

Assignees
No one assigned
Labels
pr-feature this PR introduces a new feature to the codebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@lewiszlw @b41sh @sundy-li @forsaken628