Skip to content

Conversation

@cloud-fan
Copy link
Contributor

@cloud-fan cloud-fan commented Mar 8, 2023

What changes were proposed in this pull request?

This PR fixes a few issues of parameterized query:

  1. replace placeholders in CTE/subqueries
  2. don't replace placeholders in non-DML commands as it may store the original SQL text with placeholders and we can't resolve it later (e.g. CREATE VIEW).

Why are the changes needed?

make the parameterized query feature complete

Does this PR introduce any user-facing change?

yes, bug fix

How was this patch tested?

new tests

Copy link
Member

@MaxGekk MaxGekk left a comment

Choose a reason for hiding this comment

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

Waiting for CI.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is "query parameters" a term? I thought we go by "parameter marker". Either way we can be more descriptive.

Suggested change
"Query parameters in Command."
"Parameter markers in a DDL statement. Parameter markers must only be used in a query, or DML statement. Usage within CREATE or ALTER is unsupported."

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"PARAMETERIZED_COMMAND" : {
"PARAMETERIZED_DDL" : {

Copy link
Member

Choose a reason for hiding this comment

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

If you don't mind, can we have a negative test case for this case-sensitive?

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Also, cc @sunchao , @huaxingao , @viirya , too.

@cloud-fan
Copy link
Contributor Author

The failed BasicSchedulerIntegrationSuite is not related to this PR, I'm merging it to master/3.4, thanks for the review!

@cloud-fan cloud-fan closed this in a780703 Mar 10, 2023
cloud-fan added a commit that referenced this pull request Mar 10, 2023
…ry and CTE

### What changes were proposed in this pull request?

This PR fixes a few issues of parameterized query:
1. replace placeholders in CTE/subqueries
2. don't replace placeholders in non-DML commands as it may store the original SQL text with placeholders and we can't resolve it later (e.g. CREATE VIEW).

### Why are the changes needed?

make the parameterized query feature complete

### Does this PR introduce _any_ user-facing change?

yes, bug fix

### How was this patch tested?

new tests

Closes #40333 from cloud-fan/parameter.

Authored-by: Wenchen Fan <[email protected]>
Signed-off-by: Wenchen Fan <[email protected]>
(cherry picked from commit a780703)
Signed-off-by: Wenchen Fan <[email protected]>
@cloud-fan
Copy link
Contributor Author

This is a bug fix of a new feature in 3.4, so I won't call it a release blocker. I've set the fixed version to 3.4.0, if rc3 passes, I'll change it to 3.4.1.

@HyukjinKwon
Copy link
Member

Seems like the compliation didn't pass. Let me just quickly revert this and reopen.

@HyukjinKwon HyukjinKwon reopened this Mar 10, 2023
@cloud-fan
Copy link
Contributor Author

maybe there is a conflict right after my last commit, let me rebase

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM (Pending CIs)

@cloud-fan
Copy link
Contributor Author

GA passes, let me merge it back.

@cloud-fan cloud-fan closed this in f8966e7 Mar 10, 2023
cloud-fan added a commit that referenced this pull request Mar 10, 2023
…ry and CTE

This PR fixes a few issues of parameterized query:
1. replace placeholders in CTE/subqueries
2. don't replace placeholders in non-DML commands as it may store the original SQL text with placeholders and we can't resolve it later (e.g. CREATE VIEW).

make the parameterized query feature complete

yes, bug fix

new tests

Closes #40333 from cloud-fan/parameter.

Authored-by: Wenchen Fan <[email protected]>
Signed-off-by: Wenchen Fan <[email protected]>
(cherry picked from commit f8966e7)
Signed-off-by: Wenchen Fan <[email protected]>
snmvaughan pushed a commit to snmvaughan/spark that referenced this pull request Jun 20, 2023
…ry and CTE

This PR fixes a few issues of parameterized query:
1. replace placeholders in CTE/subqueries
2. don't replace placeholders in non-DML commands as it may store the original SQL text with placeholders and we can't resolve it later (e.g. CREATE VIEW).

make the parameterized query feature complete

yes, bug fix

new tests

Closes apache#40333 from cloud-fan/parameter.

Authored-by: Wenchen Fan <[email protected]>
Signed-off-by: Wenchen Fan <[email protected]>
(cherry picked from commit f8966e7)
Signed-off-by: Wenchen Fan <[email protected]>
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.

6 participants